httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "BOYA SUN" <boya....@case.edu>
Subject Re: Re: bugs/inappropriate coding practice discovered by interproceduralcode analysis for version 2.2.8 of Apache
Date Thu, 15 May 2008 03:29:00 GMT

Here is another potential bug we've just discovered, and it seems to be occured in several
places. Please also take a look at it if interested, thanks a lot!

Boya
-----------------------
Bug#7

File Name: /httpd-2.2.8/srclib/apr/file_io/unidx/readwrite.c  (63)
Function Name: apr_file_puts()
Code:
   304: APR_DECLARE(apr_status_t) apr_file_puts(const char *str, apr_file_t *thefile)
   305: {
   306:     return apr_file_write_full(thefile, str, strlen(str), NULL);
   307: }
 
Description: An error occur if apr_file_write_full() returns “!APR_SUCCESS”. According
to the above code, we infer that an error occurs if apr_file_puts() returns “!APR_SUCCESS”.
However, the return values of apr_file_puts() are not checked in the following locations.
 
 \apache\src\log.c(682):        apr_file_puts(errstr, logf);
 \apache\src\mod_cgi.c(254):    apr_file_puts("%request\n", f);
 \apache\src\mod_cgi.c(265):    apr_file_puts("%response\n", f);
 \apache\src\mod_cgi.c(291):            apr_file_puts("%stdout\n", f);
 \apache\src\mod_cgi.c(295):        apr_file_puts("\n", f);
 \apache\src\mod_cgi.c(299):        apr_file_puts("%stderr\n", f);
 \apache\src\mod_cgi.c(300):        apr_file_puts(argsbuffer, f);
 \apache\src\mod_cgi.c(303):            apr_file_puts(argsbuffer, f);
 \apache\src\mod_cgi.c(305):        apr_file_puts("\n", f);
 \apache\src\mod_cgid.c(1029):    apr_file_puts("%request\n", f);
 \apache\src\mod_cgid.c(1040):    apr_file_puts("%response\n", f);
 \apache\src\mod_cgid.c(1067):            apr_file_puts("%stdout\n", f);
 \apache\src\mod_cgid.c(1071):        apr_file_puts("\n", f);
 \apache\src\mod_cgid.c(1077):            apr_file_puts("%stderr\n", f);
 \apache\src\mod_cgid.c(1078):            apr_file_puts(argsbuffer, f);
 \apache\src\mod_cgid.c(1081):                apr_file_puts(argsbuffer, f);
 \apache\src\mod_cgid.c(1082):            apr_file_puts("\n", f); 
 




BOYA SUN
2008-05-14



发件人: Ruediger Pluem
发送时间: 2008-05-14 16:50:30
收件人: dev@httpd.apache.org
抄送: 
主题: Re: bugs/inappropriate coding practice discovered by interproceduralcode analysis
for version 2.2.8 of Apache



On 05/14/2008 10:19 PM, Paul Querna wrote:
> BOYA SUN wrote:

> 
> > *BUG#1*
> > *Category: 1*
> > *File Name:* /httpd-2.2.8/support/ab.c  *Function Name:* open_postfile()
> > *Buggy Code:*
> >   1901:     apr_file_info_get(&finfo, APR_FINFO_NORM, postfd);
> >   1902:     postlen = (apr_size_t)finfo.size;
> > *Description:* An error might occur if the output of the function 
> > apr_file_info_get() is not APR_SUCCESS. The above code does not check 
> > the return value of the function.
> 
> 
> I agree, its a bug.
> 
> Fixed in r656400:
> https://svn.apache.org/viewvc?view=rev&revision=656400
> 
> (Rudger, you hit commit 30 seconds before me, and then I got a conflict 
> when trying to commit!)

The same happened to me in the htcacheclean case but only the other way round :-).

> 
> 
> > *BUG#2*
> > *Category: 1 *
> > *File Name:* / httpd-2.2.8/srclib/apr/file_io/unix/seek.c
> > *Function Name:* apr_file_seek()
> > *Correct Code:*
> .....
> > *Function Name:* As follows
> > *Buggy Code:*
> > 229:     apr_file_flush(…); // file:/server/log.c  *Function Name:* 
> > ap_replace_stderr_log()
> >    424:     apr_file_flush(…);  // file:/server/log.c  *Function 
> > Name:* ap_open_logs() 683:     apr_file_flush(…);  // 
> > file:/server/log.c  *Function Name:* log_error_core()
> > 901:     apr_file_flush(…);  // file:/src/mod_cgi.c  *Function Name:* 
> > cgi_handler()
> > 123:     apr_file_flush(…);  // file:/src/open.c  *Function Name:* 
> > apr_file_close()
> > *Description:* As shown in the first code fragment (apr_file_seek()), 
> > the output of apr_file_flush_locked() should be checked to determine 
> > if it is APR_SUCESS. By inspecting the source code of 
> > apr_file_flush(), we infer that the output of apr_file_flush() should 
> > be checked to determine if it is APR_SUCCESS also. However, the output 
> > of apr_file_flush() is not checked in the third code fragment.  
> 
> Yup, these are all bugs.
> 
> And no, its not okay if flush fails, because APR's 'flush' call will 
> call write() if you are using buffered file IO.

I think nothing really useful can be done in these situations, but ignoring it
if things fail here.
An regarding the one in apr_file_close: The code looks different now in trunk.


> 
> > *BUG#4*
> > *Category: 2*
> > *File Name:* /httpd-2.2.8/modules/filters/mod_include.c  *Function 
> > Name:* handle_include
> > *Buggy Code
> >   1714:         else {
> >   1715:             rr = ap_sub_req_lookup_uri(parsed_string, r, 
> > f- >next);
> >   1716:         }
>   >
> > *Description:* We found a potential rule requiring that the 
> > ap_destroy_sub_req() be executed to release the object returned by 
> > ap_sub_req_lookup_uri(). However, ap_destroy_sub_req() is not called 
> > in the above code.
> 
> 
> Ah, your first miss :-)
> 
> This one isn't a bug.
> 
> While it is potentially non-optimal, due to how Pool cleanups work, we 
> can 'leak' the rr variable.  When the main request finishes, the 
> sub-pool for RR, created from the main request would be recursively 
> cleaned up.

While it is true that this finally gets cleaned up when r- >pool gets cleaned up
it is unfortunate that this happens in a loop. OTOH this problem is well known
(see comment from jorton a few lines later):

         /* Do *not* destroy the subrequest here; it may have allocated
          * variables in this r- >subprocess_env in the subrequest's
          * r- >pool, so that pool must survive as long as this request.
          * Yes, this is a memory leak. */


Regards

Rüdiger
Mime
View raw message