Re: [Libcdio-devel] static analysis

[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
>>
>>
>
>