Our detectors have identified the following error code mitigation. In httpd/modules/http2/h2_conn.c: apr_status_t h2_conn_child_init(apr_pool_t *pool, server_rec *s) { ... if (status != APR_SUCCESS) { /* some MPMs do not implemnent this */ async_mpm = 0; status = APR_SUCCESS; } ... } All errors have been mitigated to APR_SUCCESS. I don't see why ignoring them. Thanks.
Hi, The line above is: status = ap_mpm_query(AP_MPMQ_IS_ASYNC, &async_mpm); As said in the comment: "some MPMs do not implement this", so if an "error" is reported by a MPM, we consider that it can not process async connections. The call chain is: ap_mpm_query --> ap_run_mpm_query --> function defined by 'ap_hook_mpm_query' in each MPM Theses functions return in rv, either APR_SUCCESS or APR_ENOTIMPL. So it is not really an error. For example,: worker_query: rv = APR_ENOTIMPL event_query: rv = APR_SUCCESS So, this is not strictly speaking error handling, but fallback to a default behavior if a functionality is not explicitly supported.
(In reply to Christophe JAILLET from comment #1) > Hi, > > The line above is: > status = ap_mpm_query(AP_MPMQ_IS_ASYNC, &async_mpm); > > > As said in the comment: "some MPMs do not implement this", so if an "error" > is reported by a MPM, we consider that it can not process async connections. > > The call chain is: > ap_mpm_query > --> ap_run_mpm_query > --> function defined by 'ap_hook_mpm_query' in each MPM > > Theses functions return in rv, either APR_SUCCESS or APR_ENOTIMPL. > So it is not really an error. > > For example,: > worker_query: rv = APR_ENOTIMPL > event_query: rv = APR_SUCCESS > > > So, this is not strictly speaking error handling, but fallback to a default > behavior if a functionality is not explicitly supported. Understood. But I would suggest tp preserve APR_ENOTIMPL rather than to translate it to APR_SUCCESS to represent the default behavior. If more error codes have been implemented in the ap_run_mpm_query function, simply ignoring such errors can be potentially harmful.
(In reply to Christophe JAILLET from comment #1) > Hi, > > The line above is: > status = ap_mpm_query(AP_MPMQ_IS_ASYNC, &async_mpm); > > > As said in the comment: "some MPMs do not implement this", so if an "error" > is reported by a MPM, we consider that it can not process async connections. > > The call chain is: > ap_mpm_query > --> ap_run_mpm_query > --> function defined by 'ap_hook_mpm_query' in each MPM > > Theses functions return in rv, either APR_SUCCESS or APR_ENOTIMPL. > So it is not really an error. > > For example,: > worker_query: rv = APR_ENOTIMPL > event_query: rv = APR_SUCCESS > > > So, this is not strictly speaking error handling, but fallback to a default > behavior if a functionality is not explicitly supported. Though they return in rv, but some do return APR_EGENERAL. For example, the ap_mpm_query does not preserve APR_ENOTIMP. It does return APR_EGENERAL. So what you said cannot hold. AP_DECLARE(apr_status_t) ap_mpm_query(int query_code, int *result) { apr_status_t rv; if (ap_run_mpm_query(query_code, result, &rv) == DECLINED) { rv = APR_EGENERAL; } ... } I would suggest more add the detailed error handling, like if (status != APR_SUCCESS && status != APR_EGENERAL) { /* some MPMs do not implemnent this */ async_mpm = 0; status = APR_SUCCESS; } You do miss the cases for the declined query.
(In reply to legendt from comment #2) > But I would suggest tp preserve APR_ENOTIMPL rather than to translate it to > APR_SUCCESS to represent the default behavior. If more error codes have been > implemented in the ap_run_mpm_query function, simply ignoring such errors > can be potentially harmful. There is no reason to have other error code returned by an 'ap_hook_mpm_query' hook function. These functions are just there to tell if something is supported by the MPM or not. If it is so puzzling, the 'status = APR_SUCCESS;' could be removed. 'status' is not used anywhere below, before it is overridden by another (real) error code. (In reply to legendt from comment #3) > Though they return in rv, but some do return APR_EGENERAL. > > For example, the ap_mpm_query does not preserve APR_ENOTIMP. It does return > APR_EGENERAL. So what you said cannot hold. 'ap_hook_mpm_query' hook functions never return DECLINED. So APR_EGENERAL will never be returned by 'ap_mpm_query()'. And even if one did, there would be much more trouble somewhere else. > if (status != APR_SUCCESS && status != APR_EGENERAL) { Your proposal is just over-engineering something that looks safe, just in order to please a static analyzer. As said, IMHO, the only thing that could go in your direction (and would align with what is done in 'http_post_config()') would be to remove the 'status = APR_SUCCESS;'. It is useless.
Created attachment 36869 [details] Removed the useless status assignment 'status' is not used anywhere below, before it is overridden by another (real) error code.
(In reply to Christophe JAILLET from comment #4) > (In reply to legendt from comment #2) > > But I would suggest tp preserve APR_ENOTIMPL rather than to translate it to > > APR_SUCCESS to represent the default behavior. If more error codes have been > > implemented in the ap_run_mpm_query function, simply ignoring such errors > > can be potentially harmful. > > There is no reason to have other error code returned by an > 'ap_hook_mpm_query' hook function. > These functions are just there to tell if something is supported by the MPM > or not. > > If it is so puzzling, the 'status = APR_SUCCESS;' could be removed. > 'status' is not used anywhere below, before it is overridden by another > (real) error code. > > > (In reply to legendt from comment #3) > > Though they return in rv, but some do return APR_EGENERAL. > > > > For example, the ap_mpm_query does not preserve APR_ENOTIMP. It does return > > APR_EGENERAL. So what you said cannot hold. > > 'ap_hook_mpm_query' hook functions never return DECLINED. So APR_EGENERAL > will never be returned by 'ap_mpm_query()'. > And even if one did, there would be much more trouble somewhere else. > > > > if (status != APR_SUCCESS && status != APR_EGENERAL) { > Your proposal is just over-engineering something that looks safe, just in > order to please a static analyzer. > > > As said, IMHO, the only thing that could go in your direction (and would > align with what is done in 'http_post_config()') would be to remove the > 'status = APR_SUCCESS;'. It is useless. I understand what you have mentioned now. The line `status = ap_mpm_query` in this context can never return the APR_EGENERAL error, so the check is totally useless. Our analyzers do make the assumption about the original status, which can be APR_EGENERAL. And there could be a possible status conversion violation. If the status conversion is redundant, your proposal looks good to me. I have attached the corresponding patch.