[HACKERS] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/28/2012 11:03 PM, Tom Lane wrote: [ digs around ... ] It looks like the failure is coming from here: if (strlen(path) = sizeof(unp-sun_path)) return EAI_FAIL; Looks like it was. Good catch. What's the best way to fix? So far as I can see, none of the spec-defined EAI_XXX codes map very nicely to path name too long. Possibly we could return EAI_SYSTEM and set errno to ENAMETOOLONG, but I'm not sure the latter is very portable either. Another line of attack is to just teach getnameinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. The portability risk here is if sun_path is not the last field in struct sockaddr_un on some platform --- but that seems a bit unlikely, and even if it isn't I doubt we access any other members besides sun_family and sun_path. I kind of like this approach, since it gets rid of the length limitation rather than just reporting it more clearly. 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
I wrote: Andrew Dunstan and...@dunslane.net writes: Looks like it was. Good catch. What's the best way to fix? So far as I can see, none of the spec-defined EAI_XXX codes map very nicely to path name too long. Possibly we could return EAI_SYSTEM and set errno to ENAMETOOLONG, but I'm not sure the latter is very portable either. I tried this out and found that at least on Linux, gai_strerror() is too stupid to pay attention to errno anyway; you just get System error, which is about as unhelpful as it could possibly be. I don't see any way that we can get a more specific error message to be printed without eliminating use of gai_strerror and providing our own infrastructure for reporting getaddrinfo errors. While that wouldn't be incredibly awful (we have such infrastructure already for ancient platforms...), it still kinda sucks. Another line of attack is to just teach getaddrinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. I tried this out too, and found that it doesn't work well, because both libpq and the backend expect to be able to copy getaddrinfo results into fixed-size SockAddr structs. We could probably fix that by adding another layer of pointers and malloc operations, but it would be somewhat invasive. Given the lack of prior complaints it's not clear to me that it's worth that much trouble --- although getting rid of our hard-wired assumptions about the maximum result size from getaddrinfo is attractive from a robustness standpoint. I'm a bit tempted to just replace EAI_FAIL with EAI_MEMORY here, so that you'd get messages similar to Memory allocation failure. That might at least point your thoughts in the right direction, whereas Non-recoverable failure in name resolution certainly conveys nothing of use. Thoughts? 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 03:33 PM, Tom Lane wrote: I wrote: Andrew Dunstan and...@dunslane.net writes: Looks like it was. Good catch. What's the best way to fix? So far as I can see, none of the spec-defined EAI_XXX codes map very nicely to path name too long. Possibly we could return EAI_SYSTEM and set errno to ENAMETOOLONG, but I'm not sure the latter is very portable either. I tried this out and found that at least on Linux, gai_strerror() is too stupid to pay attention to errno anyway; you just get System error, which is about as unhelpful as it could possibly be. I don't see any way that we can get a more specific error message to be printed without eliminating use of gai_strerror and providing our own infrastructure for reporting getaddrinfo errors. While that wouldn't be incredibly awful (we have such infrastructure already for ancient platforms...), it still kinda sucks. Another line of attack is to just teach getaddrinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. I tried this out too, and found that it doesn't work well, because both libpq and the backend expect to be able to copy getaddrinfo results into fixed-size SockAddr structs. We could probably fix that by adding another layer of pointers and malloc operations, but it would be somewhat invasive. Given the lack of prior complaints it's not clear to me that it's worth that much trouble --- although getting rid of our hard-wired assumptions about the maximum result size from getaddrinfo is attractive from a robustness standpoint. I'm a bit tempted to just replace EAI_FAIL with EAI_MEMORY here, so that you'd get messages similar to Memory allocation failure. That might at least point your thoughts in the right direction, whereas Non-recoverable failure in name resolution certainly conveys nothing of use. Thoughts? I don't think it's worth a heroic effort. Meanwhile I'll add a check in the upgrade test module(s) to check for overly long paths likely to give problems. cheers andrew -- 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 03:33 PM, Tom Lane wrote: Another line of attack is to just teach getaddrinfo_unix() to malloc its result struct big enough to hold whatever the supplied path is. I tried this out too, and found that it doesn't work well, because both libpq and the backend expect to be able to copy getaddrinfo results into fixed-size SockAddr structs. We could probably fix that by adding another layer of pointers and malloc operations, but it would be somewhat invasive. Given the lack of prior complaints it's not clear to me that it's worth that much trouble --- although getting rid of our hard-wired assumptions about the maximum result size from getaddrinfo is attractive from a robustness standpoint. I don't think it's worth a heroic effort. Meanwhile I'll add a check in the upgrade test module(s) to check for overly long paths likely to give problems. I'm thinking maybe we should try to fix this. What's bugging me is that Jeremy's build path doesn't look all that unreasonably long. The scary scenario that's in the back of my mind is that one day somebody decides to rearrange the Red Hat build servers a bit and suddenly I can't build Postgres there anymore, because the build directory is nested a bit too deep. Murphy's law would of course dictate that I find this out only when under the gun to get a security update packaged. 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Noah Misch n...@leadboat.com writes:
Linux enforces a hard limit matching the static buffer in sockaddr_un. You'd
proceed a bit further and hit could not bind Unix socket: Invalid argument
or some such.
Hm, I was wondering about that. The Single Unix Spec suggests that
bind/connect ought to accept pathnames at least up to PATH_MAX, but
if popular implementations don't honor that then it is a bit pointless
for us to do a lot of pushups in userspace.
I agree we should perhaps fix pg_upgrade to work even when its CWD is not
usable as a socket path. It could create a temporary directory under /tmp and
place the socket there, for example.
Yeah, I was starting to think that pg_upgrade's test script is the real
culprit here. Every other variant of make check just puts the socket
in the default place, typically /tmp, so it's rather useless that this
one place is doing things differently.
Another thing that we should possibly consider if we're going to hack on
that is that make check is not currently very friendly to people who
try to move the default socket location to someplace other than /tmp,
such as the ever-popular /var/run/postgresql. The reason that this is
problematic is that /var/run/postgresql may not be there at all in a
build environment, and if it is, it's likely not writable by the user
you're running your build as. So just using the default socket
directory isn't real friendly in any case. In converting the Fedora
packages to use /var/run/postgresql recently, I found I had to add the
attached crude hacks to support running the regression tests during
build. It'd be nice if the consideration were handled by unmodified
sources ...
regards, tom lane
diff -Naur postgresql-9.2.0.sockets/contrib/pg_upgrade/test.sh
postgresql-9.2.0/contrib/pg_upgrade/test.sh
--- postgresql-9.2.0.sockets/contrib/pg_upgrade/test.sh 2012-09-06
17:26:17.0 -0400
+++ postgresql-9.2.0/contrib/pg_upgrade/test.sh 2012-09-06 18:13:18.178092176
-0400
@@ -62,10 +62,14 @@
rm -rf $logdir
mkdir $logdir
+# we want the Unix sockets in $temp_root
+PGHOST=$temp_root
+export PGHOST
+
set -x
$oldbindir/initdb
-$oldbindir/pg_ctl start -l $logdir/postmaster1.log -w
+$oldbindir/pg_ctl start -l $logdir/postmaster1.log -o -c
unix_socket_directories='$PGHOST' -w
if $MAKE -C $oldsrc installcheck; then
pg_dumpall -f $temp_root/dump1.sql || pg_dumpall1_status=$?
if [ $newsrc != $oldsrc ]; then
@@ -108,7 +112,7 @@
pg_upgrade -d ${PGDATA}.old -D ${PGDATA} -b $oldbindir -B $bindir
-pg_ctl start -l $logdir/postmaster2.log -w
+pg_ctl start -l $logdir/postmaster2.log -o -c
unix_socket_directories='$PGHOST' -w
if [ $testhost = Msys ] ; then
cmd /c analyze_new_cluster.bat
diff -Naur postgresql-9.2.0.sockets/src/test/regress/pg_regress.c
postgresql-9.2.0/src/test/regress/pg_regress.c
--- postgresql-9.2.0.sockets/src/test/regress/pg_regress.c 2012-09-06
17:26:17.0 -0400
+++ postgresql-9.2.0/src/test/regress/pg_regress.c 2012-09-06
18:13:18.184092537 -0400
@@ -772,7 +772,7 @@
if (hostname != NULL)
doputenv(PGHOST, hostname);
else
- unsetenv(PGHOST);
+ doputenv(PGHOST, /tmp);
unsetenv(PGHOSTADDR);
if (port != -1)
{
@@ -2233,7 +2233,7 @@
*/
header(_(starting postmaster));
snprintf(buf, sizeof(buf),
-SYSTEMQUOTE \%s/postgres\ -D \%s/data\
-F%s -c \listen_addresses=%s\ \%s/log/postmaster.log\ 21 SYSTEMQUOTE,
+SYSTEMQUOTE \%s/postgres\ -D \%s/data\
-F%s -c \listen_addresses=%s\ -c \unix_socket_directories=/tmp\
\%s/log/postmaster.log\ 21 SYSTEMQUOTE,
bindir, temp_install,
debug ? -d 5 : ,
hostname ? hostname : ,
--
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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 05:20 PM, Tom Lane wrote: I don't think it's worth a heroic effort. Meanwhile I'll add a check in the upgrade test module(s) to check for overly long paths likely to give problems. I'm thinking maybe we should try to fix this. What's bugging me is that Jeremy's build path doesn't look all that unreasonably long. The scary scenario that's in the back of my mind is that one day somebody decides to rearrange the Red Hat build servers a bit and suddenly I can't build Postgres there anymore, because the build directory is nested a bit too deep. Murphy's law would of course dictate that I find this out only when under the gun to get a security update packaged. The only thing it breaks AFAIK is pg_upgrade testing because pg_upgrade insists on setting the current directory as the socket dir. Maybe we need a pg_upgrade option to specify the socket dir to use. Or maybe the postmaster needs to check the length somehow before it calls StreamServerPort() so we can give a saner error message. cheers andrew -- 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: ... Or maybe the postmaster needs to check the length somehow before it calls StreamServerPort() so we can give a saner error message. Hm. That's ugly, but a lot less invasive than trying to get the official API to pass the information through. Sounds like a plan to me. However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 06:23 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: ... Or maybe the postmaster needs to check the length somehow before it calls StreamServerPort() so we can give a saner error message. Hm. That's ugly, but a lot less invasive than trying to get the official API to pass the information through. Sounds like a plan to me. However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. The test script doesn't do anything. It's pg_upgrade itself that sets the socket dir. See option.c:get_sock_dir() cheers andrew -- 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 06:23 PM, Tom Lane wrote: However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. The test script doesn't do anything. It's pg_upgrade itself that sets the socket dir. See option.c:get_sock_dir() Um ... that's *another* place that needs to change then, because the test script fires up postmasters on its own, outside of pg_upgrade. 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Tom Lane t...@sss.pgh.pa.us writes:
Andrew Dunstan and...@dunslane.net writes:
... Or maybe the
postmaster needs to check the length somehow before it calls
StreamServerPort() so we can give a saner error message.
Hm. That's ugly, but a lot less invasive than trying to get the
official API to pass the information through. Sounds like a plan to me.
Here's a patch for that --- I think we should apply and back-patch this.
regards, tom lane
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 5e86987f221fee01c5049f925492e0f9c441d372..15a01a8324b8a6ff9727a1b02e7ba8fe385d3a39 100644
*** a/src/backend/libpq/pqcomm.c
--- b/src/backend/libpq/pqcomm.c
*** StreamServerPort(int family, char *hostN
*** 308,313
--- 308,321
* that file path
*/
UNIXSOCK_PATH(unixSocketPath, portNumber, unixSocketDir);
+ if (strlen(unixSocketPath) = UNIXSOCK_PATH_BUFLEN)
+ {
+ ereport(LOG,
+ (errmsg(Unix-domain socket path \%s\ is too long (maximum %d bytes),
+ unixSocketPath,
+ (int) (UNIXSOCK_PATH_BUFLEN - 1;
+ return STATUS_ERROR;
+ }
if (Lock_AF_UNIX(unixSocketDir, unixSocketPath) != STATUS_OK)
return STATUS_ERROR;
service = unixSocketPath;
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 604b5535df6b24885b782688fee9ff7fd284746b..635132dec9317b4045108c75cd8c67a70c360968 100644
*** a/src/include/libpq/pqcomm.h
--- b/src/include/libpq/pqcomm.h
*** typedef struct
*** 74,79
--- 74,92
(port))
/*
+ * The maximum workable length of a socket path is what will fit into
+ * struct sockaddr_un. This is usually only 100 or so bytes :-(.
+ *
+ * For consistency, always pass a MAXPGPATH-sized buffer to UNIXSOCK_PATH(),
+ * then complain if the resulting string is = UNIXSOCK_PATH_BUFLEN bytes.
+ * (Because the standard API for getaddrinfo doesn't allow it to complain in
+ * a useful way when the socket pathname is too long, we have to test for
+ * this explicitly, instead of just letting the subroutine return an error.)
+ */
+ #define UNIXSOCK_PATH_BUFLEN sizeof(((struct sockaddr_un *) NULL)-sun_path)
+
+
+ /*
* These manipulate the frontend/backend protocol version number.
*
* The major number should be incremented for incompatible changes. The minor
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9eaf41025beb652c6c242035490d93319a6bc5d0..1386bb791a96fab087af1ba33546ab89772327fd 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** static int
*** 1322,1328
connectDBStart(PGconn *conn)
{
int portnum;
! char portstr[128];
struct addrinfo *addrs = NULL;
struct addrinfo hint;
const char *node;
--- 1322,1328
connectDBStart(PGconn *conn)
{
int portnum;
! char portstr[MAXPGPATH];
struct addrinfo *addrs = NULL;
struct addrinfo hint;
const char *node;
*** connectDBStart(PGconn *conn)
*** 1384,1389
--- 1384,1398
node = NULL;
hint.ai_family = AF_UNIX;
UNIXSOCK_PATH(portstr, portnum, conn-pgunixsocket);
+ if (strlen(portstr) = UNIXSOCK_PATH_BUFLEN)
+ {
+ appendPQExpBuffer(conn-errorMessage,
+ libpq_gettext(Unix-domain socket path \%s\ is too long (maximum %d bytes)\n),
+ portstr,
+ (int) (UNIXSOCK_PATH_BUFLEN - 1));
+ conn-options_valid = false;
+ goto connect_errReturn;
+ }
#else
/* Without Unix sockets, default to localhost instead */
node = DefaultHost;
--
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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
On 11/29/2012 07:16 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 06:23 PM, Tom Lane wrote: However, that's only addressing the reporting end of it; I think we also need to change the pg_upgrade test script as suggested by Noah. The test script doesn't do anything. It's pg_upgrade itself that sets the socket dir. See option.c:get_sock_dir() Um ... that's *another* place that needs to change then, because the test script fires up postmasters on its own, outside of pg_upgrade. True, but it doesn't do anything about setting the socket dir, so those just get the compiled-in defaults. What exactly do you want to change about the test script? cheers andrew -- 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] Overlength socket paths (was Re: [COMMITTERS] pgsql: Refactor flex and bison make rules)
Andrew Dunstan and...@dunslane.net writes: On 11/29/2012 07:16 PM, Tom Lane wrote: Um ... that's *another* place that needs to change then, because the test script fires up postmasters on its own, outside of pg_upgrade. True, but it doesn't do anything about setting the socket dir, so those just get the compiled-in defaults. What exactly do you want to change about the test script? Well, I was thinking about also solving the problem that the compiled-in default might be no good in a build environment. 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
