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.
Unless
On Sat, Jun 14, 2014 at 7:17 AM, Robert Kausch <address@hidden>
wrote:
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>
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