[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: Sat, 14 Jun 2014 13:17:19 +0200
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Thunderbird/30.0

There are no external incompatibilities introduced by the changes in the static-analysis branch.

udf_ff_traverse is a helper function only used internally in udf_fs.c. You only need to be aware of the new semantics when working on libudf itself. cdio_dirname and cdio_abspath are exported, but seemingly only to be able to write a test for them. They are not part of the external API and are declared in cdio_private.h. And even if someone used them in his own code, they will keep working as before. Sorry if that wasn't clear from my previous mail.

By the way, if any API/ABI incompatibilites are introduced before the next release, we might also call it 0.100. It's a little ugly, but several other projects did that before.

Am 14.06.2014 03:01, schrieb 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>

Thank you for reporting this!

I have set up a static-analysis branch on git and pushed fixes for these

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

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

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

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]