INN commit: branches/2.6 (3 files)
INN Commit
rra at isc.org
Thu Dec 24 22:33:03 UTC 2020
Date: Thursday, December 24, 2020 @ 14:33:03
Author: eagle
Revision: 10467
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:
branches/2.6/nnrpd/auth-ext.c
branches/2.6/tests/nnrpd/auth-ext-t.c
branches/2.6/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 22:32:34 UTC (rev 10466)
+++ nnrpd/auth-ext.c 2020-12-24 22:33:03 UTC (rev 10467)
@@ -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 22:32:34 UTC (rev 10466)
+++ tests/nnrpd/auth-ext-t.c 2020-12-24 22:33:03 UTC (rev 10467)
@@ -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 22:32:34 UTC (rev 10466)
+++ tests/nnrpd/auth-test 2020-12-24 22:33:03 UTC (rev 10467)
@@ -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