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.