You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by William A Rowe Jr <wr...@rowe-clan.net> on 2019/01/16 18:02:01 UTC
Re: svn commit: r1850835 - /httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c
Doesn't this simply gloss over an underlying defect?
[...]
if (apr_dbm_fetch(f, key, &val) == APR_SUCCESS && val.dptr) {
*value = apr_pstrmemdup(pool, val.dptr, val.dsize);
}
apr_dbm_close(f);
return rv;
}
Shouldn't we capture and return the failure code from apr_dbm_fetch here?
On Wed, Jan 9, 2019 at 3:34 AM <jo...@apache.org> wrote:
> Author: jorton
> Date: Wed Jan 9 09:34:34 2019
> New Revision: 1850835
>
> URL: http://svn.apache.org/viewvc?rev=1850835&view=rev
> Log:
> * modules/aaa/mod_authn_dbm.c (fetch_dbm_value): No functional change:
> return APR_SUCCESS rather than rv, which is guaranteed to be
> APR_SUCCESS in current code.
>
> Modified:
> httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c
>
> Modified: httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c?rev=1850835&r1=1850834&r2=1850835&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c Wed Jan 9 09:34:34 2019
> @@ -101,7 +101,7 @@ static apr_status_t fetch_dbm_value(cons
>
> apr_dbm_close(f);
>
> - return rv;
> + return APR_SUCCESS;
> }
>
> static authn_status check_dbm_pw(request_rec *r, const char *user,
>
>
>
Re: svn commit: r1850835 - /httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Thu, Jan 17, 2019 at 4:03 AM Joe Orton <jo...@redhat.com> wrote:
> On Wed, Jan 16, 2019 at 12:02:01PM -0600, William A Rowe Jr wrote:
> > Doesn't this simply gloss over an underlying defect?
> >
> > [...]
> > if (apr_dbm_fetch(f, key, &val) == APR_SUCCESS && val.dptr) {
> > *value = apr_pstrmemdup(pool, val.dptr, val.dsize);
> > }
> >
> > apr_dbm_close(f);
> >
> > return rv;
> > }
> >
> > Shouldn't we capture and return the failure code from apr_dbm_fetch here?
>
> The error code from the failed lookup is not an interesting result, I
> assume because it can only fail one way. That the lookup failed is
> reflected by returning with success *value set to NULL.
>
> I had this change in my wc from looking at clang/coverity static
> analysis results so I assume it was flagged up somewhere but can't
> remember exactly. It seems like a harmless change.
>
Thanks for the reminder, yes... most providers will return only found
or not found; not found is not an error. However some providers may
fault, e.g. DB corrupt or storage interrupted or some other very rare
edge case. APR provided to return this info, whether the underlying
provider does is another question.
I'm concerned if clang/coverity flagged the existing code, since that
suggests some edge case that may not optimize correctly.
I withdraw my objection if we layer on the following comment to
avoid future confusion/research;
/* NOT FOUND is not an error case; this is indicated by NULL result.
* Treat all NULL lookup/error results as success for the simple case
* of auth credential lookup, these are DECLINED in both cases.
*/
Does that sound like a resolution Joe?
Re: svn commit: r1850835 -
/httpd/httpd/trunk/modules/aaa/mod_authn_dbm.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jan 16, 2019 at 12:02:01PM -0600, William A Rowe Jr wrote:
> Doesn't this simply gloss over an underlying defect?
>
> [...]
> if (apr_dbm_fetch(f, key, &val) == APR_SUCCESS && val.dptr) {
> *value = apr_pstrmemdup(pool, val.dptr, val.dsize);
> }
>
> apr_dbm_close(f);
>
> return rv;
> }
>
> Shouldn't we capture and return the failure code from apr_dbm_fetch here?
The error code from the failed lookup is not an interesting result, I
assume because it can only fail one way. That the lookup failed is
reflected by returning with success *value set to NULL.
I had this change in my wc from looking at clang/coverity static
analysis results so I assume it was flagged up somewhere but can't
remember exactly. It seems like a harmless change.