You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Brian J. France" <br...@brianfrance.com> on 2012/03/15 14:56:40 UTC
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);
Re: mod_dav_fs does not check for return value on stream_close
Posted by "Brian J. France" <br...@brianfrance.com>.
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
> --
>
Re: mod_dav_fs does not check for return value on stream_close
Posted by Graham Leggett <mi...@sharp.fm>.
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
--