Thread: Error handling fix in interfaces/libpq/fe-secure.c

Here's another one similar to what I described in my previous message.  In
libpq's pqsecure_read(), if SSL_read() returns -1 and sets an error of
SSL_ERROR_SYSCALL, errno may be polluted by libpq_gettext() before a
human-readable string is derived from it.  Also, pqReadData() will see the
wrong errno value after the call.

The attached patch fixes both by introducing a named variable to hold the
significant value of errno.


Jeroen

jtv@xs4all.nl writes:
> Here's another one similar to what I described in my previous message.

More or less proves my point, no?  Even if you manage to fix every
occurence of this issue now, it'll keep popping up as people change
the code.  This approach is just not maintainable.

            regards, tom lane

BTW, I read at

http://www.gnu.org/software/libc/manual/html_node/Translation-with-gettext.html

  The gettext function does not modify the value of the global errno
  variable. This is necessary to make it possible to write something like

                 printf (gettext ("Operation failed: %m\n"));

which is pretty much what I expected to find.  Ergo, this entire
discussion is wrong, and whatever bug you are concerned about will
not be solved this way.

What you may actually be running into is the problem that there are two
different definitions of strerror_r() out there, the SUS spec and the
GNU spec, and pre-8.0 we failed to distinguish these.  The 7.4 coding
will yield garbage messages in some cases when GNU strerror_r is in use.

            regards, tom lane

Tom Lane wrote:
>   The gettext function does not modify the value of the global errno
>   variable. This is necessary to make it possible to write something like
>
>                  printf (gettext ("Operation failed: %m\n"));
>
> which is pretty much what I expected to find.  Ergo, this entire
> discussion is wrong, and whatever bug you are concerned about will
> not be solved this way.

Tom, I didn't know that gettext() preserved errno--but I still believe
you're wrong.  The problem is not gettext() but libpq_gettext().  The
latter calls the former, but it may go through initialization first--which
would still pollute errno on the first call.  And that first call may well
be the "failed to connect" message that so many people have been seeing
replaced with garbage data.

Given that gettext() doesn't pollute errno, you'd see this problem occur
only if the first error message you got from libpq included an errno-based
string.


> What you may actually be running into is the problem that there are two
> different definitions of strerror_r() out there, the SUS spec and the
> GNU spec, and pre-8.0 we failed to distinguish these.  The 7.4 coding
> will yield garbage messages in some cases when GNU strerror_r is in use.

Actually I'm fairly sure that people have been seeing the problem with
8.0.  In fact I had so much trouble getting a portable, reliable result
for my strerror_r check in the libpqxx configure script that I ended up
checking for both versions, and then verifying that at least one of the
checks failed.  I use function pointer assignments now, but I may end up
adding the "repeated declaration" method of checking back in as well.  My
situation is a bit different from that of postgres since the base language
is C++.

On a side note, is there any risk of a packager building libpq against a
GNU-style strerror_r() and the user who downloads the binary then loading
it against a SUS-style strerror_r() or vice versa?


Jeroen

jtv@xs4all.nl writes:
> Tom Lane wrote:
>> The gettext function does not modify the value of the global errno
>> variable. This is necessary to make it possible to write something like
>>
>> printf (gettext ("Operation failed: %m\n"));
>>
>> which is pretty much what I expected to find.  Ergo, this entire
>> discussion is wrong, and whatever bug you are concerned about will
>> not be solved this way.

> Tom, I didn't know that gettext() preserved errno--but I still believe
> you're wrong.  The problem is not gettext() but libpq_gettext().  The
> latter calls the former, but it may go through initialization first--which
> would still pollute errno on the first call.

Good point --- we should make it save and restore errno around the
bindtextdomain() call.  Will do.

            regards, tom lane