Hi,
I ran my static checker Calysto on openldap v2.4.4alpha. Here are the results.
Could you please check them out and replace ??? with Yes/No, and if it's a false positive, write a short explanation.
calysto v1.5 on openldap_v2.4.4alpha: ??/20
*** Analyzing clients_tools_ldapcompare.bc *** ------------------------------------------ Possible NULL-ptr deref (vc148): @/work/projects/llvm/tools/Calysto/IfaceSpecs/clib.c:1334 Bug: ??? Explanation: ber_strdup call (common.c:734) can return NULL, which is dereferenced in strlen call (@742) ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc162): @/work/projects/llvm/tools/Calysto/IfaceSpecs/clib.c:921 Bug: No Explanation: Pointer arithmetic + lazy summary expansion ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc192): @/work/projects/llvm/tools/Calysto/IfaceSpecs/clib.c:846 Bug: ??? Explanation: ber_strdup call (ldapcompare.c:136) can return NULL, which is dereferenced in strchr call on the next line. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc209): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/sasl.c:53 Bug: ??? Explanation: lutil_sasl_defaults call (@common.c:1060) can return NULL, which is passed to lutil_sasl_freedefs (@1071), which dereferences it (@sasl.c:53). ------------------------------------------
*** Analyzing clients_tools_ldapmodify.bc *** ------------------------------------------ Possible NULL-ptr deref (vc1789): @/work/projects/llvm/tools/Calysto/IfaceSpecs/clib.c:1909 Bug: ??? Explanation: ber_strdup call (@fetch.c:62) can return NULL, which is dereferenced by fopen (@65). ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc1791): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/ldif.c:868 Bug: ??? Explanation: ber_memalloc call (@ldif.c:867) can return NULL, which is dereferenced on the next line. ------------------------------------------
*** Analyzing clients_tools_ldapsearch.bc *** ------------------------------------------ Possible NULL-ptr deref (vc3851): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/base64.c:157 Bug: ??? Explanation: ldap_parse_reference call (@ldapsearch.c:1394) calls ldap_pvt_get_controls (@references.c:116), which can set ctrls[i]->ldctl_value.bv_val to NULL and return LDAP_SUCCESS. Later, in the call chain: tool_print_ctrls (@ldapsearc.c:1411) -> lutil_b64_ntop (@common.c:1636) ctrls[i]->ldctl_value.bv_val is received as parameter src, which is dereferenced @base64.c:157. If it helps, my trace says that the first test (@129) fails and that the value of ctrls[i]->ldctl_value.bv_len is 1. Don't know if that's relevant or not. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc3852): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/base64.c:178 Bug: ??? Explanation: ber_memalloc call (@common.c:1634) can return NULL, which is passed to lutil_b64_ntop (@1636), which dereferences it @base64.c:178. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc3853): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/base64.c:130 Bug: ??? Explanation: Seems like a duplicate of vc3851, but according to this trace, it seems that src can be uninitialized. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc3929): @/work/projects/llvm/tools/Calysto/IfaceSpecs/clib.c:922 Bug: ??? Explanation: Seems like ctrls[i]->ldctl_oid, dereferenced @common.c:1671) can be uninitialized. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc4342): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/ldif.c:718 Bug: No Explanation: pointer arithmetic + lazy summary expansion ------------------------------------------
*** Analyzing libraries_liblber_etest.bc *** ------------------------------------------ Possible NULL-ptr deref (vc7): @/work/projects/llvm/tools/Calysto/IfaceSpecs/clib.c:212 Bug: ??? Explanation: getbuf call (@etest.c:151) can return NULL, which atoi dereferences on the next line. ------------------------------------------
*** Analyzing libraries_libldap_ltest.bc *** ------------------------------------------ Possible NULL-ptr deref (vc44): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/libldap/test.c:778 Bug: ??? Explanation: Seems like vals[i]->bv_val can remain uninitialized. ------------------------------------------
*** Analyzing libraries_librewrite_rewrite.bc *** ------------------------------------------ Possible NULL-ptr deref (vc4622): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/avl.c:289 Bug: ??? Explanation: It seems that rewrite_info_init @rewrite.c:52 call can return a valid info, but leave info->li_params->avl_link[0] uninitialized. info is then passed to rewrite_read @54, which passes it to rewrite_parse @parse.c:112, which passes it to rewrite_param_set @config.c:283, which passes &info->li_params to rewrite_var_insert_f @params.c:51 (received as pointer tree). p is initialized with *tree @190. If nside==0, p->avl_link[0] is dereferenced @289. Note that Calysto finds _a trace_, not _the shortest_ trace. So, perhaps this would fail sooner.
Related problem: rewrite_info_init call @rewrite.c:52 can return NULL, which is then passed to rewrite_read @54, which calls rewrite_parse @parse.c:112. rewrite_parse has an assertion that will fail @config.c:59. The pointer returned by rewrite_info_init should be checked right away, and if NULL exit with an appropriate msg. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc4626): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/libraries/liblutil/avl.c:308 Bug: ??? Explanation: Seems like r->avl_link can be NULL. Trace is equally long to vc4622, but the control flow context is somewhat more complicated, so I'm leaving that out. ------------------------------------------
*** Analyzing servers_slurpd_slurpd.bc *** ------------------------------------------ Possible NULL-ptr deref (vc216): @/work/projects/llvm/tools/Calysto/IfaceSpecs/clib.c:869 Bug: ??? Explanation: CATLINE(buf) is called @config.c:427, but it can fail to initialize line. The trace shows that lcur+len+1 can result in an integer overflow, in which case the test @403 would fail, and line would not be initialized. Later, line is dereferenced @445. ------------------------------------------
*** Analyzing tests_progs_slapd-bind.bc *** ------------------------------------------ Possible NULL-ptr deref (vc27): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/tests/progs/slapd-common.c:176 Bug: ??? Explanation: ldap_str2charray call (@slapd-common.c:174) can return NULL, which is dereferenced two lines lower. ------------------------------------------
*** Analyzing tests_progs_slapd-tester.bc *** ------------------------------------------ Possible NULL-ptr deref (vc59): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/tests/progs/slapd-tester.c:277 Bug: ??? Explanation: ldap_str2charray call (@slapd-tester.c:275) can return NULL, which is dereferenced two lines lower. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc62): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/tests/progs/slapd-tester.c:224 Bug: ??? Explanation: ldap_str2charray call (@slapd-tester.c:218) can return NULL, which is dereferenced @224. ------------------------------------------
------------------------------------------ Possible NULL-ptr deref (vc64): @/work/benchmarks/SOURCES/openldap-2.4.4alpha/tests/progs/slapd-tester.c:226 Bug: ??? Explanation: calloc call (@slapd-tester.c:225) can return a NULL pointer, which is dereferenced on the next line. ------------------------------------------
Thx,
-- Domagoj Babic
On Aug 20, 2007, at 12:02 PM, Domagoj Babic wrote:
Could you please check them out and replace ??? with Yes/No, and if it's a false positive, write a short explanation.
It seems that most (if not all) of this is simply dereferencing the NULL result of a failed malloc (or the like) memory allocation. I don't consider such derefs of NULL to be bugs in OpenLDAP Software as it done intentionally. That is, the expected behavior of OpenLDAP Software, in general, is to failure abruptly upon malloc(3) failure, either by dereferencing NULL or abort(3)ing.
calysto v1.5 on openldap_v2.4.4alpha: ??/20
FYI, 2.4.4 is old code.
-- Kurt
Kurt,
On 8/20/07, Kurt Zeilenga kurt@openldap.org wrote:
On Aug 20, 2007, at 12:02 PM, Domagoj Babic wrote:
Could you please check them out and replace ??? with Yes/No, and if it's a false positive, write a short explanation.
It seems that most (if not all) of this is simply dereferencing the NULL result of a failed malloc (or the like) memory allocation. I don't consider such derefs of NULL to be bugs in OpenLDAP Software as it done intentionally. That is, the expected behavior of OpenLDAP Software, in general, is to failure abruptly upon malloc(3) failure, either by dereferencing NULL or abort(3)ing.
I've analyzed quite a few applications out there, and my recommendation would be to insert NULL-checks, and print an appropriate message. If users experience an unexplained crash, they will contribute it to bad code quality, not the current conditions on the machine. The message is clearly useful to the developers.
Probably the highest quality code I've seen so far is ISC BIND, they check every single pointer before dereference, and every single data structure for consistency.
So, I guess I can consider ??? marked VCs to be conditions that would crash openldap, right?
calysto v1.5 on openldap_v2.4.4alpha: ??/20
FYI, 2.4.4 is old code.
Are you interested in having the newer one checked?
Cheers,
Domagoj Babic wrote:
Kurt,
On 8/20/07, Kurt Zeilenga kurt@openldap.org wrote:
On Aug 20, 2007, at 12:02 PM, Domagoj Babic wrote:
Could you please check them out and replace ??? with Yes/No, and if it's a false positive, write a short explanation.
It seems that most (if not all) of this is simply dereferencing the NULL result of a failed malloc (or the like) memory allocation. I don't consider such derefs of NULL to be bugs in OpenLDAP Software as it done intentionally. That is, the expected behavior of OpenLDAP Software, in general, is to failure abruptly upon malloc(3) failure, either by dereferencing NULL or abort(3)ing.
I've analyzed quite a few applications out there, and my recommendation would be to insert NULL-checks, and print an appropriate message. If users experience an unexplained crash, they will contribute it to bad code quality, not the current conditions on the machine. The message is clearly useful to the developers.
Probably the highest quality code I've seen so far is ISC BIND, they check every single pointer before dereference, and every single data structure for consistency.
So, I guess I can consider ??? marked VCs to be conditions that would crash openldap, right?
calysto v1.5 on openldap_v2.4.4alpha: ??/20
FYI, 2.4.4 is old code.
Are you interested in having the newer one checked?
Feel free to check against CVS HEAD, which will shortly be synced up to become the 2.4.5 release.
But, expanding on Kurt's comments - most of the items you reported are in one-shot client or test code. The probability of an alloc routine returning NULL here is near zero, and since it is in code that is either (a) only used for one-shot tests or (b) only invoked for a single request and then exited, we really don't care. For any cases that you find that are in library code that can be executed multiple times in an app or server, we probably need to pay attention.
Howard,
On 8/20/07, Howard Chu hyc@symas.com wrote:
Feel free to check against CVS HEAD, which will shortly be synced up to become the 2.4.5 release.
Precise static checking is quite expensive computationally, and I keep quite a few machines busy 24/7. If you are interested in having openldap checked regularly, please see: http://www.cs.ubc.ca/~babic/index_calysto_community.htm
I'll need more precise feedback than you provided me right now. For instance, there is one report about which I'm not 100% certain, and no one has even looked at reports carefully enough to figure that out.
Also, keep in mind that Calysto is constantly being developed, so although I'm checking only NULL-ptrs now, by the end of the year Calysto will enter the second phase - checking of user provided assertions. Later, I'll introduce checking of implicitly implied properties of C lib (like proper nesting of lock-unlock calls, and so on...)
But, expanding on Kurt's comments - most of the items you reported are in one-shot client or test code. The probability of an alloc routine returning NULL here is near zero, and since it is in code that is either (a) only used for one-shot tests or (b) only invoked for a single request and then exited, we really don't care. For any cases that you find that are in library code that can be executed multiple times in an app or server, we probably need to pay attention.
Even though the probability is near zero, it still will happen, considering the large user base you have.
Cheers,
Domagoj Babic wrote:
But, expanding on Kurt's comments - most of the items you reported are in one-shot client or test code. The probability of an alloc routine returning NULL here is near zero, and since it is in code that is either (a) only used for one-shot tests or (b) only invoked for a single request and then exited, we really don't care. For any cases that you find that are in library code that can be executed multiple times in an app or server, we probably need to pay attention.
Even though the probability is near zero, it still will happen, considering the large user base you have.
If a malloc fails in a one-shot command, that means the runtime environment is broken, not any of our code. Whether it is very probable or totally unlikely is irrelevant in that case.
On 8/20/07, Howard Chu hyc@symas.com wrote:
If a malloc fails in a one-shot command, that means the runtime environment is broken, not any of our code. Whether it is very probable or totally unlikely is irrelevant in that case.
That's one opinion. The fact that most apps I've checked so far actually guard against "broken environment" speaks for itself.
Cheers,
Domagoj Babic wrote:
On 8/20/07, Howard Chu hyc@symas.com wrote:
If a malloc fails in a one-shot command, that means the runtime environment is broken, not any of our code. Whether it is very probable or totally unlikely is irrelevant in that case.
That's one opinion. The fact that most apps I've checked so far actually guard against "broken environment" speaks for itself.
viola:~/Desktop> limit vmemoryuse 128 viola:~/Desktop> ls Killed viola:~/Desktop> limit vmemoryuse 1024 viola:~/Desktop> ls ls: error while loading shared libraries: libc.so.6: failed to map segment from shared object: Cannot allocate memory viola:~/Desktop> limit vmemoryuse 2048 viola:~/Desktop> ls Segmentation fault <<<
1) Sometimes you will run out of memory and just die, regardless of what kind of precautionary code you write. 2) In library code that can be invoked many times, yes, it's important to provide diagnostics. 3) In code that only executes once and then exits, it's pointless.
This is just common sense, life is short, put the effort where it matters.
Bad example. Try doing that with bind or postfix. ls really doesn't matter, we can agree on that.
I'm actually more interested in your opinion on the long term collaboration (bug reports in exchange for feedback and a bit of marketing :-) ).
viola:~/Desktop> limit vmemoryuse 128 viola:~/Desktop> ls Killed viola:~/Desktop> limit vmemoryuse 1024 viola:~/Desktop> ls ls: error while loading shared libraries: libc.so.6: failed to map segment from shared object: Cannot allocate memory viola:~/Desktop> limit vmemoryuse 2048 viola:~/Desktop> ls Segmentation fault
Cheers,
On Aug 20, 2007, at 12:55 PM, Domagoj Babic wrote:
calysto v1.5 on openldap_v2.4.4alpha: ??/20
FYI, 2.4.4 is old code.
Are you interested in having the newer one checked?
I'm generally interested in new reports of actual or potential bugs in OpenLDAP Software. I'm generally uninterested in redundant reports of actual or potential bugs, or redundant reports of non-bugs.
Others, of course, might find more of your report interesting than I do. And I might even find one or two of the cases more interesting if they were get pulled from the weeds of non-bugs. I simply don't have time/energy to pull them from the weeds myself.
For instance, there is one report about which I'm not 100% certain, and no one has even looked at reports carefully enough to figure that out.
Aside from the fact your message was only distributed to the list hours ago, and you already have two responses to your message, it seems like it getting pretty good attention to me. I cannot imagine any message discussing such issues to have gained much more attention in a shorter period of time.
That said, as I noted above, I might be find one or two cases more interesting if they were pulled from the weeds. If you have some urgent need to have one or two examined soon, I suggest you do the pulling.
-- Kurt
Kurt Zeilenga wrote:
That said, as I noted above, I might be find one or two cases more interesting if they were pulled from the weeds. If you have some urgent need to have one or two examined soon, I suggest you do the pulling.
Also, since discovering potential bugs in an automated manner does not allow to directly figure out their impact, posting them to a public list could either
1) cause security issues in case of real, yet undiscovered vulnerabilities. In this case, publicity should occur only __after__ the issue has been fixed and the fix released.
2) generate confusion in case of false positives.
For this purpose, the ITS allows to mark submissions as PRIVATE.
p.
Ing. Pierangelo Masarati OpenLDAP Core Team
SysNet s.r.l. via Dossi, 8 - 27100 Pavia - ITALIA http://www.sys-net.it --------------------------------------- Office: +39 02 23998309 Mobile: +39 333 4963172 Email: pierangelo.masarati@sys-net.it ---------------------------------------
-
Domagoj Babic -
Howard Chu -
Kurt Zeilenga -
Pierangelo Masarati