[Bug 63900] New: Why mitigate the apr errors?

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 63900] New: Why mitigate the apr errors?

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

            Bug ID: 63900
           Summary: Why mitigate the apr errors?
           Product: Apache httpd-2
           Version: 2.5-HEAD
          Hardware: PC
                OS: All
            Status: NEW
          Severity: major
          Priority: P2
         Component: mod_http2
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Why mitigate the apr errors?

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

Christophe JAILLET <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID

--- Comment #1 from Christophe JAILLET <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Why mitigate the apr errors?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |REMIND

--- Comment #2 from [hidden email] ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Why mitigate the apr errors?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|REMIND                      |---
             Status|RESOLVED                    |REOPENED

--- Comment #3 from [hidden email] ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Why mitigate the apr errors?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

Christophe JAILLET <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #4 from Christophe JAILLET <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Why mitigate the apr errors?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #5 from [hidden email] ---
Created attachment 36869
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36869&action=edit
Removed the useless status assignment

'status' is not used anywhere below, before it is overridden by another (real)
error code.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Why mitigate the apr errors?

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

--- Comment #6 from [hidden email] ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Redundant apr error checking

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Why mitigate the apr        |Redundant apr error
                   |errors?                     |checking

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63900] Redundant apr error checking

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63900

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |INFORMATIONPROVIDED

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]