core: For consistency, ensure that read lines are NUL terminated on any

error, not only on buffer full.
trunk patch: http://svn.apache.org/r1824303
+1: ylavic, rpluem, minfrin


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

3 files changed, 45 insertions, 40 deletions

diff --git a/CHANGES b/CHANGES
index 0fad546769..fd523d0743 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,9 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.4.30
 
+  *) core: For consistency, ensure that read lines are NUL terminated on any
+     error, not only on buffer full.  [Yann Ylavic]
+
   *) mod_authnz_ldap: Fix language long names detection as short name.
      [Yann Ylavic]
 
diff --git a/STATUS b/STATUS
index c4e5c54e04..e0701fa752 100644
--- a/STATUS
+++ b/STATUS
@@ -118,12 +118,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  *) core: For consistency, ensure that read lines are NUL terminated on any
-     error, not only on buffer full.
-     trunk patch: http://svn.apache.org/r1824303
-     2.4.x patch: trunk works (modulo CHANGES)
-     +1: ylavic, rpluem, minfrin
-
 
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   [ New proposals should be added at the end of the list ]
diff --git a/server/protocol.c b/server/protocol.c
index a89e2feb24..6994633297 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -225,6 +225,11 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
     int fold = flags & AP_GETLINE_FOLD;
     int crlf = flags & AP_GETLINE_CRLF;
 
+    if (!n) {
+        /* Needs room for NUL byte at least */
+        return APR_BADARG;
+    }
+
     /*
      * Initialize last_char as otherwise a random value will be compared
      * against APR_ASCII_LF at the end of the loop if bb only contains
@@ -238,14 +243,15 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
         rv = ap_get_brigade(r->proto_input_filters, bb, AP_MODE_GETLINE,
                             APR_BLOCK_READ, 0);
         if (rv != APR_SUCCESS) {
-            return rv;
+            goto cleanup;
         }
 
         /* Something horribly wrong happened.  Someone didn't block! 
          * (this also happens at the end of each keepalive connection)
          */
         if (APR_BRIGADE_EMPTY(bb)) {
-            return APR_EGENERAL;
+            rv = APR_EGENERAL;
+            goto cleanup;
         }
 
         for (e = APR_BRIGADE_FIRST(bb);
@@ -263,7 +269,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
 
             rv = apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
             if (rv != APR_SUCCESS) {
-                return rv;
+                goto cleanup;
             }
 
             if (len == 0) {
@@ -276,17 +282,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
 
             /* Would this overrun our buffer?  If so, we'll die. */
             if (n < bytes_handled + len) {
-                *read = bytes_handled;
-                if (*s) {
-                    /* ensure this string is NUL terminated */
-                    if (bytes_handled > 0) {
-                        (*s)[bytes_handled-1] = '\0';
-                    }
-                    else {
-                        (*s)[0] = '\0';
-                    }
-                }
-                return APR_ENOSPC;
+                rv = APR_ENOSPC;
+                goto cleanup;
             }
 
             /* Do we have to handle the allocation ourselves? */
@@ -294,7 +291,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
                 /* We'll assume the common case where one bucket is enough. */
                 if (!*s) {
                     current_alloc = len;
-                    *s = apr_palloc(r->pool, current_alloc);
+                    *s = apr_palloc(r->pool, current_alloc + 1);
                 }
                 else if (bytes_handled + len > current_alloc) {
                     /* Increase the buffer size */
@@ -305,7 +302,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
                         new_size = (bytes_handled + len) * 2;
                     }
 
-                    new_buffer = apr_palloc(r->pool, new_size);
+                    new_buffer = apr_palloc(r->pool, new_size + 1);
 
                     /* Copy what we already had. */
                     memcpy(new_buffer, *s, bytes_handled);
@@ -329,19 +326,15 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
         }
     }
 
-    if (crlf && (last_char <= *s || last_char[-1] != APR_ASCII_CR)) {
-        *last_char = '\0';
-        bytes_handled = last_char - *s;
-        *read = bytes_handled;
-        return APR_EINVAL;
-    }
-
-    /* Now NUL-terminate the string at the end of the line;
+    /* Now terminate the string at the end of the line;
      * if the last-but-one character is a CR, terminate there */
     if (last_char > *s && last_char[-1] == APR_ASCII_CR) {
         last_char--;
     }
-    *last_char = '\0';
+    else if (crlf) {
+        rv = APR_EINVAL;
+        goto cleanup;
+    }
     bytes_handled = last_char - *s;
 
     /* If we're folding, we have more work to do.
@@ -361,7 +354,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
             rv = ap_get_brigade(r->proto_input_filters, bb, AP_MODE_SPECULATIVE,
                                 APR_BLOCK_READ, 1);
             if (rv != APR_SUCCESS) {
-                return rv;
+                goto cleanup;
             }
 
             if (APR_BRIGADE_EMPTY(bb)) {
@@ -378,7 +371,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
             rv = apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
             if (rv != APR_SUCCESS) {
                 apr_brigade_cleanup(bb);
-                return rv;
+                goto cleanup;
             }
 
             /* Found one, so call ourselves again to get the next line.
@@ -395,10 +388,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
             if (c == APR_ASCII_BLANK || c == APR_ASCII_TAB) {
                 /* Do we have enough space? We may be full now. */
                 if (bytes_handled >= n) {
-                    *read = n;
-                    /* ensure this string is terminated */
-                    (*s)[n-1] = '\0';
-                    return APR_ENOSPC;
+                    rv = APR_ENOSPC;
+                    goto cleanup;
                 }
                 else {
                     apr_size_t next_size, next_len;
@@ -411,7 +402,6 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
                         tmp = NULL;
                     }
                     else {
-                        /* We're null terminated. */
                         tmp = last_char;
                     }
 
@@ -420,7 +410,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
                     rv = ap_rgetline_core(&tmp, next_size,
                                           &next_len, r, 0, bb);
                     if (rv != APR_SUCCESS) {
-                        return rv;
+                        goto cleanup;
                     }
 
                     if (do_alloc && next_len > 0) {
@@ -434,7 +424,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
                         memcpy(new_buffer, *s, bytes_handled);
 
                         /* copy the new line, including the trailing null */
-                        memcpy(new_buffer + bytes_handled, tmp, next_len + 1);
+                        memcpy(new_buffer + bytes_handled, tmp, next_len);
                         *s = new_buffer;
                     }
 
@@ -447,8 +437,21 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
             }
         }
     }
+
+cleanup:
+    if (bytes_handled >= n) {
+        bytes_handled = n - 1;
+    }
+    if (*s) {
+        /* ensure the string is NUL terminated */
+        (*s)[bytes_handled] = '\0';
+    }
     *read = bytes_handled;
 
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+
     /* PR#43039: We shouldn't accept NULL bytes within the line */
     if (strlen(*s) < bytes_handled) {
         return APR_EINVAL;
@@ -487,6 +490,11 @@ AP_DECLARE(int) ap_getline(char *s, int n, request_rec *r, int flags)
     apr_size_t len;
     apr_bucket_brigade *tmp_bb;
 
+    if (n < 1) {
+        /* Can't work since we always NUL terminate */
+        return -1;
+    }
+
     tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
     rv = ap_rgetline(&tmp_s, n, &len, r, flags, tmp_bb);
     apr_brigade_destroy(tmp_bb);