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.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
