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
收件人： [email protected]
抄送：
主题： 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
