[Top][All Lists]

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

Re: [Libcdio-devel] static analysis

From: Robert Kausch
Subject: Re: [Libcdio-devel] static analysis
Date: Fri, 13 Jun 2014 14:05:07 +0200
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Thunderbird/30.0

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.


Am 17.04.2014 17:55, schrieb Frantisek Kluknavsky:

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.


reply via email to

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