Dear Paul, Thank you so much for replying to the email. It seems that our approach is accurate in finding bugs of Category 1, but not so accurate for Category 2 which involves ordering rules. Anyway, it is a big encouragement of our work, thanks again!
Boya ----- Original Message ----- From: Paul Querna <[EMAIL PROTECTED]> Date: Wednesday, May 14, 2008 4:27 pm Subject: Re: bugs/inappropriate coding practice discovered by interprocedural code analysis for version 2.2.8 of Apache To: [email protected] > 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 >
