Re: mod_dav_fs does not check for return value on stream_close
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
--
smime.p7s
Description: S/MIME cryptographic signature
Re: mod_dav_fs does not check for return value on stream_close
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
--
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);
