Hi,
I'd appreciate if anyone could provide feedback on this report from another tool. I've filtered out all the warnings that are provably false.
BTW, playing with openldap for a couple of days, I found that the code is of much lower quality than some other open source projects, like ISC bind & ntp, openssh,... Static checkers also find more bugs.
Some things I spotted: - code is very non-uniform, the same things are done in very different ways - in several places (10-20) pointers are checked with an assertion, but after the pointer has already been dereferenced. What's the purpose of assertions then? - "fat interfaces" - very long sequences of dereferences, like: *desc->ad_type->sat_equality->smr_normalize make GDB debugging harder, discourage consistency checking, and make static checking reports less readable. - inconsistency, a lot of it, for instance, a pointer is dereferenced, and ten lines below it's actually checked for NULL and then dereferenced
Anyways, here is the report, I hope it's going to be helpful:
Please replace ??? with Yes/No, and a short explanation. Bad programming practices are not considered bugs. A bug (by my definition) is a sequence of events that would crash the app, no matter how small the probability is, or how irrelevant the app is. Please, also consider the calling context (unlike Calysto, Saturn does not provide a complete trace, so it is hard to figure out whether the functions can be called under the right conditions to trigger the bug). If you can prove that the function can never be called so as to trigger the 'bug', then it's not a bug.
---------------------------------------------------------------- Bug: ???
@blue @2114 blue __arg0*.bv_val of function select_backend can evaluate to NULL select_backend (2114:servers/slapd/acl.c), final site of dereference is: (678:servers/slapd/backend.c) @servers/slapd/acl.c @Null pointer is passed to a function which dereferences it. ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ???
@violet @774 violet (INCONSISTENT USE) Possible null dereference __arg1*.e_nname.bv_val. This variable is checked for Null at lines: 704, and is dereferenced through call chain: servers/slapd/acl.c:regex_matches (774:servers/slapd/acl.c), acl_string_expand (2507:servers/slapd/acl.c), final site of dereference is: (2455:servers/slapd/acl.c) @servers/slapd/acl.c @Interprocedural inconsistency error
@violet @837 violet (INCONSISTENT USE) Possible null dereference __arg1*.e_nname.bv_val. This variable is checked for Null at lines: 704, and is dereferenced through call chain: acl_string_expand (837:servers/slapd/acl.c), final site of dereference is: (2455:servers/slapd/acl.c) @servers/slapd/acl.c @Interprocedural inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like a bug.
@violet @3868 violet (INCONSISTENT USE) Possible null dereference __arg0*.si_anlist. This variable is checked for Null at lines: 3859, and is dereferenced through call chain: anlist_unparse (3868:servers/slapd/syncrepl.c), final site of dereference is: (2841:servers/slapd/bconfig.c) @servers/slapd/syncrepl.c @Interprocedural inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like a bug.
@red @1753 red Possible NULL dereference of keys+keycount @servers/slapd/schema_init.c @Intraprocedural Null error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ???
@orange @2212 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_suffix+i. This variable is checked for Null at lines: 2192 @servers/slapd/bconfig.c @Inconsistency error
@orange @2211 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_suffix+i. This variable is checked for Null at lines: 2192 @servers/slapd/bconfig.c @Inconsistency error
@orange @2212 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_suffix+(i+1). This variable is checked for Null at lines: 2192 @servers/slapd/bconfig.c @Inconsistency error
@orange @2209 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_suffix+i. This variable is checked for Null at lines: 2192 @servers/slapd/bconfig.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ???
@orange @3147 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_update_refs+i. This variable is checked for Null at lines: 3135 @servers/slapd/bconfig.c @Inconsistency error
@orange @3149 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_update_refs+(i+1). This variable is checked for Null at lines: 3135 @servers/slapd/bconfig.c @Inconsistency error
@orange @3148 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_update_refs+i. This variable is checked for Null at lines: 3135 @servers/slapd/bconfig.c @Inconsistency error
@orange @3149 orange (INCONSISTENT USE) Possible null dereference of variable c->be->be_update_refs+i. This variable is checked for Null at lines: 3135 @servers/slapd/bconfig.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? There seems to be some correlation between default_passwd_hash and c->valx interprocedurally, but I couldn't figure it out.
@orange @1934 orange (INCONSISTENT USE) Possible null dereference of variable default_passwd_hash+i. This variable is checked for Null at lines: 1938 @servers/slapd/bconfig.c @Inconsistency error
@orange @1933 orange (INCONSISTENT USE) Possible null dereference of variable default_passwd_hash+i. This variable is checked for Null at lines: 1938 @servers/slapd/bconfig.c @Inconsistency error
@orange @1934 orange (INCONSISTENT USE) Possible null dereference of variable default_passwd_hash+(i+1). This variable is checked for Null at lines: 1938 @servers/slapd/bconfig.c @Inconsistency error
@orange @1932 orange (INCONSISTENT USE) Possible null dereference of variable default_passwd_hash+i. This variable is checked for Null at lines: 1938 @servers/slapd/bconfig.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? The same as above
@orange @2743 orange (INCONSISTENT USE) Possible null dereference of variable default_referral+i. This variable is checked for Null at lines: 2731 @servers/slapd/bconfig.c @Inconsistency error
@orange @2744 orange (INCONSISTENT USE) Possible null dereference of variable default_referral+i. This variable is checked for Null at lines: 2731 @servers/slapd/bconfig.c @Inconsistency error
@orange @2745 orange (INCONSISTENT USE) Possible null dereference of variable default_referral+i. This variable is checked for Null at lines: 2731 @servers/slapd/bconfig.c @Inconsistency error
@orange @2745 orange (INCONSISTENT USE) Possible null dereference of variable default_referral+(i+1). This variable is checked for Null at lines: 2731 @servers/slapd/bconfig.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ???
@orange @377 orange (INCONSISTENT USE) Possible null dereference of variable b1. This variable is checked for Null at lines: 348 @servers/slapd/backglue.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like a false positive, but not sure.
@violet @463 violet (INCONSISTENT USE) Possible null dereference servers/slapd/oc.c:oc_list.stqh_first*.soc_next.stqe_next. This variable is checked for Null at lines: 461, and is dereferenced through call chain: servers/slapd/oc.c:oc_delete_names (463:servers/slapd/oc.c), final site of dereference is: (394:servers/slapd/oc.c) @servers/slapd/oc.c @Interprocedural inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ???
@orange @2807 orange (INCONSISTENT USE) Possible null dereference of variable a->acl_attrs+0. This variable is checked for Null at lines: 2784 @servers/slapd/aclparse.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like attrs_alloc can return NULL @799.
@red @869 red Possible NULL dereference of a with trace a @servers/slapd/entry.c @Intraprocedural error with loop summary propagation
@red @837 red Possible NULL dereference of a with trace a @servers/slapd/entry.c @Intraprocedural error with loop summary propagation
@red @871 red Possible NULL dereference of a with trace a @servers/slapd/entry.c @Intraprocedural error with loop summary propagation
@red @856 red Possible NULL dereference of a with trace a @servers/slapd/entry.c @Intraprocedural error with loop summary propagation
@red @840 red Possible NULL dereference of a with trace a @servers/slapd/entry.c @Intraprocedural error with loop summary propagation
@red @838 red Possible NULL dereference of a with trace a @servers/slapd/entry.c @Intraprocedural error with loop summary propagation ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like out is initialized in the loop body, and the loop is executed at least once, but I'm not sure.
@orange @292 orange (INCONSISTENT USE) Possible null dereference of variable out+outpos. This variable is checked for Null at lines: 268 @libraries/liblunicode/ucstr.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Bug, pointer returned by realloc not checked.
@red @288 red Possible NULL dereference of values+(nvalues+j) with trace values @tests/progs/slapd-search.c @Intraprocedural error with loop summary propagation ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like anew->an_name.bv_val can remain uninitialized or NULL
@red @943 red Possible NULL dereference of anew->an_name.bv_val+0 with trace anew*.an_name.bv_val @servers/slapd/ad.c @Intraprocedural error with loop summary propagation
@red @921 red Possible NULL dereference of anew->an_name.bv_val+0 with trace anew*.an_name.bv_val @servers/slapd/ad.c @Intraprocedural error with loop summary propagation ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Inconsistency, can it be NULL?
@orange @128 orange (INCONSISTENT USE) Possible null dereference of variable info. This variable is checked for Null at lines: 136 @libraries/librewrite/rule.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ???
@blue @172 blue Trace entry of function tests/progs/slapd-modrdn.c:do_modrdn evaluates to NULL and is dereferenced through call chain: tests/progs/slapd-modrdn.c:do_modrdn (172:tests/progs/slapd-modrdn.c), final site of dereference is: (205:tests/progs/slapd-modrdn.c) @tests/progs/slapd-modrdn.c @Interprocedural Null error with loop summary propagation ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like bugs - SF_POP can return NULL.
@blue @385 blue __arg1 of function slap_set_join can evaluate to NULL slap_set_join (385:servers/slapd/sets.c), final site of dereference is: (230:servers/slapd/sets.c) @servers/slapd/sets.c @Null pointer is passed to a function which dereferences it.
@blue @411 blue __arg1 of function slap_set_join can evaluate to NULL slap_set_join (411:servers/slapd/sets.c), final site of dereference is: (230:servers/slapd/sets.c) @servers/slapd/sets.c @Null pointer is passed to a function which dereferences it. ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like bugs.
@orange @231 orange (INCONSISTENT USE) Possible null dereference of variable rset+j. This variable is checked for Null at lines: 134, 188 @servers/slapd/sets.c @Inconsistency error
@orange @244 orange (INCONSISTENT USE) Possible null dereference of variable rset+j. This variable is checked for Null at lines: 134, 188 @servers/slapd/sets.c @Inconsistency error
@orange @235 orange (INCONSISTENT USE) Possible null dereference of variable rset+j. This variable is checked for Null at lines: 134, 188 @servers/slapd/sets.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like bugs.
@orange @243 orange (INCONSISTENT USE) Possible null dereference of variable lset+i. This variable is checked for Null at lines: 120, 188 @servers/slapd/sets.c @Inconsistency error
@orange @235 orange (INCONSISTENT USE) Possible null dereference of variable lset+i. This variable is checked for Null at lines: 120, 188 @servers/slapd/sets.c @Inconsistency error
@orange @230 orange (INCONSISTENT USE) Possible null dereference of variable lset+i. This variable is checked for Null at lines: 120, 188 @servers/slapd/sets.c @Inconsistency error
@orange @244 orange (INCONSISTENT USE) Possible null dereference of variable lset+i. This variable is checked for Null at lines: 120, 188 @servers/slapd/sets.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? This pointer is checked @210, and if it's NULL a proper msg is printed, and then the pointer can be dereferenced. Would this be considered a bug, or a normal sequence of events?
@blue @227 blue __arg0 of function servers/slurpd/ldap_op.c:free_ldmarr can evaluate to NULL servers/slurpd/ldap_op.c:free_ldmarr (227:servers/slurpd/ldap_op.c), final site of dereference is: (574:servers/slurpd/ldap_op.c) @servers/slurpd/ldap_op.c @Null pointer is passed to a function which dereferences it. ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Looks like a bug.
@red @349 red Possible NULL dereference of ldmarr+nops @servers/slurpd/ldap_op.c @Intraprocedural Null error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ???
@red @646 red Possible NULL dereference of srs with trace srs @servers/slapd/overlays/syncprov.c @Intraprocedural error with loop summary propagation
@red @649 red Possible NULL dereference of srs with trace srs @servers/slapd/overlays/syncprov.c @Intraprocedural error with loop summary propagation
@red @647 red Possible NULL dereference of srs with trace srs @servers/slapd/overlays/syncprov.c @Intraprocedural error with loop summary propagation ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Can that pointer be NULL?
@orange @545 orange (INCONSISTENT USE) Possible null dereference of variable op->o_bd. This variable is checked for Null at lines: 551, 553, 597 @servers/slapd/backover.c @Inconsistency error
@orange @477 orange (INCONSISTENT USE) Possible null dereference of variable op->o_bd. This variable is checked for Null at lines: 483, 485, 529 @servers/slapd/backover.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Looks like a bug if the loop body @1104 is not executed.
@red @1112 red Possible NULL dereference of on with trace on @servers/slapd/backover.c @Intraprocedural error with loop summary propagation
@red @1113 red Possible NULL dereference of on with trace on @servers/slapd/backover.c @Intraprocedural error with loop summary propagation ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Bad programming practice, what's the point of checking the pointer after dereference?? Is this a bug?
@orange @528 orange (INCONSISTENT USE) Possible null dereference of variable val. This variable is checked for Null at lines: 534 @servers/slapd/value.c @Inconsistency error
@orange @465 orange (INCONSISTENT USE) Possible null dereference of variable val. This variable is checked for Null at lines: 471 @servers/slapd/value.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Bad programming practice, is it a bug?
@orange @298 orange (INCONSISTENT USE) Possible null dereference of variable data. This variable is checked for Null at lines: 302 @libraries/librewrite/ldapmap.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like a bug.
@orange @194 orange (INCONSISTENT USE) Possible null dereference of variable defaults. This variable is checked for Null at lines: 115, 118, 121, 125 @libraries/liblutil/sasl.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? @red @147 red Possible NULL dereference of p @libraries/liblutil/avl.c @Intraprocedural Null error
@red @148 red Possible NULL dereference of p @libraries/liblutil/avl.c @Intraprocedural Null error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? This is definitely a bad programming practice. Can rdn be NULL?
@orange @238 orange (INCONSISTENT USE) Possible null dereference of variable rdn+iAVA. This variable is checked for Null at lines: 240 @servers/slapd/dn.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? This is definitely a bad programming practice. Can dn be NULL?
@orange @1202 orange (INCONSISTENT USE) Possible null dereference of variable dn. This variable is checked for Null at lines: 1204 @servers/slapd/dn.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? This is definitely a bad programming practice. Can suffix be NULL?
@orange @1202 orange (INCONSISTENT USE) Possible null dereference of variable suffix. This variable is checked for Null at lines: 1205 @servers/slapd/dn.c @Inconsistency error ----------------------------------------------------------------
---------------------------------------------------------------- Bug: ??? Seems like a bug.
@red @634 red Possible NULL dereference of avl_list+ciltmp @libraries/liblutil/avl.c @Intraprocedural Null error ----------------------------------------------------------------
Thx,
Again, 2.4.4alpha is quite old. It's generally recommend anyone having issue with it try either use HEAD or RE23. Given the nature of your report, HEAD would be far more appropriate. This report contains items concerning code, for instance slurpd(8), that been axed from HEAD and won't appear in the upcoming 2.4 beta. It also doesn't cover codes that will be new in 2.4 beta.
On Aug 22, 2007, at 2:19 AM, Domagoj Babic wrote:
I'd appreciate if anyone could provide feedback on this report from another tool. I've filtered out all the warnings that are provably false.
I've selected a couple of items to comment on...
The first one I examined complains that a pointer returned ch_malloc () and dereferenced could be NULL. ch_malloc() obviously cannot possibly return NULL (a fact your tool apparently missed). Upon malloc failure, ch_malloc will either abort(3) (the usual behavior) or exit(3) as expected. As I noted in my initial response to your prior report, in OpenLDAP Software code, it is generally expected that a malloc failure will result in a either an abort(3) or a crash (by dereferencing NULL). This is especially so in slapd(8) (and not especially true in libldap/liblber).
The second one I examined complains about an inconsistency in a routine. A pointer argument to the routine was dereferenced before an assert(3) checking that the pointer was NULL. This is a minor inconsistency, the dereference should follow the assertion. However, this inconsistency doesn't cause the behavior of the routine to deviate from the expected (it still will crash as expected). The difference is that, if it were to crash on the assert, it's slightly more obvious to the developer that it crashed because the argument was NULL. Such inconsistencies crop up every now and then and are generally cleaned up when noticed. Thanks for noticing them.
While these asserts will fire on malloc failures, they are actually inserted to catch programming errors where a NULL (not returned by malloc) is passed into routine.
So, pointer assertions often serve one of two purposes, and some times both. As in ch_malloc(), it used to cause a sooner than later crash after a malloc() failure. As in libldap routines, it used to valid that the a pointer argument expected to be non-NULL is non-NULL instead of crashing on the deref (as is a common practice in standard C libraries). In many slapd(8) routines, some assertions may serve both purposes.
Anyways, neither of these two items are indicative of bugs in OpenLDAP Software. Here I will use the definition of software bug that I believe is widely accepted by this community: A software bug (or just "bug") is an error, flaw, mistake, failure, or fault in a computer program that prevents it from behaving as intended (e.g., producing an incorrect result). [Wikipedia] As the intended behavior is to crash, and slapd(8) will in both cases, neither is a bug (the second is an coding inconsistency). You're welcomed to use a different definition of bug for your purposes, that's your business.
You may find slapd(8) memory handling a bit odd. A lot of folks do. There are reasons for it, the details of which, if interested, you likely can find in the openldap-devel list archives. Regardless of the reasons for it, that's the way it is and it is very unlikely to change in this code base.
I note that this will likely be my first and only comment in this thread. I now need to attend to other things (like my day job).
-- Kurt
Kurt,
You seem to be a very precise guy, can you please indicate which reports you analyzed precisely. The "first one you examined" can be any one.
Thx,
-
Domagoj Babic -
Kurt Zeilenga