Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Apache: Dev

mod_dav_fs does not check for return value on stream_close

 

 

Apache dev RSS feed   Index | Next | Previous | View Threaded


brian at brianfrance

Mar 15, 2012, 6:56 AM

Post #1 of 3 (219 views)
Permalink
mod_dav_fs does not check for return value on stream_close

Could somebody review the patch below for 2.2, 2.4, and trunk?

A better error message could be sent, but I am more worried about how the return will effect the code after it.

I am thinking the file needs to be removed either via a apr_file_remove call or:

apr_pool_cleanup_kill(stream->p, stream, tmpfile_cleanup);

call, but I don't know the code well enough to know which is right and 2.4/trunk adds even more complexity compared to 2.2.x.

Thoughts/Comments?

Thanks,

Brian

- I am still getting more details why close is failing, but for some reason it is happening enough to cause issues. (responding 200, but no file)

-----

2.2:

Index: modules/dav/fs/repos.c
===================================================================
--- modules/dav/fs/repos.c (revision 1300964)
+++ modules/dav/fs/repos.c (working copy)
@@ -881,6 +881,10 @@
{
apr_file_close(stream->f);

+ if (status != APR_SUCCESS) {
+ return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
+ }
+
if (!commit) {
if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
/* ### use a better description? */


2.24 and trunk:


Index: modules/dav/fs/repos.c
===================================================================
--- modules/dav/fs/repos.c (revision 1300964)
+++ modules/dav/fs/repos.c (working copy)
@@ -970,6 +970,10 @@

apr_file_close(stream->f);

+ if (status != APR_SUCCESS) {
+ return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
+ }
+
if (!commit) {
if (stream->temppath) {
apr_pool_cleanup_run(stream->p, stream, tmpfile_cleanup);


minfrin at sharp

Apr 4, 2012, 3:24 PM

Post #2 of 3 (188 views)
Permalink
Re: mod_dav_fs does not check for return value on stream_close [In reply to]

On 15 Mar 2012, at 3:56 PM, Brian J. France wrote:

> Could somebody review the patch below for 2.2, 2.4, and trunk?
>
> A better error message could be sent, but I am more worried about how the return will effect the code after it.
>
> I am thinking the file needs to be removed either via a apr_file_remove call or:
>
> apr_pool_cleanup_kill(stream->p, stream, tmpfile_cleanup);
>
> call, but I don't know the code well enough to know which is right and 2.4/trunk adds even more complexity compared to 2.2.x.
>
> Thoughts/Comments?
>
> Thanks,
>
> Brian
>
> - I am still getting more details why close is failing, but for some reason it is happening enough to cause issues. (responding 200, but no file)
>
> -----
>
> 2.2:
>
> Index: modules/dav/fs/repos.c
> ===================================================================
> --- modules/dav/fs/repos.c (revision 1300964)
> +++ modules/dav/fs/repos.c (working copy)
> @@ -881,6 +881,10 @@
> {
> apr_file_close(stream->f);

Would be above not be

status = apr_file_close(stream->f);

>
> + if (status != APR_SUCCESS) {
> + return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
> + }
> +
> if (!commit) {
> if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
> /* ### use a better description? */
>
>
> 2.24 and trunk:
>
>
> Index: modules/dav/fs/repos.c
> ===================================================================
> --- modules/dav/fs/repos.c (revision 1300964)
> +++ modules/dav/fs/repos.c (working copy)
> @@ -970,6 +970,10 @@
>
> apr_file_close(stream->f);

Same with this one.

>
> + if (status != APR_SUCCESS) {
> + return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
> + }
> +
> if (!commit) {
> if (stream->temppath) {
> apr_pool_cleanup_run(stream->p, stream, tmpfile_cleanup);
>
>

Regards,
Graham
--
Attachments: smime.p7s (4.26 KB)


brian at brianfrance

Apr 4, 2012, 5:11 PM

Post #3 of 3 (187 views)
Permalink
Re: mod_dav_fs does not check for return value on stream_close [In reply to]

Yes, you are correct. Looks like my merge from work code to httpd svn didn't fully work.

We have been running this patch for a week or so with no issues.

Brian


On Apr 4, 2012, at 6:24 PM, Graham Leggett wrote:

> On 15 Mar 2012, at 3:56 PM, Brian J. France wrote:
>
>> Could somebody review the patch below for 2.2, 2.4, and trunk?
>>
>> A better error message could be sent, but I am more worried about how the return will effect the code after it.
>>
>> I am thinking the file needs to be removed either via a apr_file_remove call or:
>>
>> apr_pool_cleanup_kill(stream->p, stream, tmpfile_cleanup);
>>
>> call, but I don't know the code well enough to know which is right and 2.4/trunk adds even more complexity compared to 2.2.x.
>>
>> Thoughts/Comments?
>>
>> Thanks,
>>
>> Brian
>>
>> - I am still getting more details why close is failing, but for some reason it is happening enough to cause issues. (responding 200, but no file)
>>
>> -----
>>
>> 2.2:
>>
>> Index: modules/dav/fs/repos.c
>> ===================================================================
>> --- modules/dav/fs/repos.c (revision 1300964)
>> +++ modules/dav/fs/repos.c (working copy)
>> @@ -881,6 +881,10 @@
>> {
>> apr_file_close(stream->f);
>
> Would be above not be
>
> status = apr_file_close(stream->f);
>
>>
>> + if (status != APR_SUCCESS) {
>> + return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
>> + }
>> +
>> if (!commit) {
>> if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
>> /* ### use a better description? */
>>
>>
>> 2.24 and trunk:
>>
>>
>> Index: modules/dav/fs/repos.c
>> ===================================================================
>> --- modules/dav/fs/repos.c (revision 1300964)
>> +++ modules/dav/fs/repos.c (working copy)
>> @@ -970,6 +970,10 @@
>>
>> apr_file_close(stream->f);
>
> Same with this one.
>
>>
>> + if (status != APR_SUCCESS) {
>> + return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
>> + }
>> +
>> if (!commit) {
>> if (stream->temppath) {
>> apr_pool_cleanup_run(stream->p, stream, tmpfile_cleanup);
>>
>>
>
> Regards,
> Graham
> --
>

Apache dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.