Re: svn commit: r1907980 - in /httpd/httpd/trunk: changes-entries/proxy_uwsgi_response_validation.txt modules/proxy/mod_proxy_uwsgi.c
On Thu, Mar 2, 2023 at 8:22 PM Ruediger Pluem wrote: > > On 3/2/23 7:21 PM, Christophe JAILLET wrote: > > Le 02/03/2023 à 16:10, yla...@apache.org a écrit : > >> @@ -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. Yes exactly, ap_fgetline_core() returns APR_ENOSPC if the buffer is truncated, and ap_getline() returns sizeof(buffer). This change avoids failing unnecessarily for a valid response line of exactly sizeof(buffer)-1 bytes length. Regards; Yann. > > Regards > > Rüdiger >
Re: svn commit: r1907980 - in /httpd/httpd/trunk: changes-entries/proxy_uwsgi_response_validation.txt modules/proxy/mod_proxy_uwsgi.c
On 3/2/23 7:21 PM, Christophe JAILLET wrote: > Le 02/03/2023 à 16:10, yla...@apache.org 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
Re: svn commit: r1907980 - in /httpd/httpd/trunk: changes-entries/proxy_uwsgi_response_validation.txt modules/proxy/mod_proxy_uwsgi.c
Le 02/03/2023 à 16:10, yla...@apache.org 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? CJ +/* too long */ return HTTP_INTERNAL_SERVER_ERROR; } + [...]