[as-devel] read() on a socket in *BSD
Frederick Bruckman (fb@enteract.com)
Mon, 14 Jun 1999 12:09:32 -0500 (CDT)
The problem with *BSD and 1.7.111, is that, according to the read()
man page for NetBSD-1.4, read() on a socket is not guaranteed to
return any bytes at all. Empirically, if the socket is non-blocking,
it often returns -1 and then errno=EAGAIN, "Resource temporarily
unavailable." Somehow this leads to a corrupt data stream, message:
"Module command is too big."
The following patches uses recvfrom() with options to "look ahead" in
the socket, to be sure the module actually sent the required number of
bytes, and then goes into a tight loop, to fetch all the data in the
pipe (again). Testing now on netbsd-1.4-mac68k and netbsd-1.4-i386.
All the socket related errors seem to be gone.
Besides the obvious problem, that every line is read twice, there's no
guarantee that the long peek will ever succeed, even if it's only
apparent with a long command and a slow machine. The pointer
arithmetic is evil. And also, configure needs to test for recvfrom()
to make anything like this portable.
I feel it would be a cleaner design to set up an input Queue for each
connection, which would be a union of a *char and the Command
structure. This promises to make receipt of module commands more
robust on all ports.
Evidently, linux now gets away with killing the sender on all short
reads--but this is wrong. There's can be no guarantee that the read
will return the requested number of bytes unless you make it block,
which is obviously undesirable. The status quo could break suddenly,
e.g., with a kernel upgrade.
--- src/afterstep/events.c.orig Thu Jun 10 13:39:24 1999
+++ src/afterstep/events.c Sun Jun 13 04:52:48 1999
@@ -25,9 +25,11 @@
#include <sys/bsdtypes.h>
#endif
+#include <errno.h>
#include <limits.h>
#include <stdio.h>
#include <string.h>
+#include <sys/socket.h>
#include <sys/types.h>
#include <sys/time.h>
#include <unistd.h>
@@ -1625,10 +1627,11 @@
{
if (module_pipe[i] >= 0 && FD_ISSET (module_pipe[i], &in_fdset))
{
- if ((count = read (module_pipe[i], &targetWindow, sizeof (Window))) > 0)
- HandleModuleInput (targetWindow, i);
- if (count <= 0)
+ if ((count = recvfrom (module_pipe[i], &targetWindow, sizeof (Window),
+ MSG_WAITALL|MSG_PEEK, NULL, NULL)) == -1 && errno != EAGAIN)
KillModule (i, 100);
+ else if (count == sizeof (Window))
+ HandleModuleInput (targetWindow, i);
}
if (module_pipe[i] >= 0 && FD_ISSET (module_pipe[i], &out_fdset))
FlushQueue (i);
--- src/afterstep/module.c.orig Wed Apr 7 02:04:46 1999
+++ src/afterstep/module.c Sun Jun 13 13:12:44 1999
@@ -327,45 +327,101 @@
int
HandleModuleInput (Window w, int channel)
{
- char text[256];
- int size;
- int cont, n;
+ struct
+ {
+ Window w;
+ int size;
+ char text[256];
+ int cont;
+ } Command;
+ int m, n, p;
char *newaction = NULL;
- /* Already read a (possibly NULL) window id from the pipe,
- * Now read an afterstep bultin command line */
- n = read (module_pipe[channel], &size, sizeof (int));
- if (n < sizeof (int))
+ /* get size of command text */
+ m = sizeof (Window) + sizeof (int);
+ if ((n = recvfrom (module_pipe[channel], &Command, m, MSG_WAITALL|MSG_PEEK, NULL, NULL)) < m)
{
- KillModule (channel, 1);
+ if (errno == EAGAIN || errno == EINTR)
+ return -2; /* try again next time */
+ else
+ {
+ perror ("Couldn't get module command size");
+ KillModule (channel, 1);
+ return -1;
+ }
+ }
+ if (Command.size > 255)
+ {
+ fprintf (stderr, "Module command is too big (%d)\n", Command.size);
+ KillModule (channel, 2);
return -1;
}
- if (size > 255)
+
+ /*
+ Verify that the entire message is in the socket buffer.
+ On heavily loaded machines with slow i/o, this might never complete!
+ */
+ m = sizeof (Window) + sizeof (int) + Command.size + sizeof (int);
+ if ((n = recvfrom (module_pipe[channel], &Command, m, MSG_WAITALL|MSG_PEEK, NULL, NULL)) < m)
{
- fprintf (stderr, "Module command is too big (%d)\n", size);
- size = 255;
+ if (errno == EAGAIN || errno == EINTR)
+ return -2; /* try again next time */
+ else
+ {
+ perror ("Couldn't get entire module command");
+ KillModule (channel, 3);
+ return -1;
+ }
}
- pipeOn[channel] = 1;
- n = read (module_pipe[channel], text, size);
- if (n == -1)
- perror (MyName);
- if (n < size)
+ /* should be OK to do this now */
+ m = n = sizeof (Window) + sizeof (int) + Command.size;
+ while (n)
{
- KillModule (channel, 2);
- return -1;
+ p = recvfrom (module_pipe[channel], &Command + m - n, n, 0, NULL, NULL);
+ if (p == -1)
+ {
+ if (errno == EAGAIN || errno == EINTR)
+ p = 0;
+ else
+ {
+ perror ("Module command aborted");
+ KillModule (channel, 5);
+ return -1;
+ }
+ }
+ n = n - p;
}
- text[n] = 0;
- n = read (module_pipe[channel], &cont, sizeof (int));
- if (n < sizeof (int))
+
+ m = n = sizeof (int);
+ while (n)
{
- KillModule (channel, 3);
+ p = recvfrom (module_pipe[channel], &Command.cont + m - n, n, 0, NULL, NULL);
+ if (p == -1)
+ {
+ if (errno == EAGAIN)
+ p = 0;
+ else
+ {
+ perror ("Module command not finished");
+ KillModule (channel, 7);
+ return -1;
+ }
+ }
+ n = n - p;
+ }
+
+ if (Command.cont != 1)
+ {
+ fprintf (stderr, "Module command protocol error\n");
+ KillModule (channel, 8);
return -1;
}
- if (!cont)
- KillModule (channel, 4);
- if (strlen (text))
+ pipeOn[channel] = 1;
+ Command.text[Command.size] = 0;
+
+ if (strlen (Command.text))
{
char function[256], *ptr;
MenuRoot *mr = 0;
@@ -378,7 +434,7 @@
char unit_1, unit_2;
Bool update = False;
- orig_tline = text;
+ orig_tline = Command.text;
Event.xany.type = ButtonRelease;
Event.xany.window = w;
@@ -386,9 +442,10 @@
func_val_2 = 0;
unit_1 = 's';
unit_2 = 's';
- n = sscanf (text, "%s %d %d", function, &func_val_1, &func_val_2);
+ n = sscanf (Command.text, "%s %d %d", function, &func_val_1, &func_val_2);
if (n != 3)
- n = sscanf (text, "%s %d%c %d%c", function, &func_val_1, &unit_1, &func_val_2, &unit_2);
+ n = sscanf (Command.text, "%s %d%c %d%c", function, &func_val_1, &unit_1,
+ &func_val_2, &unit_2);
if (mystrcasecmp (function, "SET_MASK") == 0)
{
@@ -402,8 +459,9 @@
if (pipeName[channel] != NULL)
free (pipeName[channel]);
- for (ptr = text + strlen(text) - 1 ; ptr >= text && isspace(*ptr) ; *ptr-- = '\0');
- for (ptr = text ; isspace(*ptr) ; ptr++);
+ for (ptr = Command.text + strlen(Command.text) - 1 ;
+ ptr >= Command.text && isspace(*ptr) ; *ptr-- = '\0');
+ for (ptr = Command.text ; isspace(*ptr) ; ptr++);
for ( ; *ptr != '\0' && !isspace(*ptr) ; ptr++);
for ( ; isspace(*ptr) ; ptr++);
pipeName[channel] = mystrdup(ptr);
@@ -458,7 +516,7 @@
match_string (func_config, function, "bad module function:", NULL);
if ((func == F_POPUP) || (func == F_FUNCTION))
{
- ptr = stripcpy2 (text, 0);
+ ptr = stripcpy2 (Command.text, 0);
if (ptr != NULL)
for (mr = Scr.first_menu; mr != NULL; mr = (*mr).next)
if (mystrcasecmp ((*mr).name, ptr) == 0)
@@ -477,14 +535,14 @@
if (func == F_EXEC || func == F_RESTART || func == F_QUICKRESTART ||
func == F_MODULE || func == F_CHANGE_BACKGROUND)
{
- item = stripcpy2 (text, 0);
- newaction = stripcpy3 (text, True);
+ item = stripcpy2 (Command.text, 0);
+ newaction = stripcpy3 (Command.text, True);
free (item);
}
else
{
- item = stripcpy2 (text, 0);
- newaction = stripcpy3 (text, False);
+ item = stripcpy2 (Command.text, 0);
+ newaction = stripcpy3 (Command.text, False);
free (item);
}
}
@@ -756,14 +814,19 @@
FlushQueue (module);
- while ((e = read (module_pipe[module], &targetWindow, sizeof (Window))) > 0)
- {
- if (HandleModuleInput (targetWindow, module) == 66)
- break;
+ while (1)
+ {
+ e = recvfrom (module_pipe[module], &targetWindow, sizeof (Window),
+ MSG_WAITALL|MSG_PEEK, NULL, NULL);
+ if (e <= 0 && errno != EAGAIN)
+ {
+ KillModule (module, 10);
+ break;
+ }
+ else if (e == sizeof (Window))
+ if (HandleModuleInput (targetWindow, module) == 66)
+ break;
}
-
- if (e <= 0)
- KillModule (module, 10);
}
return size;