httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: bugs/inappropriate coding practice discovered by interprocedural code analysis for version 2.2.8 of Apache
Date Wed, 14 May 2008 20:43:31 GMT


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

Mime
View raw message