Re: [Libcdio-devel] static analysis

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

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