longjmp clobber warnings are utterly broken in modern gcc

Поиск
Список
Период
Сортировка
От Tom Lane
Тема longjmp clobber warnings are utterly broken in modern gcc
Дата
Msg-id 13955.1422212567@sss.pgh.pa.us
обсуждение исходный текст
Ответы Re: longjmp clobber warnings are utterly broken in modern gcc  (Greg Stark <stark@mit.edu>)
Re: longjmp clobber warnings are utterly broken in modern gcc  (Martijn van Oosterhout <kleptog@svana.org>)
Re: longjmp clobber warnings are utterly broken in modern gcc  (Andres Freund <andres@2ndquadrant.com>)
Re: longjmp clobber warnings are utterly broken in modern gcc  (Robert Haas <robertmhaas@gmail.com>)
Список pgsql-hackers
I've been looking for other instances of the problem Mark Wilding
pointed out, about missing "volatile" markers on variables that
are modified in PG_TRY blocks and then used in the PG_CATCH stanzas.
There definitely are some.  Current gcc versions do not warn about that.

If you turn on -Wclobbered, you don't always get warnings about the
variables that are problematic, and you do get warnings about variables
that are perfectly safe.  (This is evidently why that option is not on
by default: it's *useless*, or even counterproductive if it gives you
false confidence that the compiler is protecting you.)

I thought maybe the gcc folk no longer care about this because the
compiler is now smart enough to compile safe code in these situations.
A simple experiment disabused me of that notion.  I took guc.c's
AlterSystemSetConfigFile, which is at risk because of this sequence:
   int            Tmpfd = -1;
   ...   Tmpfd = open(AutoConfTmpFileName, O_CREAT | O_RDWR | O_TRUNC, S_IRUSR | S_IWUSR);   if (Tmpfd < 0)
ereport(ERROR,              (errcode_for_file_access(),                errmsg("could not open file \"%s\": %m",
             AutoConfTmpFileName)));
 
   PG_TRY();   {       ...       close(Tmpfd);       Tmpfd = -1;       ...   }   PG_CATCH();   {       if (Tmpfd >= 0)
        close(Tmpfd);       ...   }   PG_END_TRY();
 

and compared the assembly language generated with and without adding
"volatile" to Tmpfd's declaration.  Without "volatile" (ie, in the
code as shipped), gcc optimizes away the assignment "Tmpfd = -1"
within PG_TRY, and it also optimizes away the if-test in PG_CATCH,
apparently believing that control cannot transfer from inside the
PG_TRY to the PG_CATCH.  This is utterly wrong of course.  The issue
is masked because we don't bother to test for a failure return from the
second close() call, but it's not hard to think of similar coding
patterns where this type of mistaken optimization would be disastrous.
(Even here, the bogus close call could cause a problem if we'd happened
to open another file during the last part of the PG_TRY stanza.)

Not only does -Wclobbered fail to point out this risk, but to add
insult to injury it does whinge about two *other* variables in the
same function that are actually perfectly safe.  I'm not sure what
algorithm it's using to decide what to warn about, but the algorithm
has approximately nothing to do with reality.

I tested this on gcc 4.4.7 (current on RHEL6), and gcc 4.8.3 (current
on Fedora 20); they behave the same.  Even if this were fixed in
bleeding-edge gcc, we'd definitely still need the "volatile" marker
to get non-broken code compiled on most current platforms.

This is scary as hell.  I intend to go around and manually audit
every single PG_TRY in the current source code, but that is obviously
not a long-term solution.  Anybody have an idea about how we might
get trustworthy mechanical detection of this type of situation?
        regards, tom lane



В списке pgsql-hackers по дате отправления:

Предыдущее
От: Andrew Gierth
Дата:
Сообщение: Re: New CF app deployment
Следующее
От: Greg Stark
Дата:
Сообщение: Re: longjmp clobber warnings are utterly broken in modern gcc