httpd: memory hole in function addEnv() and more

[Rob Landley]

On Sunday 04 September 2005 03:06, Dirk Clemens wrote:

> > And since this could be used repeatedly from a long-running script,
> it's not
> > even a FEATURE_CLEAN_UP free, either.
>
> The problem is a long running http-server (if not startet via inetd).



> > Too bad there's no obvious way to apply alloca() here.  Hmmm...
>
> We know the maximum needed size of the temp buffer within one
> call of sendCgi():

Considering that the URL is of arbitrar arbitrary length...

>  needed_size = max_env_var_name + maximum( stdlen(uri), MAX_PATH) + 2;
>
> At the very beginning of sendCgi() we could alloca the needed memory:
>
>   const int url_len = strlen(url);
>   int envbuf_size = 50 + ( url_len > MAX_PATH ? url_len : MAX_PATH  );
>   char * envbuf = alloca(envbuf_size);

And if url_len is greater than MAX_PATH we just wander off the end, then?

Is there anything specifying the maximum length a URL can be, with form data 
and all?

> And for every env-var:
>
>   snprintf(envbuf,envbuf_size,"NAME=%s",value);
>   putenv(envbuf);

It's still possible that numerous calls to one wrapper with two arguments is 
going to be smaller than numerous calls to two functions with a total of four 
arguments.  The wrapper isn't all that big, and a simplification that makes 
the binary bigger is a hard sell here...

And if the malloc/free take place only in the wrapper, then from a complexity 
standpoint it's not so bad.  (From a runtime standpoint it's almost certainly 
slower, but that's about third on what we're optimizing for.  Binary size, 
run-time memory usage, speed and least complexity are all in there fighting 
for attention...)

> >>I think that we can save memory and cpu time if we change such lines into
> >>    putenv("SERVER_PROTOCOL=HTTP/1.0");
> >
> > I heartily support this cleanup. :)
>
> If you give your ok I will do all this work in this week.

You don't need my ok to do the work. :)

It sounds like a good idea.  I haven't seen the final patch, and others may 
argue.  (Erik's is the final word, but I have SVN access and a tendency to 
commit things that look good to shake complaints out of the woodwork...)

> I'll take the httpd.c file from the trunk as startup?

Yup.  Always provide patches against the current contents of the repository 
you'd like us to check it into.

> Dirk

Rob