Hi, Thank you for your valuable feedback Assaf.
Den tors 28 mars 2019 kl 18:33 skrev Assaf Gordon: > > Hello Alexander, > > On 2019-03-28 11:04 a.m., Alexander Vickberg wrote: > > This patch creates a list of unmatched HTTP headers and sets up > > environment variables before running the CGI script. > > I assume this is inspired by my (more limited) patch > of passing "Content-encoding" header: > http://lists.busybox.net/pipermail/busybox/2019-March/087141.html > (or, just a very strange timing coincidence?). > > I like your patch and of course, if it is accepted, > mine isn't needed. However unlikely it might seem I didn't see your patch until after. Besides I wanted to be able to use completely custom headers for which there has to be a general approach implementation. > two small comments: > > > @@ -417,6 +423,7 @@ struct globals { > > IF_FEATURE_HTTPD_CGI(char *host;) > > IF_FEATURE_HTTPD_CGI(char *http_accept;) > > IF_FEATURE_HTTPD_CGI(char *http_accept_language;) > > +IF_FEATURE_HTTPD_CGI(HTTP_Header *hdr_list;) > > Since your mechanism is now much more generic than > the hard-coded CGI headers, perhaps they can > be safely removed? > i.e. host/http_accept/http_accept_language/cookie/referer . > > Seems like this could save some space. Good point. I agree. > > +HTTP_Header *cur = xzalloc(sizeof(HTTP_Header)); > > +char *after_colon = strchr(iobuf, ':'); > > +char *ch = iobuf; > > + > > +if (!after_colon) > > + continue; > > + > > I think the combination of "xzalloc" + "continue" > opens the possibility of a resource leak - > if a malicious client sends lots of HTTP header lines without > a colon, there's no corresponding "free". Another good catch. I also found another issue that if the requested URL is not a CGI then the list will be populated but never freed. However I see other places which do allocs or strdups and not freeing them. I wonder if that behavior is counting on that the fork handling the request will exit and that then the kernel will free it for us? > regards, > - assaf /Alexander
_______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
