Re: [PATCH] cryptui: use add_oid_to_usage correctly (Coverity)

[Juan Lang]

Hi Marcus,

-            add_oid_to_usage(usage, ptr);
+            usage = add_oid_to_usage(usage, ptr);

This looks fine, but would you mind making the same change on line 337?

Thanks,

--Juan

[PATCH] cryptui: use add_oid_to_usage correctly (Coverity)

[Juan Lang]

On Fri, Feb 1, 2013 at 3:45 PM, Juan Lang <juan.lang(a)gmail.com> wrote:
> Hi Marcus,
>
> -            add_oid_to_usage(usage, ptr);
> +            usage = add_oid_to_usage(usage, ptr);
>
> This looks fine, but would you mind making the same change on line 337?
>
> Actually, perhaps I hit sent too early. If this memory allocation fails,
> which is the only situation under which add_oid_to_usage doesn't just
> return its first parameter, it'll immediately crash on the next invocation
> with a NULL pointer dereference.

I'm not sure it's worth all the trouble in an out of memory situation.
Perhaps just remove the return value and let the caller crash.

Thanks again,
--Juan

[PATCH] cryptui: use add_oid_to_usage correctly (Coverity)

[Marcus Meissner]

On Fri, Feb 01, 2013 at 03:48:27PM -0800, Juan Lang wrote:
> On Fri, Feb 1, 2013 at 3:45 PM, Juan Lang <juan.lang(a)gmail.com> wrote:
> > Hi Marcus,
> >
> > -            add_oid_to_usage(usage, ptr);
> > +            usage = add_oid_to_usage(usage, ptr);
> >
> > This looks fine, but would you mind making the same change on line 337?
> >
> > Actually, perhaps I hit sent too early. If this memory allocation fails,
> > which is the only situation under which add_oid_to_usage doesn't just
> > return its first parameter, it'll immediately crash on the next invocation
> > with a NULL pointer dereference.
>
> I'm not sure it's worth all the trouble in an out of memory situation.
> Perhaps just remove the return value and let the caller crash.

Actually the loop around checks that as a condition and would lead to return NULL:

        for (ptr = usageStr, comma = strchr(ptr, ','); usage && ptr && *ptr;

For the second one the loop around does not catch it.

I think the add_oid_to_usage() should not even do it this way  and not touch "usage"
at all, but instead return a memory allocation error and let the caller handle it:/

Or just let it crash.

Ciao, Marcus

[PATCH] cryptui: use add_oid_to_usage correctly (Coverity)

[Juan Lang]

On Sat, Feb 2, 2013 at 5:05 AM, Marcus Meissner <marcus(a)jet.franken.de>wrote:
> On Fri, Feb 01, 2013 at 03:48:27PM -0800, Juan Lang wrote:
> > On Fri, Feb 1, 2013 at 3:45 PM, Juan Lang <juan.lang(a)gmail.com> wrote:
> > > Hi Marcus,
> > >
> > > -            add_oid_to_usage(usage, ptr);
> > > +            usage = add_oid_to_usage(usage, ptr);
> > >
> > > This looks fine, but would you mind making the same change on line 337?
> > >
> > > Actually, perhaps I hit sent too early. If this memory allocation
> > > fails,
> > > which is the only situation under which add_oid_to_usage doesn't just
> > > return its first parameter, it'll immediately crash on the next
> > > invocation
> > > with a NULL pointer dereference.
> >
> > I'm not sure it's worth all the trouble in an out of memory situation.
> > Perhaps just remove the return value and let the caller crash.
>
> Actually the loop around checks that as a condition and would lead to
> return NULL:
>
> for (ptr = usageStr, comma = strchr(ptr, ','); usage && ptr &&
> *ptr;
>
> For the second one the loop around does not catch it.
>
> I think the add_oid_to_usage() should not even do it this way  and not
> touch "usage"
> at all, but instead return a memory allocation error and let the caller
> handle it:/
>
> Or just let it crash.

Yeah, I think just letting it crash is better than trying to sort it all
out. Thanks, and sorry for the confusing and buggy code :/
--Juan