On Mon, May 9, 2016 08:51:56 Daiki Ueno wrote:
> Follow-up Comment #3, bug #47847 (project gettext):
>
> Thanks for the comment, Bruno.
>
> The reasoning sounds convincing, but I'm a bit confused that there is no
> such path in the original code. ISO C 6.2.4 also says: "The result of
> attempting to indirectly access an object with automatic storage duration
> from a thread other than the one with which the object is associated is
> implementation-defined", but I neither see a possibility of this.
>
> So far, the more I think of this, the more it seems like a false positive
> (and if so, perhaps we could add an annotation instead to suppress the
> warning).
It is absolutely not a false positive. It is warning because the pointer is
referred to after it has been free()'d. This is a class of behavior which is
explicitly described as undefined behavior in the C standard (Annex J.2 lists
the conditions if you have the reference handy, as does the CERT link I
provided in the bug link).
In simplified form the code currently does:
char *msg_ctxt_id = malloc(...)
char *translation = dgettext(...)
free (msg_ctxt_id); /* msg_ctxt_id's value now indeterminate */
if (translation != msg_ctxt_id) {
/* Undefined behavior; msg_ctxt_id's indeterminate value
was used in comparison */
return translation;
}
To be clear this only happens when variable sized arrays are not available,
but given that this feature is supported in gettext, it should work whether
VLAs are used or not. But please don't be confused into thinking this is a
false positive. I've seen those in Coverity but this one is not only not a
false positive, it's undefined behavior which is clearly visible to the
compiler without any fancy theorem-proving needed. It's just a fancy future
optimization pass away from breaking real code that has by pure happenstance
worked right in the past.
Regards,
- Michael Pyne
