On 3/2/23 7:21 PM, Christophe JAILLET wrote:
> Le 02/03/2023 à 16:10, [email protected] a écrit :
>> Author: ylavic
>> Date: Thu Mar 2 15:10:30 2023
>> New Revision: 1907980
>>
>> URL: http://svn.apache.org/viewvc?rev=1907980&view=rev
>> Log:
>> mod_proxy_uwsgi: Stricter backend HTTP response parsing/validation
>>
>>
>> Added:
>> httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> Modified:
>> httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c
>>
>> Added: httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt?rev=1907980&view=auto
>> ==============================================================================
>> --- httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> (added)
>> +++ httpd/httpd/trunk/changes-entries/proxy_uwsgi_response_validation.txt
>> Thu Mar 2 15:10:30 2023
>> @@ -0,0 +1,2 @@
>> + *) mod_proxy_uwsgi: Stricter backend HTTP response parsing/validation.
>> + [Yann Ylavic]
>>
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c?rev=1907980&r1=1907979&r2=1907980&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_uwsgi.c Thu Mar 2 15:10:30
>> 2023
>> @@ -313,18 +313,16 @@ static int uwsgi_response(request_rec *r
>> pass_bb = apr_brigade_create(r->pool, c->bucket_alloc);
>> len = ap_getline(buffer, sizeof(buffer), rp, 1);
>> -
>> if (len <= 0) {
>> - /* oops */
>> + /* invalid or empty */
>> return HTTP_INTERNAL_SERVER_ERROR;
>> }
>> -
>> backend->worker->s->read += len;
>> -
>> - if (len >= sizeof(buffer) - 1) {
>> - /* oops */
>> + if ((apr_size_t)len >= sizeof(buffer)) {
>
> Hi Yann,
>
> Why removing the -1?
>
> My understading is that it is there in case of:
> uwsgi_response()
> ap_getline()
> ap_rgetline()
> ap_fgetline_core()
> code around cleanup:
>
> In this path, IIUC, sizeof(buffer) - 1 is returned.
> Can this happen?
I think ap_fgetline_core can only return a len of sizeof(buffer) - 1 when:
1. The line is really that long.
2. The line is longer, but then rv != APR_SUCCESS.
In case 1. all is fine and we should proceed the result.
In case 2. ap_getline will return sizeof(buffer).
Hence I think the change is correct.
Regards
Rüdiger
