Unchecked return value on third party code
Christian MAUDERER
christian.mauderer at embedded-brains.de
Tue Feb 23 16:29:14 UTC 2021
Am 23.02.21 um 17:14 schrieb Joel Sherrill:
>
>
> On Tue, Feb 23, 2021 at 9:50 AM Christian MAUDERER
> <christian.mauderer at embedded-brains.de
> <mailto: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.
True.
>
>
> >
> > + 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.
Hm. Would have been nice if it would have worked.
>
> 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.
>
That's what I meant with "false positive".
>
> 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.
Yes, I know. Upstream loops can be quite time consuming. I'm waiting
since about half a year that a FreeBSD patch for the i.MX6ULL SDHCI gets
reviewed (after someone here suggested it should go upstream). It's not
a chip that is used in FreeBSD so it's not a high priority.
> 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. :(
Only possibility: Find another bug at the same location and fix the
Coverity issue as a side effect ;-)
>
> We'll try to be good open source citizens, file issues upstream, and do
> what we
> think is right locally.
Agreed.
Again: I think such fixes are case by case decisions. We should fix as
much as necessary and possible. But we have to make sure that we don't
make our updates more complex with unnecessary fixes.
Best regards
Christian
>
> --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 <mailto:devel at rtems.org>
> > http://lists.rtems.org/mailman/listinfo/devel
> <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
> <mailto: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/
> <https://embedded-brains.de/datenschutzerklaerung/>
>
--
--------------------------------------------
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/
More information about the devel
mailing list