You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2018/02/15 12:57:15 UTC
svn commit: r1824303 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Author: ylavic
Date: Thu Feb 15 12:57:14 2018
New Revision: 1824303
URL: http://svn.apache.org/viewvc?rev=1824303&view=rev
Log:
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).
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/server/protocol.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1824303&r1=1824302&r2=1824303&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Thu Feb 15 12:57:14 2018
@@ -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]
Modified: httpd/httpd/trunk/server/protocol.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1824303&r1=1824302&r2=1824303&view=diff
==============================================================================
--- httpd/httpd/trunk/server/protocol.c (original)
+++ httpd/httpd/trunk/server/protocol.c Thu Feb 15 12:57:14 2018
@@ -225,6 +225,11 @@ AP_DECLARE(apr_status_t) ap_rgetline_cor
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_cor
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_cor
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_cor
/* 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_cor
/* 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_cor
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_cor
}
}
- 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_cor
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_cor
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_cor
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_cor
tmp = NULL;
}
else {
- /* We're null terminated. */
tmp = last_char;
}
@@ -420,7 +410,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_cor
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_cor
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_cor
}
}
}
+
+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
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);
Re: svn commit: r1824303 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Feb 16, 2018 at 12:40 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> On 02/16/2018 12:35 PM, Yann Ylavic wrote:
>>
>> Maybe it's used to eat/discard lines somewhere?
>
> This was also one of my thoughts. Hence the post here to gather feedback :-)
Quite memory costly by the way for more than one line, we do not
recommend this :)
Re: svn commit: r1824303 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/16/2018 12:35 PM, Yann Ylavic wrote:
> On Fri, Feb 16, 2018 at 10:57 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 02/15/2018 01:57 PM, ylavic@apache.org wrote:
>>>
>>> Modified: httpd/httpd/trunk/server/protocol.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1824303&r1=1824302&r2=1824303&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/server/protocol.c (original)
>>> +++ httpd/httpd/trunk/server/protocol.c Thu Feb 15 12:57:14 2018
>>
>>> @@ -487,6 +490,11 @@ AP_DECLARE(int) ap_getline(char *s, int
>>> apr_size_t len;
>>> apr_bucket_brigade *tmp_bb;
>>>
>>> + if (n < 1) {
>>> + /* Can't work since we always NUL terminate */
>>> + return -1;
>>> + }
>>> +
>>
>> Shouldn't we check for s != NULL as well? Otherwise the contents is read to a buffer allocated by ap_rgetline_core and
>> we only return the length. I don 't see how this usage could be useful as the content cannot be read again (otherwise
>> it would be useful to see how much content is in the pipe)
>
> While the n < 0 check protects against signed to unsigned underflow
> which could crash the process, s == NULL only affects the caller (no
> strong opinion actually).
Correct.
> Maybe it's used to eat/discard lines somewhere?
This was also one of my thoughts. Hence the post here to gather feedback :-)
Regards
Rüdiger
Re: svn commit: r1824303 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Feb 16, 2018 at 10:57 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
> On 02/15/2018 01:57 PM, ylavic@apache.org wrote:
>>
>> Modified: httpd/httpd/trunk/server/protocol.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1824303&r1=1824302&r2=1824303&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/server/protocol.c (original)
>> +++ httpd/httpd/trunk/server/protocol.c Thu Feb 15 12:57:14 2018
>
>> @@ -487,6 +490,11 @@ AP_DECLARE(int) ap_getline(char *s, int
>> apr_size_t len;
>> apr_bucket_brigade *tmp_bb;
>>
>> + if (n < 1) {
>> + /* Can't work since we always NUL terminate */
>> + return -1;
>> + }
>> +
>
> Shouldn't we check for s != NULL as well? Otherwise the contents is read to a buffer allocated by ap_rgetline_core and
> we only return the length. I don 't see how this usage could be useful as the content cannot be read again (otherwise
> it would be useful to see how much content is in the pipe)
While the n < 0 check protects against signed to unsigned underflow
which could crash the process, s == NULL only affects the caller (no
strong opinion actually).
Maybe it's used to eat/discard lines somewhere?
Regards,
Yann.
Re: svn commit: r1824303 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/15/2018 01:57 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Thu Feb 15 12:57:14 2018
> New Revision: 1824303
>
> URL: http://svn.apache.org/viewvc?rev=1824303&view=rev
> Log:
> 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).
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1824303&r1=1824302&r2=1824303&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Thu Feb 15 12:57:14 2018
> @@ -487,6 +490,11 @@ AP_DECLARE(int) ap_getline(char *s, int
> apr_size_t len;
> apr_bucket_brigade *tmp_bb;
>
> + if (n < 1) {
> + /* Can't work since we always NUL terminate */
> + return -1;
> + }
> +
Shouldn't we check for s != NULL as well? Otherwise the contents is read to a buffer allocated by ap_rgetline_core and
we only return the length. I don 't see how this usage could be useful as the content cannot be read again (otherwise
it would be useful to see how much content is in the pipe)
Regards
Rüdiger