Re: [Libcdio-devel] static analysis

[Robert Kausch]

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