core: Ensure that ap_*getline*() return NUL terminated lines on any error.

This was done only on buffer full, so be consistent, and fail early if the
given buffer can't even hold the NUL bytes (negative or nul size).

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1824303 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 379d51f867f421de7f83820c634a5a81914eaefe..91c3208426c4b88d52e96f0990763d0dfb296fd1 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,9 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.1
 
+  *) core: For consistency, ensure that read lines are NUL terminated on any
+     error, not only on buffer full.  [Yann Ylavic]
+
   *) logresolve: Fix incorrect behavior or segfault if -c flag is used
      Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=823259
      [Stefan Fritsch]

diff --git a/server/protocol.c b/server/protocol.c
index d7e73f2339394a6743e3445158d12e71632a5da0..69634d78bbb6528b64a588285025b28e5b7f5886 100644 (file)
--- 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);