httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boya Sun <boya....@case.edu>
Subject Re: bugs/inappropriate coding practice discovered by interprocedural code analysis for version 2.2.8 of Apache
Date Wed, 14 May 2008 20:40:45 GMT
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 <chip@force-elite.com>
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: dev@httpd.apache.org

> 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