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 <[email protected]> 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 >> >> > >
