Consistent update of invoking ap_map_http_request_error()

[牛旭 <ni...@nudt.edu.cn>]

Our team researches on the consistent edits of Httpd during evolution. And we have figured out several spots that may be missed for consistent update. They are both about invoking of function, ap_map_http_request_error(). 
We suggest to escape call of function and return the corresponding error code directly when this call is under the control dependence where return value of ap_get_brigade() does not equal to APR_SUCCESS.


one example of recommendation code snippets is listed as follows:
> 
> 
> static int hm_handler(request_rec *r)
> 
> {
> ....
> buf = apr_pcalloc(r->pool, MAX_MSG_LEN); 
> input_brigade = apr_brigade_create(r->connection->pool, 
> r->connection->bucket_alloc); 
> status = ap_get_brigade(r->input_filters, input_brigade, AP_MODE_READBYTES, APR_BLOCK_READ, MAX_MSG_LEN); 
> if (status != APR_SUCCESS) { 
> return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
> }
> ...
> }


One example of patch that involves consistent edition is:
> 
> 
> status = ap_get_brigade(r->input_filters, input_brigade,
> 
> AP_MODE_READBYTES, APR_BLOCK_READ,
> 
> HUGE_STRING_LEN);
> 
> 
> 
> 
> if (status != APR_SUCCESS) {
> 
> -            return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
> 
> +           return HTTP_BAD_REQUEST;
> 
> }
> 
> }


More recommendations and supporting patches are saved in attachments. It is so kind of you to reply me about the correctness of our suggestions. And thank you for your reading. 

Re: Re: Consistent update of invoking ap_map_http_request_error()

[牛旭 <ni...@nudt.edu.cn>]

> -----Original Messages-----
> From: "Yann Ylavic" <yl...@gmail.com>
> Sent Time: 2018-01-05 18:41:15 (Friday)
> To: httpd-dev <de...@httpd.apache.org>
> Cc: 
> Subject: Re: Consistent update of invoking ap_map_http_request_error()
> 
> Hi,
> 
> On Fri, Jan 5, 2018 at 4:56 AM, 牛旭 <ni...@nudt.edu.cn> wrote:
> > Our team researches on the consistent edits of Httpd during
> > evolution. And we have figured out several spots that may be missed
> > for consistent update.
> 
> I'm not sure what consistent means here, consistent with?
> 
> > They are both about invoking of function,
> > ap_map_http_request_error(). We suggest to escape call of function
> > and return the corresponding error code directly when this call is
> > under the control dependence where return value of ap_get_brigade()
> > does not equal to APR_SUCCESS.
> 
> Looking at the recommendations and patches, why do you the below
> change is equivalent or consitent:
> - return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
> + return HTTP_BAD_REQUEST;
> ?
> 
> The purpose of ap_map_http_request_error() is to turn a system error
> (errno, apr_status_t) to an HTTP error code (4xx, 5xx), *and* forward
> internal errors like AP_FILTER_ERROR to the core handler.
> The above change pretty much defeats that, the "status" returned by
> the preceding ap_get_brigade() may vary and
> ap_map_http_request_error() takes care of this.
> 
In this way, i guess the patches that i have discovered is against your original design purpose. Which means the following recommendations are also not feasible. Anyway, thanks for your reply.
> >
> > More recommendations and supporting patches are saved in attachments.
> > It is so kind of you to reply me about the correctness of our
> > suggestions.
> 
> Could you elaborate a bit more about these recommendations? For now I
> don't see which consistency is talked about, sorry.
> 
> 
> Regards,
> Yann.

Re: Consistent update of invoking ap_map_http_request_error()

[Yann Ylavic <yl...@gmail.com>]

Hi,

On Fri, Jan 5, 2018 at 4:56 AM, 牛旭 <ni...@nudt.edu.cn> wrote:
> Our team researches on the consistent edits of Httpd during
> evolution. And we have figured out several spots that may be missed
> for consistent update.

I'm not sure what consistent means here, consistent with?

> They are both about invoking of function,
> ap_map_http_request_error(). We suggest to escape call of function
> and return the corresponding error code directly when this call is
> under the control dependence where return value of ap_get_brigade()
> does not equal to APR_SUCCESS.

Looking at the recommendations and patches, why do you the below
change is equivalent or consitent:
- return ap_map_http_request_error(status, HTTP_BAD_REQUEST);
+ return HTTP_BAD_REQUEST;
?

The purpose of ap_map_http_request_error() is to turn a system error
(errno, apr_status_t) to an HTTP error code (4xx, 5xx), *and* forward
internal errors like AP_FILTER_ERROR to the core handler.
The above change pretty much defeats that, the "status" returned by
the preceding ap_get_brigade() may vary and
ap_map_http_request_error() takes care of this.

>
> More recommendations and supporting patches are saved in attachments.
> It is so kind of you to reply me about the correctness of our
> suggestions.

Could you elaborate a bit more about these recommendations? For now I
don't see which consistency is talked about, sorry.


Regards,
Yann.