Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
On 10/23/2013 03:05 AM, Noah Misch wrote: I would vote for choosing the standard we want vsnprintf() to follow (probably C99) and substituting a conforming implementation wherever configure detects that libc does not conform. We'll be shipping some replacement vsnprintf() in any case; we may as well use it to insulate the rest of our code from less-preferred variants. Do you care about the snprintf behavior on very large buffers (larger than INT_MAX)? Then there's further complication, and it's an area where glibc behavior is likely to change in the future (because it is claimed that C99 and POSIX conflict, and glibc implements neither behavior). -- Florian Weimer / Red Hat Product Security Team -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
On Wed, Oct 23, 2013 at 4:00 AM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah. As a separate matter, it might be useful to revise stringinfo.c and the asprintf code so that *if* the returned value is larger than the given buffer size, we use it as a guide to resizing, avoiding the possible need to loop multiple times to make the buffer large enough. And we could also improve our own implementation of snprintf to follow the C99 spec. Attached is a draft patch which implements this. I didn't bother making the translation macros make use of the extra data as I didn't think we would have many translations long enough to take advantage of it. I think it's a good idea to take advantage of the buffer size if vsnprintf() has gone to the trouble of working out what is needed for us. It seems quite wasteful to throw this information away. Comments are welcome. Regards David appendStringInfoVA.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
Florian Weimer fwei...@redhat.com writes: Do you care about the snprintf behavior on very large buffers (larger than INT_MAX)? Then there's further complication, and it's an area where glibc behavior is likely to change in the future (because it is claimed that C99 and POSIX conflict, and glibc implements neither behavior). We do not. Note that the buffer enlargement behavior is designed not to let len exceed INT_MAX; it'll say out of memory instead. Given that vsnprintf is defined to return int, buffers larger than INT_MAX would be a real can of worms, one that we'd best not open. 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
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
David Rowley dgrowle...@gmail.com writes: On Wed, Oct 23, 2013 at 4:00 AM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah. As a separate matter, it might be useful to revise stringinfo.c and the asprintf code so that *if* the returned value is larger than the given buffer size, we use it as a guide to resizing, avoiding the possible need to loop multiple times to make the buffer large enough. And we could also improve our own implementation of snprintf to follow the C99 spec. Attached is a draft patch which implements this. I started working on a very similar patch last night, but then began to wonder if it wouldn't be better to try to share code between stringinfo.c and psprintf.c --- that is, expose the latter's pvsnprintf and use that in stringinfo. 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
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
On Tue, Oct 22, 2013 at 8:58 PM, Tom Lane t...@sss.pgh.pa.us wrote: So I returned from vacation only to find that the buildfarm has a bad case of acne. All the Windows members are red or pink, and have been for awhile. Sigh. After some research I believe that I understand the reason for the CHECK failures, at least: 1. src/port/asprintf.c exhibits a truly touching faith that vsnprintf will report exactly the number of bytes that would have been required, even if the buffer is not that large. While this is what is specified in recent versions of the POSIX standard, older platforms have much sketchier behavior. 2. In particular, our own src/port/snprintf.c follows the SUS v2 rule that it should report the number of bytes it *actually wrote*. This means that asprintf.c will never think that its initial 128-byte allocation was insufficient. So, on platforms where we use this implementation (notably including Windows), the result of any asprintf call is effectively truncated at 128 bytes. Thanks for looking at this. I had a bash and trying to figure out why vcregress check would not work last night and didn't get very far... I can confirm that you are right just by changing the 128 into 12800 and compiling, vcregress check worked after that. Regards David Rowley I have a lot of other gripes about this whole patch, but they can wait till tomorrow. 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
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
On 22/10/2013 09:58, Tom Lane wrote: So I returned from vacation only to find that the buildfarm has a bad case of acne. All the Windows members are red or pink, and have been for awhile. Sigh. After some research I believe that I understand the reason for the CHECK failures, at least: 1. src/port/asprintf.c exhibits a truly touching faith that vsnprintf will report exactly the number of bytes that would have been required, even if the buffer is not that large. While this is what is specified in recent versions of the POSIX standard, older platforms have much sketchier behavior. Just to be pedantic, this is required by C99. [...] Regards Manlio Perillo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
Manlio Perillo manlio.peri...@gmail.com writes: On 22/10/2013 09:58, Tom Lane wrote: 1. src/port/asprintf.c exhibits a truly touching faith that vsnprintf will report exactly the number of bytes that would have been required, even if the buffer is not that large. While this is what is specified in recent versions of the POSIX standard, older platforms have much sketchier behavior. Just to be pedantic, this is required by C99. Yeah. As a separate matter, it might be useful to revise stringinfo.c and the asprintf code so that *if* the returned value is larger than the given buffer size, we use it as a guide to resizing, avoiding the possible need to loop multiple times to make the buffer large enough. And we could also improve our own implementation of snprintf to follow the C99 spec. The point here is that we still need to cope with pre-C99 implementations that might return -1 or the given buffer size on overflow. The NetBSD implementation doesn't do that, which is reasonable in their context, but not workable for us. 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
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
On Tue, Oct 22, 2013 at 11:00:42AM -0400, Tom Lane wrote: Yeah. As a separate matter, it might be useful to revise stringinfo.c and the asprintf code so that *if* the returned value is larger than the given buffer size, we use it as a guide to resizing, avoiding the possible need to loop multiple times to make the buffer large enough. And we could also improve our own implementation of snprintf to follow the C99 spec. The point here is that we still need to cope with pre-C99 implementations that might return -1 or the given buffer size on overflow. The NetBSD implementation doesn't do that, which is reasonable in their context, but not workable for us. I would vote for choosing the standard we want vsnprintf() to follow (probably C99) and substituting a conforming implementation wherever configure detects that libc does not conform. We'll be shipping some replacement vsnprintf() in any case; we may as well use it to insulate the rest of our code from less-preferred variants. -- Noah Misch EnterpriseDB 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
Re: [HACKERS] Why the asprintf patch is still breaking the buildfarm
Noah Misch n...@leadboat.com writes: On Tue, Oct 22, 2013 at 11:00:42AM -0400, Tom Lane wrote: Yeah. As a separate matter, it might be useful to revise stringinfo.c and the asprintf code so that *if* the returned value is larger than the given buffer size, we use it as a guide to resizing, avoiding the possible need to loop multiple times to make the buffer large enough. And we could also improve our own implementation of snprintf to follow the C99 spec. The point here is that we still need to cope with pre-C99 implementations that might return -1 or the given buffer size on overflow. The NetBSD implementation doesn't do that, which is reasonable in their context, but not workable for us. I would vote for choosing the standard we want vsnprintf() to follow (probably C99) and substituting a conforming implementation wherever configure detects that libc does not conform. We'll be shipping some replacement vsnprintf() in any case; we may as well use it to insulate the rest of our code from less-preferred variants. The problem is that we can't tell whether vsnprintf is standard-conforming without a run-time test. That's bad for cross-compiled builds, and it's pretty hazardous even for normal cases, since conceivably an executable built on one machine could be used on another one with different run-time behavior. I'd be willing to take those risks if we got a significant benefit from it, but in this case I don't see much advantage to be had. The code in stringinfo/psprintf wouldn't get very much simpler if we assumed C99 behavior, and we've pretty well isolated the number of places that care to those. (I see a couple places in pg_dump that could be modified to use psprintf instead of direct vsnprintf calls; will go fix.) 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
