BOYA SUN wrote:
Dear Apache-HTTPD developers,
I am a Ph.D student in the Software Engineering Research Group of EECS
department in Case Western Reserve University, under the instruction of
Prof. Andy Podgurski. In our very recent research, we applied
inter-procedural code analysis and data mining technique on the
version 2.2.8 of Apache project, and we have found 6 potential bugs, as
indicated at the end of this email. The reason why we did not submit
these bugs to the bug tracking system is that these potential bugs have
not triggered any failure, and we cannot be sure whether these
potential bugs are real bugs or just bad coding practice. We hope that
these potential bugs can help you recognize some real bugs or
inappropriate coding practice. *It would also be greately appreciated if
you can reply to this email after you have gone over the bugs and tell
us whether you have confirmed any of them*, since these information are
really valuable for us for testing our current method.
Compared to previous automated code checking systems, the issues you
highlighted have a surprisingly high correctness rate.
Note that it is also generally best to work against trunk when reporting
bugs, if possible:
<https://svn.apache.org/repos/asf/httpd/httpd/trunk/>
*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!)
*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.
*BUG#3*
*Category: 1*
*File Name:* /httpd-2.2.8/support/htcacheclean.c
*Function Name:* process_dir()
*Buggy Code:
534: apr_file_read_full(fd, &expires, len, &len);
*Description:* We found a rule requiring checking if the output of
apr_file_read_full() is APR_SUCCESS. However, the above code ignores
this check.
Bug. Fixed in r656401:
https://svn.apache.org/viewvc?view=rev&revision=656401
*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.
*BUG#5*
*Category: 2*
*File Name:* /httpd-2.2.8/modules/http/http_request.c
*Function Name:* ap_internal_fast_redirect()
*Buggy Code:*
449: ap_remove_output_filter(r->output_filters);
450: r->output_filters = r->output_filters->next;
451: }
*Description:* We found a potential rule requiring that
ap_pass_brigade() be called after the execution of
ap_remove_output_filter(). However, the above code does not follow this
potential rule.
Nope, ap_pass_brigade is commonly used in that way, for example when a
filter is done, it removes itself, and then passes the content down the
chain, but they are not dependent on each other.
*Category: 1*
*File Name:* /httpd-2.2.8/srclib/srclib/apr-util/xml/expat/lib/xmlparse.c
*Function Name:* doProlog()
*Buggy Code:*
2670: ENTITY *entity = (ENTITY *)lookup(&dtd.paramEntities,
2671:
externalSubsetName,
2672:
0);
2673: if (!externalEntityRefHandler(externalEntityRefHandlerArg,
2674:
0,
2675:
entity->base,
2676:
entity->systemId,
2677:
entity->publicId))
*Description:* The function lookup() might return NULL. In the above
code, the fields of the variable /entity/, returned by look(), are
accessed without checking if it is NULL.
This one looks like a bug.. but in expat... I guess we should look at
reporting it upstream.
-Paul
