Coverity: dereference after null checks #14589
Conversation
paulidale
commented
Mar 17, 2021
paulidale
commented
- documentation is added or updated
- tests are added or updated
paulidale
added
branch: master
| ret = i2d_X509_NAME((X509_NAME *)b, NULL); | ||
| if (ret < 0) | ||
| return -2; | ||
| } | ||
|
|
||
| ret = a->canon_enclen - b->canon_enclen; | ||
| if (ret == 0 && a->canon_enclen != 0) | ||
| if (ret == 0 && a->canon_enclen != 0 | ||
| && a->canon_enc != NULL && b->canon_enc != NULL) | ||
| ret = memcmp(a->canon_enc, b->canon_enc, a->canon_enclen); |
There was a problem hiding this comment.
So this change means we skip the memcmp and return 0 if canon_enclen >0 but canon_enc == NULL. Surely though that scenario is an error (and so a -2 return is more appropriate)?
There was a problem hiding this comment.
That's more sensible. I've made the change.
There was a problem hiding this comment.
@mattcaswell and @pauli, this 'more sensible' solution turns out to cause trouble.
I found it is the reason for #14813.
The point is that NULL-DNs have zero canon_enclen and canon_enc is NULL,
and the latter must not be taken as an error in case canon_enclen is 0.
So the original plain to fix the Coverity issue was actually better.
It suppose the Coverity issue is a false positive
because canon_enclen != 0 should imply canon_enc != NULL.
There was a problem hiding this comment.
IMO the actual issue/bug is that for a NULL-DN the canon_enc should be an (static) empty string, which should also avoid artificial case distinctions. I'll fix it this way.
There was a problem hiding this comment.
You're more expert than I, PR welcome :)
If not, I'll look into it next week (assuming I remember, which is doubtful :)
There was a problem hiding this comment.
Well, my original plan was to leave this to an ASN.1 expert,
but this can be fixed without touching the actual ASN.1 stuff :)
paulidale
force-pushed
the
coverity-nulls
branch
from
3a3ce61
to
7831537
Compare
mattcaswell
added
approval: done
|
24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually. |
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #14589)
|
Merged to master, thanks for the feedback. |
