Unchecked return value on third party code

Tue Feb 23 16:14:24 UTC 2021

On Tue, Feb 23, 2021 at 9:50 AM Christian MAUDERER <
christian.mauderer at embedded-brains.de> wrote:

> Hello Joel,
>
> Am 23.02.21 um 16:23 schrieb Joel Sherrill:
> > Hi
> >
> > Ryan has wandered into the land of third party code and Coverity issues.
> > It is not a very welcoming land and we need to decide what we want to do
> > as a project. I put one
> >
> > In these cases, we have a few patterns to fall back on. There are
> > basically a few choices here:
> >
> > + ignore third party code in Coverity. I'm not a fan since we still have
> > code with issues.
>
> Depends a bit on the third party code and the types of Coverity errors.
> But in general you are right: It's not a good solution.
>

There isn't a good one.

>
> >
> >    + Do the minimum which is often just adding (void) in front of the
> > call not checking a return code..Still wrap in #ifdef __rtems__ as below
> > with a more robust solution.
> >
> > + Add #ifdef __rtems__ so the original code and rtems changes are
> > side by side. This is more like what we do with libbsd. But I would
> > still tend to use the correct fix and not just slap a (void) in front.
> > If we are going to modify the file, we might as well fix it.
> >
> > #ifdef __rtems__
> >     rc = XXX
> >     _Assert_Unused_variable_equals'
> > #else
> >     XXX
> > #endif
> >
> > Any other options? Opinions.
>
> Best option would be to fix it upstream (regardless whether it's a real
> bug or a false positive) and update our copy of the code.
>
> If that is not an option (patches not accepted; running out of time;
> some other reason) I think it needs a case by case analysis:
>
> Is it really a potential problem or is it a false positive of Coverity.
>
> 1) If it is a false positive we have two conflicting interests:
>
> a) Have as few changes as possible so that we can easily upgrade third
> party code to newer versions.
>
> b) Have as few Coverity warnings as possible.
>
> If possible I would rather disable specific warnings for specific third
> party files (or on a function level - is that possible?) rather then
> adding complexity to upgrades. Otherwise we might tend to rather not do
> an upgrade because it's too much work to re-integrate all the patches.
>

This isn't possible but we can flag a specific case as "don't care" and
move on.

I'm prone to think the one in the patch below is OK to do this with
since even if the calls fails, the next call to fdt_next_tag() will also
fail and get caught.

>
> 2) If it is not a false positive it should be definitively fixed (and
> not only masked with a (void)). In that case I think the #if(n)def
> __rtems__ like in libbsd is a good solution to make updates as simple as
> possible.
>

I suppose this is the solution and trying to at least file an issue
upstream.

I'm hesitant to hold up fixing it for us while waiting on a fix upstream
and then
an update on our side is twofold. First, the loop is quite time consuming
even
when it works. Worse, my history with upstreams and Coverity issues isn't
great.
It is possible in the end that we get stuck with the issue after all. I had
this happen
with gzip where the end result was an email that said they had decided it
was a
false positive. But every project including gzip gets the same issue
flagged. :(

We'll try to be good open source citizens, file issues upstream, and do
what we
think is right locally.

--joel

>
> Best regards
>
> Christian
>
> >
> > --joel
> >
> > ============================
> >   CID 1047324: Unchecked return value in fdt_add_subnode_namelen().
> >
> > Closes #4256
> > ---
> > cpukit/dtc/libfdt/fdt_rw.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/cpukit/dtc/libfdt/fdt_rw.c b/cpukit/dtc/libfdt/fdt_rw.c
> > index 1385425..ceeeb44 100644
> > --- a/cpukit/dtc/libfdt/fdt_rw.c
> > +++ b/cpukit/dtc/libfdt/fdt_rw.c
> > @@ -348,7 +348,7 @@ int fdt_add_subnode_namelen(void *fdt, int
> parentoffset,
> > return offset;
> > /* Try to place the new node after the parent's properties */
> > - fdt_next_tag(fdt, parentoffset, &nextoffset); /* skip the BEGIN_NODE */
> > + (void)fdt_next_tag(fdt, parentoffset, &nextoffset); /* skip the
> > BEGIN_NODE */
> > do {
> > offset = nextoffset;
> > tag = fdt_next_tag(fdt, offset, &nextoffset);
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
> >
>
> --
> --------------------------------------------
> embedded brains GmbH
> Herr Christian MAUDERER
> Dornierstr. 4
> 82178 Puchheim
> Germany
> email: christian.mauderer at embedded-brains.de
> phone: +49-89-18 94 741 - 18
> fax:   +49-89-18 94 741 - 08
>
> Registergericht: Amtsgericht München
> Registernummer: HRB 157899
> Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
> Unsere Datenschutzerklärung finden Sie hier:
> https://embedded-brains.de/datenschutzerklaerung/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210223/b867010e/attachment.html>