slotmem_shm: Error detection

trunk patch: https://svn.apache.org/viewvc?view=revision&revision=1540161
             https://svn.apache.org/viewvc?view=revision&revision=1540163
             https://svn.apache.org/viewvc?view=revision&revision=1540178
             https://svn.apache.org/viewvc?view=revision&revision=1540179
             https://svn.apache.org/viewvc?view=revision&revision=1540220
             http://svn.apache.org/viewvc?view=revision&revision=r1542413
2.4.x patch: http://people.apache.org/~jim/patches/slotmem-error2.patch

Submitted by: jim
Reviewed by: jorton, minfrin

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1542803 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 29286d236bdfd620e2f066faa202c07f06fe653e..3dab2228eded94db7d709eed1f4c41a74e0a8335 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,8 @@
 
 Changes with Apache 2.4.7
 
+  *) slotmem_shm: Error detection. [Jim Jagielski]
+
   *) event: Use skiplist data structure (requires APR 1.5).
      [Jim Jagielski]
 

diff --git a/STATUS b/STATUS
index 19fe4d62e1d5e62f61e8b13abf9731feb6d71f15..ae2a2ba0dfb092858e8718df8a01ce324ce85e0b 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -97,15 +97,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  * slotmem_shm: Error detection
-    trunk patch: https://svn.apache.org/viewvc?view=revision&revision=1540161
-                 https://svn.apache.org/viewvc?view=revision&revision=1540163
-                 https://svn.apache.org/viewvc?view=revision&revision=1540178
-                 https://svn.apache.org/viewvc?view=revision&revision=1540179
-                 https://svn.apache.org/viewvc?view=revision&revision=1540220
-                 http://svn.apache.org/viewvc?view=revision&revision=r1542413
-    2.4.x patch: http://people.apache.org/~jim/patches/slotmem-error2.patch
-    +1: jim, jorton, minfrin
 
 
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:

diff --git a/include/ap_slotmem.h b/include/ap_slotmem.h
index b9c810366dde7a8bc2613ee5a3a30b63d33c80c6..e1615e91e8a6c762cdeba86f1f81975f718f24b9 100644 (file)
--- a/include/ap_slotmem.h
+++ b/include/ap_slotmem.h
@@ -39,6 +39,7 @@
 #include "apr_shm.h"
 #include "apr_global_mutex.h"
 #include "apr_file_io.h"
+#include "apr_md5.h"
 
 #if APR_HAVE_UNISTD_H
 #include <unistd.h>         /* for getpid() */

diff --git a/modules/slotmem/mod_slotmem_shm.c b/modules/slotmem/mod_slotmem_shm.c
index ae6daadb1bb7eb32ab7aac46ad0e8c1de01042fe..1f7e557cd9dc0ba681070ad24a7aa8bff14e1e86 100644 (file)
--- a/modules/slotmem/mod_slotmem_shm.c
+++ b/modules/slotmem/mod_slotmem_shm.c
@@ -178,6 +178,7 @@ static void store_slotmem(ap_slotmem_instance_t *slotmem)
     apr_status_t rv;
     apr_size_t nbytes;
     const char *storename;
+    unsigned char digest[APR_MD5_DIGESTSIZE];
 
     storename = slotmem_filename(slotmem->gpool, slotmem->name, 1);
 
@@ -200,20 +201,27 @@ static void store_slotmem(ap_slotmem_instance_t *slotmem)
         }
         nbytes = (slotmem->desc.size * slotmem->desc.num) +
                  (slotmem->desc.num * sizeof(char)) + AP_UNSIGNEDINT_OFFSET;
-        /* XXX: Error handling */
-        apr_file_write_full(fp, slotmem->persist, nbytes, NULL);
+        apr_md5(digest, slotmem->persist, nbytes);
+        rv = apr_file_write_full(fp, slotmem->persist, nbytes, NULL);
+        if (rv == APR_SUCCESS) {
+            rv = apr_file_write_full(fp, digest, APR_MD5_DIGESTSIZE, NULL);
+        }
         apr_file_close(fp);
+        if (rv != APR_SUCCESS) {
+            apr_file_remove(storename, slotmem->gpool);
+        }
     }
 }
 
-/* should be apr_status_t really */
-static void restore_slotmem(void *ptr, const char *name, apr_size_t size,
-                            apr_pool_t *pool)
+static apr_status_t restore_slotmem(void *ptr, const char *name, apr_size_t size,
+                                    apr_pool_t *pool)
 {
     const char *storename;
     apr_file_t *fp;
     apr_size_t nbytes = size;
-    apr_status_t rv;
+    apr_status_t rv = APR_SUCCESS;
+    unsigned char digest[APR_MD5_DIGESTSIZE];
+    unsigned char digest2[APR_MD5_DIGESTSIZE];
 
     storename = slotmem_filename(pool, name, 1);
 
@@ -224,20 +232,42 @@ static void restore_slotmem(void *ptr, const char *name, apr_size_t size,
         rv = apr_file_open(&fp, storename, APR_READ | APR_WRITE, APR_OS_DEFAULT,
                            pool);
         if (rv == APR_SUCCESS) {
-            apr_finfo_t fi;
-            if (apr_file_info_get(&fi, APR_FINFO_SIZE, fp) == APR_SUCCESS) {
-                if (fi.size == nbytes) {
-                    apr_file_read(fp, ptr, &nbytes);
+            rv = apr_file_read(fp, ptr, &nbytes);
+            if ((rv == APR_SUCCESS || rv == APR_EOF) && nbytes == size) {
+                rv = APR_SUCCESS;   /* for successful return @ EOF */
+                /*
+                 * if at EOF, don't bother checking md5
+                 *  - backwards compatibility
+                 *  */
+                if (apr_file_eof(fp) != APR_EOF) {
+                    apr_size_t ds = APR_MD5_DIGESTSIZE;
+                    rv = apr_file_read(fp, digest, &ds);
+                    if ((rv == APR_SUCCESS || rv == APR_EOF) &&
+                        ds == APR_MD5_DIGESTSIZE) {
+                        rv = APR_SUCCESS;
+                        apr_md5(digest2, ptr, nbytes);
+                        if (memcmp(digest, digest2, APR_MD5_DIGESTSIZE)) {
+                            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
+                                         APLOGNO(02551) "bad md5 match");
+                            rv = APR_EGENERAL;
+                        }
+                    }
                 }
                 else {
-                    apr_file_close(fp);
-                    apr_file_remove(storename, pool);
-                    return;
+                    ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
+                                 APLOGNO(02552) "at EOF... bypassing md5 match check (old persist file?)");
                 }
             }
+            else if (nbytes != size) {
+                ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
+                             APLOGNO(02553) "Expected %" APR_SIZE_T_FMT ": Read %" APR_SIZE_T_FMT,
+                             size, nbytes);
+                rv = APR_EGENERAL;
+            }
             apr_file_close(fp);
         }
     }
+    return rv;
 }
 
 static apr_status_t cleanup_slotmem(void *param)
@@ -395,8 +425,16 @@ static apr_status_t slotmem_create(ap_slotmem_instance_t **new,
          * sense if the restore fails? Any?
          */
         if (type & AP_SLOTMEM_TYPE_PERSIST) {
-            restore_slotmem(ptr, fname, dsize, pool);
-            restored = 1;
+            rv = restore_slotmem(ptr, fname, dsize, pool);
+            if (rv == APR_SUCCESS) {
+                restored = 1;
+            }
+            else {
+                /* just in case, re-zero */
+                ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf,
+                             APLOGNO(02554) "could not restore %s", fname);
+                memset(ptr, 0, dsize);
+            }
         }
     }