[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Libcdio-devel] static analysis

From: Rocky Bernstein
Subject: Re: [Libcdio-devel] static analysis
Date: Fri, 13 Jun 2014 21:01:00 -0400

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>

> 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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]