[I have a very strong aversion to the kind of portability cruft which this adds to system.h. Other than that very general statement of opinion (which is to say, it isn't a statement of what should be done in this case), haven't really looked at the patch much, or thought about it much. -kingdon] Date: Tue, 20 Oct 1998 15:58:56 -0400 (EDT) To: bug-cvs@gnu.org Subject: Re: loginfo scripts and ARG_MAX... From: Eric Mumpower What do you think of integrating this patch into the cvs mainline? It's my opinion that, while perhaps imperfect, my patch increases the overall correctness of loginfo handling. Here's a rough pass at a ChangeLog entry for this: for src/ChangeLog: > Wed Sep 23 14:51:25 1998 Eric Mumpower > > * logmsg.c (title_proc, logfile_write, env_size): Extended > algorithm which handles the loginfo scripts so that it won't > ever overflow the command-line limits imposed by popen() > (which is used to invoke the loginfo scripts). (Detect > ARG_MAX and siblings, etc., imitating GNU xargs.) and for lib/ChangeLog: > Wed Sep 23 14:53:44 1998 Eric Mumpower > > * system.h: add CVS_ARG_MAX, for use by logmsg.c to avoid > overflowing popen()'s command-line-length limits. - ------- start of forwarded message (RFC 934 encapsulation) ------- From: Eric Mumpower To: info-cvs@gnu.org Subject: loginfo scripts and ARG_MAX... Date: Mon, 21 Sep 1998 21:41:59 -0400 (EDT) X-Mailing-List: archive/latest/4560 I've a patch I've written and tested somewhat which prevents the loginfo handler from assembling arg strings too long to pass to popen(). It's not the most brilliant or efficient of implementations, but it should be adequate and in general no worse than the current implementation. Plus, on systems with an ARG_MAX <= 20kb, my modification should only be noticeable in situations where popen()'s ARG_MAX would have been overflowed anyway (*). Essentially, I've added code which notes the system's alleged (*) limit on popen() and observes it, assembling multiple loginfo-script invocations to be performed if necessary. (Much the same as is done by xargs.) (For reference, when applied to the 1.10 release, my patch passes both 'make check' and 'make remotecheck'.) Comments and questions are welcome. (* "alleged" because sometimes (e.g. on Irix) it's much lower than the actual limit.) My patch, relative to the repository mainline, is enclosed: Index: src/logmsg.c =================================================================== RCS file: /home2/cvsroot/ccvs/src/logmsg.c,v retrieving revision 1.44 diff -u -r1.44 logmsg.c - - --- logmsg.c 1998/08/24 15:40:21 1.44 +++ logmsg.c 1998/09/22 01:31:25 @@ -9,6 +9,8 @@ #include "cvs.h" #include "getline.h" +extern char **environ; + static int find_type PROTO((Node * p, void *closure)); static int fmt_proc PROTO((Node * p, void *closure)); static int logfile_write PROTO((char *repository, char *filter, @@ -19,9 +21,12 @@ static void setup_tmpfile PROTO((FILE * xfp, char *xprefix, List * changes)); static int editinfo_proc PROTO((char *repository, char *template)); static int verifymsg_proc PROTO((char *repository, char *script)); +static long env_size PROTO((char **envp)); static FILE *fp; - - -static char *str_list; +static char **str_list_list; +size_t str_ll_end; +long max_sl_len; static char *str_list_format; /* The format for str_list's contents. */ static char *editinfo_editor; static char *verifymsg_script; @@ -556,6 +561,8 @@ { struct logfile_info *li; char *c; + char *str_add; + size_t newsize; li = (struct logfile_info *) p->data; if (li->type == type) @@ -566,14 +573,20 @@ You can verify that this assumption is safe by checking the code in add.c (add_directory) and import.c (import). */ - - - str_list = xrealloc (str_list, strlen (str_list) + 5); - - - (void) strcat (str_list, " "); + /* Here, we build in str_add the formatted string for this node. + When we're done assembling it, we check to see whether it would + cause the newest element of str_list_list[] to overflow our + calculated limit so as not to pass popen() too long a command. + If it would overflow, allocate a new element for str_list_list + and start using it instead. */ + str_add = xmalloc (5); + (void) strcpy (str_add, " "); if (li->type == T_TITLE) { - - - str_list = xrealloc (str_list, - - - strlen (str_list) + strlen (p->key) + 5); - - - (void) strcat (str_list, p->key); + str_add = xrealloc (str_add, + strlen (str_add) + strlen (p->key) + 5); + (void) strcat (str_add, p->key); } else { @@ -584,29 +597,29 @@ switch (*c) { case 's': - - - str_list = - - - xrealloc (str_list, - - - strlen (str_list) + strlen (p->key) + 5); - - - (void) strcat (str_list, p->key); + str_add = + xrealloc (str_add, + strlen (str_add) + strlen (p->key) + 5); + (void) strcat (str_add, p->key); break; case 'V': - - - str_list = - - - xrealloc (str_list, - - - (strlen (str_list) + str_add = + xrealloc (str_add, + (strlen (str_add) + (li->rev_old ? strlen (li->rev_old) : 0) + 10) ); - - - (void) strcat (str_list, (li->rev_old + (void) strcat (str_add, (li->rev_old ? li->rev_old : "NONE")); break; case 'v': - - - str_list = - - - xrealloc (str_list, - - - (strlen (str_list) + str_add = + xrealloc (str_add, + (strlen (str_add) + (li->rev_new ? strlen (li->rev_new) : 0) + 10) ); - - - (void) strcat (str_list, (li->rev_new + (void) strcat (str_add, (li->rev_new ? li->rev_new : "NONE")); break; /* All other characters, we insert an empty field (but @@ -617,11 +630,40 @@ } if (*(c + 1) != '\0') { - - - str_list = xrealloc (str_list, strlen (str_list) + 5); - - - (void) strcat (str_list, ","); + str_add = xrealloc (str_add, strlen (str_add) + 5); + (void) strcat (str_add, ","); } } } + + newsize = (strlen (str_list_list[str_ll_end]) + strlen (str_add) + + 5); + if (newsize < max_sl_len) + { + str_list_list[str_ll_end] = + xrealloc (str_list_list[str_ll_end], newsize); + (void) strcat (str_list_list[str_ll_end], str_add); + } + else + { + /* The resulting string would be too big! Start a new string. */ + + if (strlen(str_add) > max_sl_len) /* this shouldn't happen! */ + error (1, 0, + "Loginfo argument would be too large for popen. ('%s')", + str_add); + str_ll_end++; + str_list_list = xrealloc(str_list_list, + sizeof (char *) * (str_ll_end + 1)); + + str_list_list[str_ll_end] = xmalloc(strlen(str_add) + 5); + + /* and throw the arg we've assembled into the fresh str_list */ + (void) strcpy (str_list_list[str_ll_end], str_add); + } + + /* To be nice, free up some memory. */ + free (str_add); } return (0); } @@ -639,9 +681,10 @@ List *changes; { FILE *pipefp; - - - char *prog; + char **progs; char *cp; - - - int c; + int c, n; + int err = 0; int pipestatus; char *fmt_percent; /* the location of the percent sign that starts the format string. */ @@ -700,7 +743,8 @@ fmt_percent = strchr (filter, '%'); if (fmt_percent) { - - - int len; + int len, sublen; + long arg_max; char *srepos; char *fmt_begin, *fmt_end; /* beginning and end of the format string specified in @@ -763,11 +807,41 @@ strncpy (str_list_format, fmt_begin, len); str_list_format[len] = '\0'; - - - /* Allocate an initial chunk of memory. As we build up the string - - - we will realloc it. */ - - - if (!str_list) - - - str_list = xmalloc (1); - - - str_list[0] = '\0'; + + /* Allocate the initial list of str_lists. When necessary, it is + later grown via xrealloc. */ + if (!str_list_list) + { + str_list_list = (char **) xmalloc(sizeof (char *)); + /* Allocate an initial chunk of memory. As we build up the string + we will xrealloc it. */ + str_list_list[0] = xmalloc (1); + } + str_ll_end = 0; + (str_list_list[0])[0] = '\0'; + + /* Figure out what maximum argument size we want to be using */ + /* (Parts of this were cribbed from findutils/xargs/xargs.c) */ + + arg_max = CVS_ARG_MAX; + + /* Sanity check for systems with huge ARG_MAX defines (e.g., + Suns which have it at 1 meg). Things ought to work fine + with a large ARG_MAX but would probably hurt the system + more than it needs to. */ + if (arg_max > (20 * 1024)) + arg_max = 20 * 1024; + + /* Take the size of the environment into account. */ + arg_max -= env_size (environ); + if (arg_max <= 0) + error (1, 0, "environment is too large for exec (in loginfo)"); + + /* This is a conservative (safe low) estimate of max_sl_len + because we won't use all the text of filter and might not + use the full text of repository */ + max_sl_len = arg_max - (strlen(filter) + strlen (repository) + 10); + /* Add entries to the string. Don't bother looking for entries if the format string is empty. */ @@ -790,59 +864,79 @@ srepos = Short_Repository (repository); - - - prog = xmalloc ((fmt_percent - filter) + strlen (srepos) - - - + strlen (str_list) + strlen (fmt_continue) - - - + 10); - - - (void) strncpy (prog, filter, fmt_percent - filter); - - - prog[fmt_percent - filter] = '\0'; - - - (void) strcat (prog, "'"); - - - (void) strcat (prog, srepos); - - - (void) strcat (prog, str_list); - - - (void) strcat (prog, "'"); - - - (void) strcat (prog, fmt_continue); + + progs = (char **) xmalloc(sizeof(char *) * (str_ll_end + 1)); + + for (n = 0 ; n <= str_ll_end ; n++) + { + sublen = fmt_percent - filter; + progs[n] = xmalloc (sublen + strlen (srepos) + + strlen (str_list_list[n]) + + strlen (fmt_continue) + + 10); + (void) strncpy (progs[n], filter, sublen); + sprintf(progs[n] + sublen, "'%s%s'%s", + srepos, str_list_list[n], fmt_continue); + + free (str_list_list[n]); + str_list_list[n] = NULL; + } /* To be nice, free up some memory. */ - - - free (str_list); - - - str_list = (char *) NULL; + free (str_list_list); + str_list_list = (char **)0; } else { /* There's no format string. */ - - - prog = xstrdup (filter); - - - } + progs = (char **) xmalloc(sizeof(char *)); + str_ll_end = 0; + progs[0] = xstrdup (filter); + } + + for (n = 0 ; n <= str_ll_end ; n++) + { + if ((pipefp = run_popen (progs[n], "w")) == NULL) + { + if (!noexec) + error (0, 0, "cannot write entry to log filter: %s", progs[n]); + for (n = 0 ; n <= str_ll_end ; n++) + free (progs[n]); + free (progs); + return (1); + } + (void) fprintf (pipefp, "Update of %s\n", repository); + (void) fprintf (pipefp, "In directory %s:", hostname); + cp = xgetwd (); + if (cp == NULL) + fprintf (pipefp, "\n\n", + strerror (errno)); + else + { + fprintf (pipefp, "%s\n\n", cp); + free (cp); + } - - - if ((pipefp = run_popen (prog, "w")) == NULL) - - - { - - - if (!noexec) - - - error (0, 0, "cannot write entry to log filter: %s", prog); - - - free (prog); - - - return (1); - - - } - - - (void) fprintf (pipefp, "Update of %s\n", repository); - - - (void) fprintf (pipefp, "In directory %s:", hostname); - - - cp = xgetwd (); - - - if (cp == NULL) - - - fprintf (pipefp, "\n\n", - - - strerror (errno)); - - - else - - - { - - - fprintf (pipefp, "%s\n\n", cp); - - - free (cp); + setup_tmpfile (pipefp, "", changes); + (void) fprintf (pipefp, "Log Message:\n%s\n", message); + if (logfp != (FILE *) 0) + { + (void) fprintf (pipefp, "Status:\n"); + rewind (logfp); + while ((c = getc (logfp)) != EOF) + (void) putc ((char) c, pipefp); + } + pipestatus = pclose (pipefp); + if ((pipestatus == -1) || (pipestatus == 127)) + err++; + + free (progs[n]); + progs[n] = NULL; } - - - setup_tmpfile (pipefp, "", changes); - - - (void) fprintf (pipefp, "Log Message:\n%s\n", message); - - - if (logfp != (FILE *) 0) - - - { - - - (void) fprintf (pipefp, "Status:\n"); - - - rewind (logfp); - - - while ((c = getc (logfp)) != EOF) - - - (void) putc ((char) c, pipefp); - - - } - - - free (prog); - - - pipestatus = pclose (pipefp); - - - return ((pipestatus == -1) || (pipestatus == 127)) ? 1 : 0; + free (progs); + return(err); } /* @@ -883,4 +977,17 @@ free (verifymsg_script); verifymsg_script = xstrdup (script); return (0); +} + +/* Return how much of ARG_MAX is used by the environment. */ +static long +env_size (envp) + char **envp; +{ + long len = 0; + + while (*envp) + len += strlen (*envp++) + 1; + + return len; } Index: lib/system.h =================================================================== RCS file: /home2/cvsroot/ccvs/lib/system.h,v retrieving revision 1.39 diff -u -r1.39 system.h - - --- system.h 1998/02/16 18:55:18 1.39 +++ system.h 1998/09/22 01:31:28 @@ -261,6 +261,41 @@ # endif /* MAXPATHLEN */ #endif /* PATH_MAX */ +/* + * (Basic portability plan here was cribbed from findutils/xargs/xargs.c) + * + * For future reference: some qualities that might apply here: + * A: unistd.h provides sysconf(_SC_ARG_MAX) + * B: limits.h provides ARG_MAX + * C: experimentally, ARG_MAX has the same value as sysconf(_SC_ARG_MAX) + * + * + * all satisfy A, B, and C. + * Irix satisfies A and B (ARG_MAX is between 0.25-0.5 * sysconf(...).) + * SunOS 4.1.4 satisfies only A. + * + */ +#ifndef CVS_ARG_MAX +# ifdef HAVE_LIMITS_H +# include +# endif +# ifdef ARG_MAX +# define CVS_ARG_MAX ARG_MAX +# else +# ifdef _SC_ARG_MAX +# define CVS_ARG_MAX sysconf (_SC_ARG_MAX) +# else +# ifdef NCARGS +# define CVS_ARG_MAX NCARGS +# else +# define CVS_ARG_MAX 1024 +# endif +# endif +# endif +#endif /* CVS_ARG_MAX */ + /* The NeXT (without _POSIX_SOURCE, which we don't want) has a utime.h which doesn't define anything. It would be cleaner to have configure - ------- end ------- ------- end -------