Thread: [9.0beta5/cvs head] build failure due to unchecked results
Hello PostgreSQL developers, 9.0beta5 seems to enable -Werror by default (which is a good thing, thanks!). FORTIFY_SOURCE catches a few places where the result of write() and fgets() is not checked, and thus the build fails with gcc -g -O2 -g -Wall -O2 -fPIC -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing-fwrapv -g -Werror -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/tcl8.5 -c -o elog.o elog.c cc1: warnings being treated as errors elog.c: In function 'write_console': elog.c:1698: error: ignoring return value of 'write', declared with attribute warn_unused_result elog.c: In function 'write_pipe_chunks': elog.c:2390: error: ignoring return value of 'write', declared with attribute warn_unused_result elog.c:2399: error: ignoring return value of 'write', declared with attribute warn_unused_result make[4]: *** [elog.o] Error 1 [...] gcc -g -O2 -g -Wall -O2 -fPIC -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing-fwrapv -g -Werror -pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -fpic -DFRONTEND-DUNSAFE_STAT_OK -I. -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/tcl8.5 -I../../../src/port-I../../../src/port -DSO_MAJOR_VERSION=5 -c -o fe-auth.o fe-auth.c gcc -g -O2 -g -Wall -O2 -fPIC -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing-fwrapv -g -Werror -pthread -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -fpic -DFRONTEND-DUNSAFE_STAT_OK -I. -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/tcl8.5 -I../../../src/port-I../../../src/port -DSO_MAJOR_VERSION=5 -c -o fe-connect.o fe-connect.c cc1: warnings being treated as errors fe-connect.c: In function ‘PasswordFromFile’: fe-connect.c:4403: error: ignoring return value of ‘fgets’, declared with attribute warn_unused_result etc. I attach a patch (against git head) to check the results of those. For src/bin/psql/common.c this is really just an "ignore the result", but in src/bin/psql/prompt.c it actually fixes a potential crash. Thank you for considering! Martin -- Martin Pitt | http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
Attachment
On fre, 2010-04-30 at 12:55 +0200, Martin Pitt wrote: > Hello PostgreSQL developers, > > 9.0beta5 seems to enable -Werror by default (which is a good thing, > thanks!). You probably mean alpha5, unless you come from the future. ;-) That was actually a mistake in the packaging, which is why there is a -revised tarball available. I suggest you hold off for a day or two and wait for beta1.
Peter Eisentraut [2010-04-30 14:56 +0300]: > You probably mean alpha5, unless you come from the future. ;-) FYI, those are next week's lottery numbers: 12, 19, ... Right, of course I mean alpha-5, sorry. > That was actually a mistake in the packaging Oh, I see. Well, for a mistake the code is surprisingly well-behaved. Those three or four patch hunks is all it takes to make it build, and sometimes those warnings are indeed useful; if for nothing else, then to make you think explicitly why it's okay to ignore a short or failed write(). > I suggest you hold off for a day or two and wait for beta1. Sure, thank you! Martin --=20 Martin Pitt | http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
On Friday 30 April 2010 13:56:11 Peter Eisentraut wrote: > On fre, 2010-04-30 at 12:55 +0200, Martin Pitt wrote: > > Hello PostgreSQL developers, > > > > 9.0beta5 seems to enable -Werror by default (which is a good thing, > > thanks!). > > You probably mean alpha5, unless you come from the future. ;-) That was > actually a mistake in the packaging, which is why there is a -revised > tarball available. Isnt it a good idea to set this in the pre-release versions? It might catch potential problems and people trying to compile unreleased software from source should be able to change it/report a bug if necessary. I wouldnt think of considering it for a release version, but thats a different thing (as you will very likely use it with newer compilers and stuff) Andres
On fre, 2010-04-30 at 14:43 +0200, Andres Freund wrote: > On Friday 30 April 2010 13:56:11 Peter Eisentraut wrote: > > > > You probably mean alpha5, unless you come from the future. ;-) That was > > actually a mistake in the packaging, which is why there is a -revised > > tarball available. > Isnt it a good idea to set this in the pre-release versions? It might catch > potential problems and people trying to compile unreleased software from > source should be able to change it/report a bug if necessary. In practice it might prevent more people from building and thus testing PostgreSQL than it would catch actually interesting problems. You also can't assume that the same warning-freeness applies to all platforms.
On Fri, Apr 30, 2010 at 9:14 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On fre, 2010-04-30 at 14:43 +0200, Andres Freund wrote: >> On Friday 30 April 2010 13:56:11 Peter Eisentraut wrote: >> > >> > You probably mean alpha5, unless you come from the future. ;-) =A0That= was >> > actually a mistake in the packaging, which is why there is a -revised >> > tarball available. >> Isnt it a good idea to set this in the pre-release versions? It might ca= tch >> potential problems and people trying to compile unreleased software from >> source should be able to change it/report a bug if necessary. > > In practice it might prevent more people from building and thus testing > PostgreSQL than it would catch actually interesting problems. =A0You also > can't assume that the same warning-freeness applies to all platforms. Yeah, that's the real problem - one doesn't get the same warnings everywhere. Still, I think we should consider applying the portion of the proposed patch that avoid relying on the contents of the fgets() buffer after fgets() returns NULL, because at least on MacOS the man page for gets/fgets says: If an error occurs, they return NULL and the buffer contents are indetermin= ate. Indeterminate is bad. The actual chance of a crash seems low, and I notice that the Linux man page has no such caveat, but still. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> Still, I think we should consider applying the portion of
> the proposed patch that avoid relying on the contents of the fgets()
> buffer after fgets() returns NULL,
I concur, those two changes look worthwhile. The proposed Assert()
additions are right out, though, as they would turn write failures
into database crashes. The current code doesn't even think that such
a failure is worth testing for, so that's surely an overreaction.
(And in any case, if Asserts are disabled, this change would fail to
suppress the warning, no?)
regards, tom lane
Tom Lane [2010-04-30 12:51 -0400]: > I concur, those two changes look worthwhile. The proposed Assert() > additions are right out, though, as they would turn write failures > into database crashes. Right, that might be too strong. > The current code doesn't even think that such a failure is worth > testing for, so that's surely an overreaction. (And in any case, if > Asserts are disabled, this change would fail to suppress the > warning, no?) It seems gcc is happy enough if you assign the returned value to a variable. At least I have done a build without --enable-cassert (where the entire Assert() was thrown away), and it didn't complain about the unchecked result any more. I guess that heuristics gets it only so far.. Thanks, Martin -- Martin Pitt | http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)