diff --git a/server/protocol.c b/server/protocol.c index 9e23325..8428129 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -222,6 +222,12 @@ 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 */ + *read = 0; + 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 @@ -235,14 +241,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); @@ -260,7 +267,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) { @@ -273,17 +280,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? */ @@ -291,7 +289,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 */ @@ -302,7 +300,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); @@ -326,19 +324,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. @@ -358,7 +352,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)) { @@ -375,7 +369,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. @@ -392,10 +386,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; @@ -408,7 +400,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; } @@ -417,7 +408,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) { @@ -431,7 +422,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; } @@ -444,8 +435,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; @@ -484,6 +488,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);