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
