[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 14:31:36 +0200
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Thunderbird/30.0

Well, yes, if you consider cdio_dirname and cdio_abspath part of the public API, that's true. Removing the const qualifier is backward compatible, but breaks forward compatibility of older library versions.

The functions are, however, declared only in cdio_private.h and should not be used in third party programs. So it's at least controversial if this should be considered an API/ABI change. If that is the case, I agree that the version number will have to be updated.

Anyway, there are about 40 issues left to fix in the library sources and I don't know yet if any of them might require API changes. So, possibly, this whole discussion might become obsolete if we have to change the API to fix some of the open issues.

Invitations to the Coverity project have been sent to you and Pete btw., so you can have a look at the issue list too.

Am 14.06.2014 13:52, schrieb Rocky Bernstein:
There are no external incompatibilities introduced by the changes in the
static-analysis branch.

For example, signatures were changed to remove const from cdio_dirname and
cdio_abspath. Programmers now may have to issue a free() where it was wrong
to do so previously.

I'm not saying that the changes shouldn't be applied, but just that there
are some external changes that may cause programs using libcdio to change.

Unless others have objections, I think the static branch code could be
merged in. After that's merged , the static branch is still there for
further changes to potentially future merges. For things that aren't
controversial, I'd prefer sooner merges so that it gets more exposure.

As for the version number, this project has been around over a decade, so
it's it is not like we've been rushing to 1.0 release. My understanding is
that 1.0 does mean that there are incompatible changes and it also happens
to be the simplest thing here.


On Sat, Jun 14, 2014 at 7:17 AM, Robert Kausch <address@hidden>

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
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
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
    - if a recursion was needed, p_udf_dirent was discarded and a new
struct returned

The call to udf_ff_traverse in udf_fopen handled only the second and
outcome correctly. In case of NULL being returned, it would try to free
already freed dirent struct a second time.

The recursion call in udf_ff_traverse, in contrast, handled only the
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
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]