On Fri, Nov 7, 2014 at 7:17 PM, Michael Kennett <[email protected]> wrote:
> Problem - rdistd can dump core when a distfile contains cmdspecial.
...
> This occurs since the buffer cmd in dospecial() [rdistd/server.c] overflows.
> The purpose of this buffer is to store the string 'FILES=...; <cmd>" which
> is passed to an invocation of execl() in runcommand() [rdist/common.c].
Yuck. It would be better if it created an environment with
FILES=<list> directly and passed that to execle() instead of putting
the setting of the variable in the command given to the shell.
(Perhaps best to count the number of current env variables, allocate
an array for the pointers to those+1, and just put FILES=<> at the
front of the existing env, so that any other commands invoked by
rdist{,d} don't have bogus FILES= assignments in them.)
> The patch (hack?) for this is simple (noting that the preconditions of
> strunvis() are not explicitly checked...):
>
> Index: server.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/rdistd/server.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 server.c
> --- server.c 12 Jul 2014 03:10:03 -0000 1.33
> +++ server.c 8 Nov 2014 02:57:37 -0000
> @@ -462,7 +462,7 @@ clean(char *cp)
> static void
> dospecial(char *xcmd)
> {
> - char cmd[BUFSIZ];
> + char cmd[ARG_MAX];
> if (DECODE(cmd, xcmd) == -1) {
> error("dospecial: Cannot decode command.");
> return;
As you note, that doesn't solve the overflow. If something quicker
than the "pass a real env" is needed, then size out xcmd and allocate
a buffer for the strunvis.
> However, the fundamental mechanism of passing the full list of updated files
> via the environment variable FILES is broken - a long list of updated files
> will still overflow this buffer. It's not a real solution.
>
> The patch below changes the behaviour of cmdspecial in a distfile, allowing
> '-'
> to be used in the optional name list to indicate that the list of updated
> filenames should not be put into the FILES environment variable.
>
> This patch meets my needs, but I can see the value of having the list of
> updated
> files available (just not via the environment). If there are some good ideas
> and consensus on what should be done I'm willing to code them up. For example,
> to write the list of updated files into a temporary file and pass the name of
> the file via the FILELIST environemnt variable, or to pipe the contents of the
> file list to stdin...).
>
> Let me know and I'll do it.
Hmm. How about adding a new command that does the last of those,
invoking the command with the file list available on stdin? My
inclination would be to solve the newline problem at the same time and
have the file list on stdin use NUL termination on the filenames
instead of newlines; someone who is sure they can never see newlines
in filenames can always start their command with "tr '\0' '\n' |"
(I can fix up the existing cmdspecial as I described if you want to
deal with the new command...)
Philip Guenther
