Not a bug fix...

[Stanley Gambarin <st...@cs.bu.edu>]

	This is not a bug fix, but it could potentially prevent one :)
In mod_cgi.c, in the function log_script(), the variable argsbuffer is 
declared as char argsbuffer[HUGE_STRING_LEN].  However, later, it is used
with MAX_STRING_LEN, which is ok for now, since HUGE_STRING_LEN == 
MAX_STRING_LEN.  However, should this change, ...
	Also, the function fgets() writes at most n-1 chars into the 
buffer, so it should be HUGE_STRING_LEN, not HUGE_STRING_LEN, but then
again, it's just my opinion.

							Stanley.

*** mod_cgi.c	Mon Apr 21 16:29:09 1997
--- mod_cgi2.c	Mon Jul  7 23:30:40 1997
***************
*** 197,205 ****
  	((f = pfopen(r->pool, server_root_relative(r->pool, conf->logname),
  		     "a")) == NULL)) {
        /* Soak up script output */
!       while (fgets(argsbuffer, MAX_STRING_LEN-1, script_in) != NULL)
  	continue;
!       while (fgets(argsbuffer, MAX_STRING_LEN-1, script_err) != NULL)
  	continue;
        return ret;
      }
--- 197,205 ----
  	((f = pfopen(r->pool, server_root_relative(r->pool, conf->logname),
  		     "a")) == NULL)) {
        /* Soak up script output */
!       while (fgets(argsbuffer, HUGE_STRING_LEN, script_in) != NULL)
  	continue;
!       while (fgets(argsbuffer, HUGE_STRING_LEN, script_err) != NULL)
  	continue;
        return ret;
      }

Re: Not a bug fix...

[Marc Slemko <ma...@worldgate.com>]

On Mon, 7 Jul 1997, Stanley Gambarin wrote:

> 
> 	This is not a bug fix, but it could potentially prevent one :)

Yes it is a bug fix; just because it compiles into exactly the same code
now is no reason to say it isn't a bug.  Patch applied to what you mention
and a removed a few more -1s in what is now bgets() in the source tree. 
Didn't touch anything outside of mod_cgi though. 

> In mod_cgi.c, in the function log_script(), the variable argsbuffer is 
> declared as char argsbuffer[HUGE_STRING_LEN].  However, later, it is used
> with MAX_STRING_LEN, which is ok for now, since HUGE_STRING_LEN == 
> MAX_STRING_LEN.  However, should this change, ...

In many of the places where MAX_STRING_LEN or HUGE_STRING_LEN is used, it
is completely bogus.  We have it being used for a sprintf buffer for an
int (which... well... doesn't normally get quite that long until your
machine is a hell of a lot more than 128 bits), it is used in other places
with the tacit assumping that it will be long enough, etc.  I encountered
so many hundred of these cases when I did my big patch for buffer
overflows a while ago that I was almost sick, until I remembered NCSA and
that made me scared but happier about Apache.

We can not even think about touching either of the two defines without
going through each and every bit of code that uses them.  Sigh.  It is
ugly.

> 	Also, the function fgets() writes at most n-1 chars into the 
> buffer, so it should be HUGE_STRING_LEN, not HUGE_STRING_LEN, but then
> again, it's just my opinion.

I would suggest it is bogus to use -1 when the interface to the call
specifically states you don't have to, just like it is bogus to subtract
one from the size given to snprintf, and just like it is bogus to _not_
subtract one from the size given to strncpy.

Thanks for pointing it out.