INN commit: trunk (3 files)

INN Commit rra at isc.org
Thu Dec 24 22:25:01 UTC 2020


    Date: Thursday, December 24, 2020 @ 14:25:01
  Author: eagle
Revision: 10464

Fix some subtle errors with nnrpd external auth

If nnrpd saw EOF on the stderr file descriptor from an external
auth program, it would ignore any further output on stdout.  This
was the cause of the maddeningly intermittant nnrpd/auth-ext test
failures for tests 56 and 59.  Sometimes the closure of stderr
would be seen before the flush of output on stdout, causing the
code to fail to see the username.

Fix this problem with more explicit state tracking for stderr.  If
we see EOF or an error in the stderr file descriptor, remove it
from the select set but continue processing stdout until we also
see an EOF or error there.

Add a new test that explicitly closes stderr.  This failed
consistently with the previous code and passes consistently with this
code.

Also close the read and error file descriptors for the external
authentication program in nnrpd.  Previously, we were leaking those
file descriptors.

Modified:
  trunk/nnrpd/auth-ext.c
  trunk/tests/nnrpd/auth-ext-t.c
  trunk/tests/nnrpd/auth-test

--------------------------+
 nnrpd/auth-ext.c         |   38 ++++++++++++++++++++++++++++----------
 tests/nnrpd/auth-ext-t.c |   24 ++++++++++++------------
 tests/nnrpd/auth-test    |    4 ++++
 3 files changed, 44 insertions(+), 22 deletions(-)

Modified: nnrpd/auth-ext.c
===================================================================
--- nnrpd/auth-ext.c	2020-12-24 21:56:06 UTC (rev 10463)
+++ nnrpd/auth-ext.c	2020-12-24 22:25:01 UTC (rev 10464)
@@ -134,8 +134,10 @@
     count = buffer_read(buffer, fd);
     if (buffer->left >= buffer->size - 1)
         return -1;
-    if (count < 0)
+    if (count < 0) {
+        syswarn("%s auth: read error", client->host);
         return count;
+    }
 
     /* If reached end of file, process anything left as a line. */
     if (count == 0) {
@@ -185,7 +187,8 @@
     struct buffer *readbuf;
     struct buffer *errorbuf;
     double start, end;
-    bool found;
+    bool done;
+    bool error_done = false;
     bool killed = false;
     bool errored = false;
     char *user = NULL;
@@ -209,6 +212,8 @@
         status = select(maxfd + 1, &rfds, NULL, NULL, &tv);
         end = TMRnow_double();
         IDLEtime += end - start;
+
+        /* Check for select timeout or errors. */
         if (status <= 0) {
             if (status == 0)
                 syswarn("%s auth: program timeout", client->host);
@@ -221,23 +226,31 @@
             kill(process->pid, SIGTERM);
             break;
         }
-        found = false;
+
+        /* Read from the read and error file descriptors.  If we see EOF or an
+         * error from the read file descriptor, we're done.  If we see EOF or
+         * an error from the error file descriptor, remove it from the set but
+         * continue waiting for output from the read file descriptor .*/
+        done = false;
         count = 0;
         if (FD_ISSET(process->read_fd, &rfds)) {
             fd = process->read_fd;
             count = output(client, fd, readbuf, handle_result, &user);
-            if (count > 0)
-                found = true;
+            if (count <= 0)
+                done = true;
         }
-        if (count >= 0 && FD_ISSET(process->error_fd, &rfds)) {
+        if (!error_done && FD_ISSET(process->error_fd, &rfds)) {
             fd = process->error_fd;
             count = output(client, fd, errorbuf, handle_error, &user);
-            if (count > 0) {
-                found = true;
+            if (count <= 0) {
+                error_done = true;
+                FD_CLR(fd, &fds);
+                maxfd = process->read_fd;
+            }
+            if (count > 0)
                 errored = true;
-            }
         }
-        if (!found) {
+        if (done) {
             close(process->read_fd);
             close(process->error_fd);
             if (count < 0) {
@@ -259,6 +272,9 @@
         syswarn("%s auth: cannot wait for program", client->host);
         goto fail;
     }
+
+    /* Check the exit status and fall through to fail if the external
+     * authentication program didn't exit successfully. */
     if (WIFEXITED(status) && WEXITSTATUS(status) == 0)
         return user;
     else {
@@ -331,6 +347,8 @@
 
     /* Get the results. */
     user = handle_output(client, process);
+    close(process->read_fd);
+    close(process->error_fd);
     free(process);
     return user;
 }

Modified: tests/nnrpd/auth-ext-t.c
===================================================================
--- tests/nnrpd/auth-ext-t.c	2020-12-24 21:56:06 UTC (rev 10463)
+++ tests/nnrpd/auth-ext-t.c	2020-12-24 22:25:01 UTC (rev 10464)
@@ -9,6 +9,7 @@
 #include "inn/messages.h"
 #include "tap/basic.h"
 #include "tap/messages.h"
+#include "tap/string.h"
 
 #include "../../nnrpd/nnrpd.h"
 
@@ -75,12 +76,15 @@
 test_external(struct client *client, const char *arg, const char *user,
               const char *error)
 {
-    char *result;
-    char *command;
+    char *auth_test_path, *result, *command;
 
     diag("mode %s", arg);
 
-    command = concat("auth-test ", arg, (char *) 0);
+    auth_test_path = test_file_path("nnrpd/auth-test");
+    if (auth_test_path == NULL)
+        bail("cannot find nnrpd/auth-test helper");
+
+    basprintf(&command, "%s %s", auth_test_path, arg);
     errors_capture();
     result = auth_external(client, command, ".", NULL, NULL);
     errors_uncapture();
@@ -102,6 +106,8 @@
         warn("%s", errors);
     free(errors);
     errors = NULL;
+
+    test_file_path_free(auth_test_path);
 }
 
 int
@@ -109,17 +115,10 @@
 {
     struct client *client;
 
-    if (access("auth-test", F_OK) < 0) {
-        if (access("nnrpd/auth-test", F_OK) == 0) {
-            if (chdir("nnrpd") < 0) {
-                sysbail("cannot chdir to nnrpd");
-            }
-        }
-    }
+    plan(12 * 6);
+
     client = client_new();
 
-    plan(11 * 6);
-
     test_external(client, "okay", "tester", NULL);
     test_external(client, "garbage", "tester", NULL);
     test_external(client, "error", NULL,
@@ -135,6 +134,7 @@
                   "example.com auth: program caught signal 1\n");
     test_external(client, "newline", "tester", NULL);
     test_external(client, "partial", "tester", NULL);
+    test_external(client, "partial-close", "tester", NULL);
     test_external(client, "partial-error", NULL,
                   "example.com auth: program error: This is an error\n");
 

Modified: tests/nnrpd/auth-test
===================================================================
--- tests/nnrpd/auth-test	2020-12-24 21:56:06 UTC (rev 10463)
+++ tests/nnrpd/auth-test	2020-12-24 22:25:01 UTC (rev 10464)
@@ -53,6 +53,10 @@
 partial)
     printf 'User:tester'
     ;;
+partial-close)
+    exec 2>&-
+    printf 'User:tester'
+    ;;
 partial-error)
     printf 'User:tester'
     echo 'This is an error' >&2



More information about the inn-committers mailing list