Re: [Libcdio-devel] static analysis

[Robert Kausch]

There are no external incompatibilities introduced by the changes in the 
static-analysis branch.

udf_ff_traverse is a helper function only used internally in udf_fs.c. 
You only need to be aware of the new semantics when working on libudf 
itself. cdio_dirname and cdio_abspath are exported, but seemingly only 
to be able to write a test for them. They are not part of the external 
API and are declared in cdio_private.h. And even if someone used them in 
his own code, they will keep working as before. Sorry if that wasn't 
clear from my previous mail.

By the way, if any API/ABI incompatibilites are introduced before the 
next release, we might also call it 0.100. It's a little ugly, but 
several other projects did that before.

Am 14.06.2014 03:01, schrieb Rocky Bernstein:
> Thanks, Robert for moving this forward. I've merged the uncontroversial
> patches from the static-analysis branch and merged into master. This would
> be the changes to iso9660.c and the getopt changes.
>
> Of course, I looked at the other changes and I'm okay with them. Even if it
> means an incompatible change. But of course I invite others to look at and
> comment on.
>
> And it means we need to note the next release is incompatible. Perhaps now
> the next release will be a 1.0 release.
>
>
> On Fri, Jun 13, 2014 at 8:05 AM, Robert Kausch <address@hidden>
> wrote:
>
> > Thank you for reporting this!
> >
> > I have set up a static-analysis branch on git and pushed fixes for these
> > problems.
> >
> > The leaks in iso9660_fs.c were straight forward to fix. The iso-info.c
> > report was a false positive, as getopt_long makes sure that optarg will not
> > be NULL for option "r".
> >
> > udf_fs.c needed some addional care. The actual cause of both reported
> > problems was in extremely intransparent and complex semantics of
> > udf_ff_traverse. The function actually had three different possible
> > outcomes:
> >
> >    - if no entry was not found in the current dir, p_udf_dirent was be
> > freed and NULL returned
> >    - if an entry was found in the current dir, p_udf_dirent was updated and
> > returned
> >    - if a recursion was needed, p_udf_dirent was discarded and a new dirent
> > struct returned
> >
> > The call to udf_ff_traverse in udf_fopen handled only the second and third
> > outcome correctly. In case of NULL being returned, it would try to free the
> > already freed dirent struct a second time.
> >
> > The recursion call in udf_ff_traverse, in contrast, handled only the first
> > and second outcome correctly. If more than one recursion step was
> > performed, intermediately created dirent structs were leaked.
> >
> > My commit changes the semantics of udf_ff_traverse so that it always takes
> > care of p_udf_dirent. Callers can and must ignore the passed struct
> > afterwards from now on and process only the value returned by
> > udf_ff_traverse. This greatly simplifies usage of that function.
> > Fortunately it was not exposed in the API and only used internally in
> > udf_fs.c.
> >
> > I also set up projects for libcdio and libcdio-paranoia on Coverity Scan.
> > I want to process the results and push fixes, before merging the
> > static-analysis branch. I'll send invitations for both projects to Rocky
> > soon. If anyone else would like to have a look at the results, contact me
> > to get an invitation too.
> >
> > Robert
> >
> > Am 17.04.2014 17:55, schrieb Frantisek Kluknavsky:
> >
> >   Hi,
> > > Maybe you want to see results of static analysis. First two defects seem
> > > noteworthy.
> > >
> > > libcdio-0.92/lib/udf/udf_fs.c:266:double_free – Calling
> > > "udf_dirent_free(udf_dirent_t *)" frees pointer "p_udf_dirent" which has
> > > already been freed.
> > >
> > > libcdio-0.92/src/iso-info.c:159:var_deref_model – Passing null pointer
> > > "optarg" to function "atoll(char const *)", which dereferences it.
> > >
> > > libcdio-0.92/lib/iso9660/iso9660_fs.c:1568:leaked_storage – Variable
> > > "p_stat" going out of scope leaks the storage it points to.
> > >
> > > libcdio-0.92/lib/iso9660/iso9660_fs.c:757:leaked_storage – Variable
> > > "p_stat" going out of scope leaks the storage it points to.
> > >
> > > libcdio-0.92/lib/udf/udf_fs.c:234:dead_error_line – Execution cannot
> > > reach this statement "free(p_udf_dirent->psz_name);".
> > >
> > > Have a nice day.
> > >
> > > Fero