I scanned the SSSD source code with the clang static analyzer and found a number of issues. Patches are attached -- I think that most of them are OK to just include in master. Patches #1, #13 and #16 may be something that we want to include sooner.
Because the clang analyzer is freely available in Fedora, I think it would make sense to run a scan at least before a release.
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
On Thu, 2012-04-26 at 13:51 +0200, Jakub Hrozek wrote:
I scanned the SSSD source code with the clang static analyzer and found a number of issues. Patches are attached -- I think that most of them are OK to just include in master. Patches #1, #13 and #16 may be something that we want to include sooner.
Because the clang analyzer is freely available in Fedora, I think it would make sense to run a scan at least before a release.
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Nack There is no NULL-termination guarantee here. We only initialize pid_str with the first element being NULL.
Patch 0005: Ack
Patch 0006: Ack
Patch 0007: Ack
Patch 0008: Ack
Patch 0009: Ack
Patch 0010: Nack If we're reporting ret (which is an errno_t value) I'd prefer if the DEBUG messages included the strerror() conversion, since it's much easier to read.
Patch 0011: Ack
Patch 0012: Ack
Patch 0013: Ack
Patch 0014: Nack This is not the only case in ipa_get_autofs_options(), but returning here leaks tmp_ctx and anything still attached to it. Please fix all such cases here.
Patch 0015: Ack.
Patch 0016: Ack.
Patch 0017: Ack.
Patch 0018: Nack A tevent_req *_send() function should only return NULL if the tevent_req_create() fails. Otherwise, all other failures should be reported with tevent_req_error() and tevent_req_post().
I've pushed the acked patches to master (that's all but 0004, 0010, 0014 and 0018).
On Wed, May 02, 2012 at 08:12:06AM -0400, Stephen Gallagher wrote:
On Thu, 2012-04-26 at 13:51 +0200, Jakub Hrozek wrote:
I scanned the SSSD source code with the clang static analyzer and found a number of issues. Patches are attached -- I think that most of them are OK to just include in master. Patches #1, #13 and #16 may be something that we want to include sooner.
Because the clang analyzer is freely available in Fedora, I think it would make sense to run a scan at least before a release.
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
Patch 0004: Nack There is no NULL-termination guarantee here. We only initialize pid_str with the first element being NULL.
This is the pid_str initializer: char pid_str[MAX_PID_LENGTH] = {'\0'};
I think that this construct initializes the *whole* array with zeroes, because if there is an initializer but the number of initializers is less than the number of array elements, the rest of the array elements are set to 0.
Patch 0010: Nack If we're reporting ret (which is an errno_t value) I'd prefer if the DEBUG messages included the strerror() conversion, since it's much easier to read.
Fixed
Patch 0014: Nack This is not the only case in ipa_get_autofs_options(), but returning here leaks tmp_ctx and anything still attached to it. Please fix all such cases here.
Fixed
Patch 0018: Nack A tevent_req *_send() function should only return NULL if the tevent_req_create() fails. Otherwise, all other failures should be reported with tevent_req_error() and tevent_req_post().
Fixed, but frankly, there's tens of _send() functions in SSSD that just return NULL on any failure.
On 05/02/2012 05:05 PM, Jakub Hrozek wrote:
On Wed, May 02, 2012 at 08:12:06AM -0400, Stephen Gallagher wrote:
On Thu, 2012-04-26 at 13:51 +0200, Jakub Hrozek wrote:
I scanned the SSSD source code with the clang static analyzer and found a number of issues. Patches are attached -- I think that most of them are OK to just include in master. Patches #1, #13 and #16 may be something that we want to include sooner.
Because the clang analyzer is freely available in Fedora, I think it would make sense to run a scan at least before a release.
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
Patch 0004: Nack There is no NULL-termination guarantee here. We only initialize pid_str with the first element being NULL.
This is the pid_str initializer: char pid_str[MAX_PID_LENGTH] = {'\0'};
I think that this construct initializes the *whole* array with zeroes, because if there is an initializer but the number of initializers is less than the number of array elements, the rest of the array elements are set to 0.
I am not sure it does. I remember seeing it not being fully initialized.
Patch 0010: Nack If we're reporting ret (which is an errno_t value) I'd prefer if the DEBUG messages included the strerror() conversion, since it's much easier to read.
Fixed
Patch 0014: Nack This is not the only case in ipa_get_autofs_options(), but returning here leaks tmp_ctx and anything still attached to it. Please fix all such cases here.
Fixed
Patch 0018: Nack A tevent_req *_send() function should only return NULL if the tevent_req_create() fails. Otherwise, all other failures should be reported with tevent_req_error() and tevent_req_post().
Fixed, but frankly, there's tens of _send() functions in SSSD that just return NULL on any failure.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, 2012-05-02 at 23:05 +0200, Jakub Hrozek wrote:
On Wed, May 02, 2012 at 08:12:06AM -0400, Stephen Gallagher wrote:
On Thu, 2012-04-26 at 13:51 +0200, Jakub Hrozek wrote:
I scanned the SSSD source code with the clang static analyzer and found a number of issues. Patches are attached -- I think that most of them are OK to just include in master. Patches #1, #13 and #16 may be something that we want to include sooner.
Because the clang analyzer is freely available in Fedora, I think it would make sense to run a scan at least before a release.
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
Patch 0004: Nack There is no NULL-termination guarantee here. We only initialize pid_str with the first element being NULL.
This is the pid_str initializer: char pid_str[MAX_PID_LENGTH] = {'\0'};
I think that this construct initializes the *whole* array with zeroes, because if there is an initializer but the number of initializers is less than the number of array elements, the rest of the array elements are set to 0.
Nack
Please terminate after the lookup instead. The fread() doesn't guarantee termination either, so if it reads the full MAX_PID_LENGTH, we could overrun reading from it (it would still overwrite the last NULL, even if you're right about the initialization, which I don't think you are but I'm too tired to verify right now).
Patch 0010: Nack If we're reporting ret (which is an errno_t value) I'd prefer if the DEBUG messages included the strerror() conversion, since it's much easier to read.
Fixed
Ack
Patch 0014: Nack This is not the only case in ipa_get_autofs_options(), but returning here leaks tmp_ctx and anything still attached to it. Please fix all such cases here.
Fixed
Ack.
Patch 0018: Nack A tevent_req *_send() function should only return NULL if the tevent_req_create() fails. Otherwise, all other failures should be reported with tevent_req_error() and tevent_req_post().
Fixed, but frankly, there's tens of _send() functions in SSSD that just return NULL on any failure.
Yes, and I'm trying to eliminate each and every one as I can. They are relics of the dark times before we really understood the tevent_req style :)
Anyway, ack.
On Wed, May 02, 2012 at 08:05:05PM -0400, Stephen Gallagher wrote:
On Wed, 2012-05-02 at 23:05 +0200, Jakub Hrozek wrote:
On Wed, May 02, 2012 at 08:12:06AM -0400, Stephen Gallagher wrote:
On Thu, 2012-04-26 at 13:51 +0200, Jakub Hrozek wrote:
I scanned the SSSD source code with the clang static analyzer and found a number of issues. Patches are attached -- I think that most of them are OK to just include in master. Patches #1, #13 and #16 may be something that we want to include sooner.
Because the clang analyzer is freely available in Fedora, I think it would make sense to run a scan at least before a release.
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
Patch 0004: Nack There is no NULL-termination guarantee here. We only initialize pid_str with the first element being NULL.
This is the pid_str initializer: char pid_str[MAX_PID_LENGTH] = {'\0'};
I think that this construct initializes the *whole* array with zeroes, because if there is an initializer but the number of initializers is less than the number of array elements, the rest of the array elements are set to 0.
Nack
Please terminate after the lookup instead. The fread() doesn't guarantee termination either, so if it reads the full MAX_PID_LENGTH, we could overrun reading from it (it would still overwrite the last NULL, even if you're right about the initialization, which I don't think you are but I'm too tired to verify right now).
OK, that's better. Fixed.
I am right, though :-), see http://stackoverflow.com/questions/629017/how-does-array100-0-set-the-entire... which even links to the C standard document.
Patch 0010: Nack If we're reporting ret (which is an errno_t value) I'd prefer if the DEBUG messages included the strerror() conversion, since it's much easier to read.
Fixed
Ack
Patch 0014: Nack This is not the only case in ipa_get_autofs_options(), but returning here leaks tmp_ctx and anything still attached to it. Please fix all such cases here.
Fixed
Ack.
Patch 0018: Nack A tevent_req *_send() function should only return NULL if the tevent_req_create() fails. Otherwise, all other failures should be reported with tevent_req_error() and tevent_req_post().
Fixed, but frankly, there's tens of _send() functions in SSSD that just return NULL on any failure.
Yes, and I'm trying to eliminate each and every one as I can. They are relics of the dark times before we really understood the tevent_req style :)
Anyway, ack.
On Thu, 2012-05-03 at 10:22 +0200, Jakub Hrozek wrote:
Please terminate after the lookup instead. The fread() doesn't guarantee termination either, so if it reads the full MAX_PID_LENGTH, we could overrun reading from it (it would still overwrite the last NULL, even if you're right about the initialization, which I don't think you are but I'm too tired to verify right now).
OK, that's better. Fixed.
I am right, though :-), see http://stackoverflow.com/questions/629017/how-does-array100-0-set-the-entire... which even links to the C standard document.
Ack, and thanks for the pointer. It's always good to be reminded that I don't know everything :)
Pushed all four to master.
On Thu, Apr 26, 2012 at 01:51:39PM +0200, Jakub Hrozek wrote:
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
https://fedorahosted.org/sssd/wiki/DevelTips#Usingclangtoperformstaticanalys...
On Wed, May 09, 2012 at 11:39:02AM +0200, Jakub Hrozek wrote:
On Thu, Apr 26, 2012 at 01:51:39PM +0200, Jakub Hrozek wrote:
Developers can also run the clang tool themselves - I'll add info how into our Developers page.
https://fedorahosted.org/sssd/wiki/DevelTips#Usingclangtoperformstaticanalys...
On today's git HEAD, clang reports the following 20 bugs: * http://jhrozek.fedorapeople.org/sssd-clang-llvm/
Some of them are false positives, I'll address the rest: * API Argument with 'nonnull' attribute passed null - util/sss_krb5.c:1108 ** real bug * API Argument with 'nonnull' attribute passed null - sss_client/nss_mc_common.c:201 ** I'd like someone more familiar with the fast cache code to take a look. Is it possible for the assignment at sss_client/nss_mc_common.c:182 to yield rec_len == 0 ? * API Argument with 'nonnull' attribute passed null - responder/nss/nsssrv_mmap_cache.c:714 ** False positive, mc_ctx->fd would be still -1 so the code wouldn't run * Logic error Assigned value is garbage or undefined - providers/krb5/krb5_child_handler.c:164 ** False positive, the path clang describes can never be run, stops 3 and 9 cannot happen at the same time * Most of the "dead storage" warnings are false positives due to our use of in_transaction. I'll fix the rest, although it's not terribly urgent to do so. * Logic error Dereference of null pointer - sss_client/common.c:142 ** False positive, poll would have to fail and return -1 but set errno to 0 at the same time. * Logic error Dereference of null pointer - ldb_modules/memberof.c:328 ** Seems like a false positive to me, but I asked Jan to double check * Logic error Dereference of null pointer - confdb/confdb.c:1030 ** I'm not sure if this code is still useful, it seems like it was written to support changing configuration on the fly? * Logic error Dereference of null pointer providers/ldap/sdap_async.c:1125 ** False positive, state->nserverctrls would be 0 after step 5 and the loop in step 7 would never enter * Logic error Dereference of null pointer - resolv/async_resolv.c:2143 ** This seems like a real bug but the code path can't be reached with how the caller uses reply_weight_rearrange(). We should make it more robust, though.
Because clang is also able to output machine-readable XML results, I plan to make a "whitelist" of known false positives and run the analysis in cron nightly to catch new bugs automatically.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Stephen Gallagher