[SSSD] [PATCH] Assorted fixes found by the clang static analyzer

Stephen Gallagher sgallagh at redhat.com
Thu May 3 00:05:05 UTC 2012


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120502/dc883d65/attachment.sig>


More information about the sssd-devel mailing list