Solaris testers wanted for strxfrm() behavior

On Sat, Jun 27, 2015 at 9:51 AM, Noah Misch <noah@leadboat.com> wrote:

PostgreSQL 9.5 adds a strxfrm() call in bttext_abbrev_convert(), which does
not account for the Solaris bug. I wish to determine whether that bug is
still relevant today. If you have access to Solaris with the is_IS.ISO8859-1
locale installed (or root access to install it), please compile and run the
attached test program on each Solaris version you have available. Reply here
with the program's output. I especially need a report from Solaris 10, but
reports from older and newer versions are valuable. Thanks.

I did consider this.

Sorry, but I must question the point of worrying about an ancient bug
in Solaris. When you have to worry about a standard library function
blithely writing past the end of a buffer, when its C89 era interface
must be passed the size of said buffer, where does it end?

This is not a portability concern, like checking for an INT_MAX return
value from strxfrm() on Windows. The Solaris issue is patently a bug
that existed in some particular release of the Solaris C stdlib many
years ago. The documented behavior of strxfrm() in that library was
surely not "We ignore the bufsize argument".
--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

27.06.2015, 19:51, Noah Misch kirjoitti:

convert_string_datum() says:

/*
* Note: originally we guessed at a suitable output buffer size, and
* only needed to call strxfrm twice if our guess was too small.
* However, it seems that some versions of Solaris have buggy strxfrm
* that can write past the specified buffer length in that scenario.
* So, do it the dumb way for portability.

That arrived in commit 59d9a37, and I think this is the background:
/messages/by-id/3224.1020394610@sss.pgh.pa.us

PostgreSQL 9.5 adds a strxfrm() call in bttext_abbrev_convert(), which does
not account for the Solaris bug. I wish to determine whether that bug is
still relevant today. If you have access to Solaris with the is_IS.ISO8859-1
locale installed (or root access to install it), please compile and run the
attached test program on each Solaris version you have available. Reply here
with the program's output. I especially need a report from Solaris 10, but
reports from older and newer versions are valuable. Thanks.

Here is the output on OmniOS r151006, which does not have the bug:

SunOS ip-10-152-178-106.ec2.internal 5.11 omnios-b281e50 i86pc i386 i86xpv
locale "is_IS.ISO8859-1": strxfrm returned 212; last modified byte at 58 (0x0)
locale "is_IS.ISO8859-1": strxfrm returned 212; last modified byte at 58 (0x0)
locale "": strxfrm returned 264; last modified byte at 58 (0x0)

SunOS larry 5.10 Generic_147147-26 sun4u sparc SUNW,Sun-Fire-V215
locale "is_IS.ISO8859-1": strxfrm returned 216; last modified byte at 58
(0x0)
locale "is_IS.ISO8859-1": strxfrm returned 216; last modified byte at 58
(0x0)
locale "": strxfrm returned 26; last modified byte at 27 (0x0)

/ Oskari

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Geoghegan <pg@heroku.com> writes:

On Sat, Jun 27, 2015 at 9:51 AM, Noah Misch <noah@leadboat.com> wrote:

PostgreSQL 9.5 adds a strxfrm() call in bttext_abbrev_convert(), which does
not account for the Solaris bug. I wish to determine whether that bug is
still relevant today. If you have access to Solaris with the is_IS.ISO8859-1
locale installed (or root access to install it), please compile and run the
attached test program on each Solaris version you have available. Reply here
with the program's output. I especially need a report from Solaris 10, but
reports from older and newer versions are valuable. Thanks.

I did consider this.

Sorry, but I must question the point of worrying about an ancient bug
in Solaris.

I think the point of Noah's query is to find out whether "ancient" is an
accurate description. If indeed nothing newer than Solaris 8 exhibits
the bug, I'd be okay with not working around it, but otherwise we have
some decisions to make.

Also, the post Noah dug up was merely the oldest description of the
problem. I believe what prompted us to actually change the code was
some reports in July 2003:

/messages/by-id/56510AAEF435D240958D1CE8C6B1770A016D2DB9@mailc03.aurigin.com
/messages/by-id/56510AAEF435D240958D1CE8C6B1770A016D2DB0@mailc03.aurigin.com

(the archive threading here is pretty crappy, but you can poke around
among posts of similar date)

Those reports suggest that the problem was observable in many non-C
locales on Solaris 8, not only Icelandic.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Jun 27, 2015 at 7:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think the point of Noah's query is to find out whether "ancient" is an
accurate description. If indeed nothing newer than Solaris 8 exhibits
the bug, I'd be okay with not working around it, but otherwise we have
some decisions to make.

Even Solaris 9 is EOL.

Also, the post Noah dug up was merely the oldest description of the
problem. I believe what prompted us to actually change the code was
some reports in July 2003:

/messages/by-id/56510AAEF435D240958D1CE8C6B1770A016D2DB9@mailc03.aurigin.com

You said it yourself at the time -- why trust the strxfrm()
implementation when a NULL pointer is passed? It may have worked on
someone's machine in 2003, but that isn't a very good reason. It was
never a documented part of the interface that this fails or that
works; how could it be? This Solaris strxfrm() issue is (in the
simplest and least contentious sense) a bug. It is not a portability
issue. Someone made a mistake, and most likely the mistake was
corrected in the next point release. That is the only logical
explanation I can see.

I wouldn't say that adding defenses to workaround serious bugs in a C
stdlib is something we should never do, but ISTM that the standard for
undertaking that ought to be very high. BTW, unlike the author of
convert_string_datum(), I think it's okay that glibc sometimes returns
different values with strxfrm() calls on the same string (based on
whether or not the passed-in buffer is actually big enough to store
the transformed string) -- that is actually allowed by the standard, I
think. In other words, Solaris 8 has the only actually buggy strxfrm()
of the cases convert_string_datum() enumerates, since AFAICT the
standard doesn't promise that the strxfrm() return value need be
exact, only sufficient (this leeway seems useful to me).

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Geoghegan <pg@heroku.com> writes:

On Sat, Jun 27, 2015 at 7:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think the point of Noah's query is to find out whether "ancient" is an
accurate description.

You said it yourself at the time -- why trust the strxfrm()
implementation when a NULL pointer is passed? It may have worked on
someone's machine in 2003, but that isn't a very good reason. It was
never a documented part of the interface that this fails or that
works; how could it be? This Solaris strxfrm() issue is (in the
simplest and least contentious sense) a bug. It is not a portability
issue. Someone made a mistake, and most likely the mistake was
corrected in the next point release.

The point here is to *find out*, rather than assuming. I agree that
Sun should have been embarrassed that such a bug ever made it into
a released libc, but it did. The question is how long did it take
them to notice and fix it. Assuming that it happened in "the next point
release", with absolutely no evidence to support that optimistic guess,
doesn't seem like a good idea to me.

(BTW, another way to settle the question might be to check the Solaris
release history. Wonder if that's available anywhere.)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jun 28, 2015 at 8:31 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The point here is to *find out*, rather than assuming. I agree that
Sun should have been embarrassed that such a bug ever made it into
a released libc, but it did. The question is how long did it take
them to notice and fix it. Assuming that it happened in "the next point
release", with absolutely no evidence to support that optimistic guess,
doesn't seem like a good idea to me.

(BTW, another way to settle the question might be to check the Solaris
release history. Wonder if that's available anywhere.)

I did attempt that before abbreviated keys went in, too. I didn't have
much luck finding the relevant information.

*My* point here is that this seems like taking unbounded
responsibility for a massive number of standard library bugs --
indisputable, garden-variety bugs. This bug was really likely to
result in a segfault back when Postgres lacked (but also arguably
needed) defenses against it, which was a period of about a year. The
reason that defenses were added was because there were several
complaints, but maybe those complaints were misdirected.

It might have been the right decision at the time to paper over the
problem, but only for a year or two. I'd only favor adding defenses if
it could be expected to take longer for the Solaris stdlib people to
ship a fix for their egregious bug than it would take for the next
Postgres point release. Why should that be true, though?

This just happened to be one of the more embarrassing, obvious stdlib
bugs that appeared in the last 15 years, and so we're talking about it
now, but there are plenty more, a good deal of which are far more
recent than this one. Where does it end? I don't see why this bug is
special just because 4 or 5 people complained about it on
pgsql-hackers over a decade ago.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/28/2015 12:29 PM, Peter Geoghegan wrote:

It might have been the right decision at the time to paper over the
problem, but only for a year or two. I'd only favor adding defenses if
it could be expected to take longer for the Solaris stdlib people to
ship a fix for their egregious bug than it would take for the next
Postgres point release. Why should that be true, though?

BTW, I've asked Joyent to test on their platform.

My perspective is that if both SmartOS and OmniOS pass, it's not our
responsibility to support OldSolaris if they won't update libraries.

However, OpenSCG and EDB support more OldSolaris customers than anyone,
AFAIK, so I'd like to her some opinions from them ...

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 29, 2015 at 7:58 AM, Josh Berkus <josh@agliodbs.com> wrote:

My perspective is that if both SmartOS and OmniOS pass, it's not our
responsibility to support OldSolaris if they won't update libraries.

Just by the way, I wonder if this was that bug:

https://illumos.org/issues/1594

--
Thomas Munro
http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jun 28, 2015 at 12:58 PM, Josh Berkus <josh@agliodbs.com> wrote:

My perspective is that if both SmartOS and OmniOS pass, it's not our
responsibility to support OldSolaris if they won't update libraries.

Obviously I especially don't want to double the number of strxfrm()
calls made during text abbreviation for *everyone* just to work around
this silly bug. Those calls will generally be a large fraction of the
cost of any text sort in 9.5, so clearly that would be unacceptable.

Maybe Noah should commit a patch that makes the initial size of the
buffer that stores the transformed string blob very small. This can be
reverted once it has some buildfarm cycles. That is a bit of a scatter
gun approach, but maybe that's inevitable given the paucity of
information around the issue.
--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thomas Munro <thomas.munro@enterprisedb.com> writes:

Just by the way, I wonder if this was that bug:
https://illumos.org/issues/1594

Oooh. Might or might not be *same* bug, but it sure looks like it could
have the right symptom. If this is indeed inherited from old Solaris,
I'm afraid we are totally fooling ourselves if we guess that it's no
longer present in the wild.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Geoghegan <pg@heroku.com> writes:

On Sun, Jun 28, 2015 at 12:58 PM, Josh Berkus <josh@agliodbs.com> wrote:

My perspective is that if both SmartOS and OmniOS pass, it's not our
responsibility to support OldSolaris if they won't update libraries.

Obviously I especially don't want to double the number of strxfrm()
calls made during text abbreviation for *everyone* just to work around
this silly bug. Those calls will generally be a large fraction of the
cost of any text sort in 9.5, so clearly that would be unacceptable.

I agree, but ...

Maybe Noah should commit a patch that makes the initial size of the
buffer that stores the transformed string blob very small. This can be
reverted once it has some buildfarm cycles. That is a bit of a scatter
gun approach, but maybe that's inevitable given the paucity of
information around the issue.

Another idea would be to make a test during postmaster start to see
if this bug exists, and fail if so. I'm generally on board with the
thought that we don't need to work on systems with such a bad bug,
but it would be a good thing if the failure was clean and produced
a helpful error message, rather than looking like a Postgres bug.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Geoghegan <pg@heroku.com> writes:

It might have been the right decision at the time to paper over the
problem, but only for a year or two. I'd only favor adding defenses if
it could be expected to take longer for the Solaris stdlib people to
ship a fix for their egregious bug than it would take for the next
Postgres point release. Why should that be true, though?

This just happened to be one of the more embarrassing, obvious stdlib
bugs that appeared in the last 15 years, and so we're talking about it
now, but there are plenty more, a good deal of which are far more
recent than this one. Where does it end? I don't see why this bug is
special just because 4 or 5 people complained about it on
pgsql-hackers over a decade ago.

Peter, your arguments are beginning to sound like desperate excuse-making.

The reason that bug is "special" is that it looks like a crash in
Postgres, one that people have complained of because they didn't see it
in other programs, which is not totally surprising because it requires
a somewhat unusual usage of strxfrm(). I think the dumb two-call
implementation exhibited in convert_string_datum() is mainstream usage,
which would explain why Sun hadn't noticed the bug ages ago.

It might be all right to refuse to support platforms that have this
bug, but I think we need to determine with some more clarity just
how widespread the bug still is, and in any case we should at least
install some defense that would allow us to report that libc is
buggy. Otherwise we'll be back to fielding bug reports that trace
to this, which is no fun for anyone and does not make us look good.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jun 28, 2015 at 7:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Geoghegan <pg@heroku.com> writes:

It might have been the right decision at the time to paper over the
problem, but only for a year or two. I'd only favor adding defenses if
it could be expected to take longer for the Solaris stdlib people to
ship a fix for their egregious bug than it would take for the next
Postgres point release. Why should that be true, though?

This just happened to be one of the more embarrassing, obvious stdlib
bugs that appeared in the last 15 years, and so we're talking about it
now, but there are plenty more, a good deal of which are far more
recent than this one. Where does it end? I don't see why this bug is
special just because 4 or 5 people complained about it on
pgsql-hackers over a decade ago.

Peter, your arguments are beginning to sound like desperate excuse-making.

The reason that bug is "special" is that it looks like a crash in
Postgres, one that people have complained of because they didn't see it
in other programs, which is not totally surprising because it requires
a somewhat unusual usage of strxfrm(). I think the dumb two-call
implementation exhibited in convert_string_datum() is mainstream usage,
which would explain why Sun hadn't noticed the bug ages ago.

It might be all right to refuse to support platforms that have this
bug, but I think we need to determine with some more clarity just
how widespread the bug still is, and in any case we should at least
install some defense that would allow us to report that libc is
buggy. Otherwise we'll be back to fielding bug reports that trace
to this, which is no fun for anyone and does not make us look good.

I completely agree. Noah is quite right to try to find out whether
this is still an issue, and I'm glad he's doing it, and I think it's
very unfortunate that Peter is trying to discourage that research. If
in fact the bug does not still exist in the wild, then Noah's research
will demonstrate that we have no problem, and we do not need to do
anything. If it does, then we can decide what to do about that. But
I have come to value Noah's diligent attitude towards hunting down
problems in our code base, and I hope Peter (and everyone else) will
appreciate that attitude as well - or at the very least, stay out of
the way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jun 28, 2015 at 4:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The reason that bug is "special" is that it looks like a crash in
Postgres, one that people have complained of because they didn't see it
in other programs, which is not totally surprising because it requires
a somewhat unusual usage of strxfrm(). I think the dumb two-call
implementation exhibited in convert_string_datum() is mainstream usage,
which would explain why Sun hadn't noticed the bug ages ago.

It hardly matters much, but I don't think that it is. I think the
issue is entirely explained by sloppy code in the Solaris 8 stdlib.

Anyone using strxfrm() is probably doing so for exactly the same
reason as bttext_abbrev_convert() -- making sorting text in a
collation-aware fashion faster. That's what strxfrm() is more or less
advertised for by the glibc docs, for example, and in fact the
strxfrm() glibc example involves resizing a buffer to fit [1]http://www.gnu.org/software/libc/manual/html_node/Collation-Functions.html -- Peter Geoghegan, to
avoid doubling the number of strxfrm() calls.

For that reason, I think that convert_string_datum() is much further
from mainstream usage than bttext_abbrev_convert().

It might be all right to refuse to support platforms that have this
bug, but I think we need to determine with some more clarity just
how widespread the bug still is, and in any case we should at least
install some defense that would allow us to report that libc is
buggy. Otherwise we'll be back to fielding bug reports that trace
to this, which is no fun for anyone and does not make us look good.

I can definitely get behind having a defense at startup. That seems
like cheap insurance. It probably isn't too much work to adopt Noah's
test program.

[1]: http://www.gnu.org/software/libc/manual/html_node/Collation-Functions.html -- Peter Geoghegan
--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 29, 2015 at 10:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@enterprisedb.com> writes:

Just by the way, I wonder if this was that bug:
https://illumos.org/issues/1594

Oooh. Might or might not be *same* bug, but it sure looks like it could
have the right symptom. If this is indeed inherited from old Solaris,
I'm afraid we are totally fooling ourselves if we guess that it's no
longer present in the wild.

Also, here is an interesting patch that went into the Apache C++
standard library. Maybe the problem was limited to amd64 system...

https://github.com/illumos/illumos-userland/blob/master/components/stdcxx/patches/047-collate.cpp.patch

--
Thomas Munro
http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jun 28, 2015 at 4:35 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I completely agree. Noah is quite right to try to find out whether
this is still an issue, and I'm glad he's doing it, and I think it's
very unfortunate that Peter is trying to discourage that research.

Far from it. I am providing constructive feedback.

If in fact the bug does not still exist in the wild, then Noah's research
will demonstrate that we have no problem, and we do not need to do
anything. If it does, then we can decide what to do about that. But
I have come to value Noah's diligent attitude towards hunting down
problems in our code base, and I hope Peter (and everyone else) will
appreciate that attitude as well - or at the very least, stay out of
the way.

I hope that it goes without saying that I greatly appreciate Noah's
efforts in tracking down this kind of thing. I am not standing in
anyone's way. My intent is to save Noah some work. I imagined that it
might not be clear to him how completely unreasonable it is for a
strxfrm() implementation to have this issue -- it is totally
unreasonable. It is not a portability problem. Clearly Tom agrees with
that view, since he stated that he thinks it's okay to not support a
platform with an abjectly broke strxfrm().

Now we're talking about failing in a sane way, rather than trying to
add band aids, and now the exact extent of the problem seems less
important.
--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jun 28, 2015 at 4:35 PM, Peter Geoghegan <pg@heroku.com> wrote:

It hardly matters much, but I don't think that it is. I think the
issue is entirely explained by sloppy code in the Solaris 8 stdlib.

I don't imagine that it will come as a surprise to anybody, but the
manpage [1]http://docs.oracle.com/cd/E19455-01/806-0627/6j9vhfn88/index.html#indexterm-1090 -- Peter Geoghegan for strxfrm() covering old Solaris libc versions
(predating even version 8) states:

"No more than n bytes are placed into the resulting array pointed to
by s1, including the terminating null byte" ('n' is the bufsize
argument passed by the caller).

This is proof positive (though it is hardly needed) that Solaris was
never supposed to allow this. I also noticed that the manpage had a
typo:

"Because no return value is reserved to indicate an error, an
application wishing to check for error situations should set errno to
0, then call strcoll(3C), then check errno and if it is non-zero,
assume an error has occurred."

Obviously this is a copy-paste leftover from the strcoll() manpage.
Pretty slipshod for a C standard library implementation, I would say.

[1]: http://docs.oracle.com/cd/E19455-01/806-0627/6j9vhfn88/index.html#indexterm-1090 -- Peter Geoghegan
--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

All,

Joyent confirms that the bug is fixed on SmartOS:

This is what I see:

SunOS pkgsrc-pbulk-2014Q4-1.local 5.11 joyent_20141030T081701Z i86pc
i386 i86pc
locale "is_IS.ISO8859-1": strxfrm returned 212; last modified byte at
58 (0x0)
locale "is_IS.ISO8859-1": strxfrm returned 212; last modified byte at
58 (0x0)
locale "": strxfrm returned 26; last modified byte at 27 (0x0)

Cheers,

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Josh Berkus <josh@agliodbs.com> writes:

Joyent confirms that the bug is fixed on SmartOS:

The more interesting bit of information would be *when* it was fixed.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers