serf_bucket_file_create() could use some error checks

[GoogleCodeExporter]

#if APR_HAS_MMAP
    apr_finfo_t finfo;
    const char *file_path;

    apr_file_name_get(&file_path, file);
    apr_stat(&finfo, file_path, APR_FINFO_SIZE,
             serf_bucket_allocator_get_pool(allocator));
    if (APR_MMAP_CANDIDATE(finfo.size)) {
<snip>

If apr_file_name_get fails then apr_stat might segfault.
And if the stat fails we might just try to mmap something invalid.

Original issue reported on code.google.com by b...@qqmail.nl on 17 May 2013 at 10:11

[GoogleCodeExporter]

Index: buckets/file_buckets.c
===================================================================
--- buckets/file_buckets.c  (revision 1856)
+++ buckets/file_buckets.c  (working copy)
@@ -43,6 +43,7 @@
 #if APR_HAS_MMAP
     apr_finfo_t finfo;
     const char *file_path;
+    apr_status_t status;

     /* See if we'd be better off mmap'ing this file instead.
      *
@@ -51,11 +52,11 @@
      * versions of APR, we have no way of knowing this - but apr_mmap_create
      * will check for this and return APR_EBADF.
      */
-    apr_file_name_get(&file_path, file);
-    apr_stat(&finfo, file_path, APR_FINFO_SIZE,
-             serf_bucket_allocator_get_pool(allocator));
-    if (APR_MMAP_CANDIDATE(finfo.size)) {
-        apr_status_t status;
+    status = apr_file_name_get(&file_path, file);
+    if (! status)
+      status = apr_stat(&finfo, file_path, APR_FINFO_SIZE,
+                        serf_bucket_allocator_get_pool(allocator));
+    if (! status && APR_MMAP_CANDIDATE(finfo.size)) {
         apr_mmap_t *file_mmap;
         status = apr_mmap_create(&file_mmap, file, 0, finfo.size,
                                  APR_MMAP_READ,

Original comment by b...@qqmail.nl on 18 May 2013 at 10:16

[GoogleCodeExporter]

Do you have a particular scenario that leads to this issue?

Original comment by lieven.govaerts@gmail.com on 18 May 2013 at 11:47

[GoogleCodeExporter]

No, I was just reviewing error leaks for Subversion and this one came up.

It is very unlikely that you get problems on using this code unless you pass a 
stream instead of a file. But eventually it should be fixed anyway.

Original comment by b...@qqmail.nl on 18 May 2013 at 12:37

[GoogleCodeExporter]

The issue fixed in r2007.

Original comment by chemodax@gmail.com on 9 Aug 2013 at 5:05

• Changed state: Fixed