httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Querna <c...@force-elite.com>
Subject Re: bugs/inappropriate coding practice discovered by interprocedural code analysis for version 2.2.8 of Apache
Date Wed, 14 May 2008 20:19:54 GMT
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

Mime
View raw message