Sanity checking for ./configure options?
I just discovered that ./configure will happily accept '--with-pgport='
(I was actually doing =$PGPORT, and didn't realize $PGPORT was empty).
What you end up with is a compile error in guc.c, with no idea why it's
broken. Any reason not to have configure or at least make puke if pgport
isn't valid?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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 Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote:
I just discovered that ./configure will happily accept '--with-pgport=' (I
was actually doing =$PGPORT, and didn't realize $PGPORT was empty). What you
end up with is a compile error in guc.c, with no idea why it's broken. Any
reason not to have configure or at least make puke if pgport isn't valid?
That seems like a good idea.
I've been getting rejection to happen with phrases like
--with-pgport=${PGPORT:?}
which while it looks a little odd, only adds 4 characters to each
shell variable.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/5/16 10:08 AM, David Fetter wrote:
On Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote:
I just discovered that ./configure will happily accept '--with-pgport=' (I
was actually doing =$PGPORT, and didn't realize $PGPORT was empty). What you
end up with is a compile error in guc.c, with no idea why it's broken. Any
reason not to have configure or at least make puke if pgport isn't valid?That seems like a good idea.
Patch attached. I've verified it with --with-pgport=, =0, =77777 and =1.
It catches what you'd expect it to.
As the comment states, it doesn't catch things like --with-pgport=1a in
configure, but the compile error you get with that isn't too hard to
figure out, so I think it's OK.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
Attachments:
configure_port.patchtext/plain; charset=UTF-8; name=configure_port.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/configure b/configure
index b3f3abe..2beee31 100755
--- a/configure
+++ b/configure
@@ -3099,6 +3099,14 @@ cat >>confdefs.h <<_ACEOF
_ACEOF
+# It's worth testing for this because it creates a very confusing error
+if test "$default_port" == ""; then
+ as_fn_error $? "Invalid empty string supplied for \$PGPORT or --with-pgport" "$LINENO" 5
+# This won't catch something like "PGPORT=11a" but that produces a pretty easy
+# to understand compile error.
+elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then
+ as_fn_error $? "port must be between 1 and 65535" "$LINENO" 5
+fi
#
# '-rpath'-like feature can be disabled
diff --git a/configure.in b/configure.in
index 0bd90d7..54e9a16 100644
--- a/configure.in
+++ b/configure.in
@@ -164,6 +164,14 @@ but it's convenient if your clients have the right default compiled in.
AC_DEFINE_UNQUOTED(DEF_PGPORT_STR, "${default_port}",
[Define to the default TCP port number as a string constant.])
AC_SUBST(default_port)
+# It's worth testing for this because it creates a very confusing error
+if test "$default_port" == ""; then
+ AC_MSG_ERROR([Invalid empty string supplied for \$PGPORT or --with-pgport])
+# This won't catch something like "PGPORT=11a" but that produces a pretty easy
+# to understand compile error.
+elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then
+ AC_MSG_ERROR([port must be between 1 and 65535])
+fi
#
# '-rpath'-like feature can be disabled
Jim Nasby wrote:
On 2/5/16 10:08 AM, David Fetter wrote:
On Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote:
I just discovered that ./configure will happily accept '--with-pgport=' (I
was actually doing =$PGPORT, and didn't realize $PGPORT was empty). What you
end up with is a compile error in guc.c, with no idea why it's broken. Any
reason not to have configure or at least make puke if pgport isn't valid?That seems like a good idea.
Patch attached. I've verified it with --with-pgport=, =0, =77777 and =1. It
catches what you'd expect it to.
Does it work to specify port numbers below 1024?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/23/16 9:37 AM, Alvaro Herrera wrote:
Jim Nasby wrote:
On 2/5/16 10:08 AM, David Fetter wrote:
On Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote:
I just discovered that ./configure will happily accept '--with-pgport=' (I
was actually doing =$PGPORT, and didn't realize $PGPORT was empty). What you
end up with is a compile error in guc.c, with no idea why it's broken. Any
reason not to have configure or at least make puke if pgport isn't valid?That seems like a good idea.
Patch attached. I've verified it with --with-pgport=, =0, =77777 and =1. It
catches what you'd expect it to.Does it work to specify port numbers below 1024?
Presumably not if you're trying to open a network port. But I just
checked and if listen_addresses='' then you can use a low port number:
select name,quote_nullable(setting) from pg_settings where name in
('port','listen_addresses');
name | quote_nullable
------------------+----------------
listen_addresses | ''
port | '1'
(2 rows)
Plus, the GUC check allows 1-1024, so I'm inclined to do the same in the
config check. But I don't have a strong opinion about it.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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 Tue, Feb 23, 2016 at 04:09:00PM -0600, Jim Nasby wrote:
On 2/23/16 9:37 AM, Alvaro Herrera wrote:
Jim Nasby wrote:
On 2/5/16 10:08 AM, David Fetter wrote:
On Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote:
I just discovered that ./configure will happily accept '--with-pgport=' (I
was actually doing =$PGPORT, and didn't realize $PGPORT was empty). What you
end up with is a compile error in guc.c, with no idea why it's broken. Any
reason not to have configure or at least make puke if pgport isn't valid?That seems like a good idea.
Patch attached. I've verified it with --with-pgport=, =0, =77777 and =1. It
catches what you'd expect it to.Does it work to specify port numbers below 1024?
Presumably not if you're trying to open a network port. But I just checked
and if listen_addresses='' then you can use a low port number:select name,quote_nullable(setting) from pg_settings where name in
('port','listen_addresses');
name | quote_nullable
------------------+----------------
listen_addresses | ''
port | '1'
(2 rows)Plus, the GUC check allows 1-1024, so I'm inclined to do the same in the
config check. But I don't have a strong opinion about it.
I'm thinking that both the GUC check and the configure one should
restrict it to [1024..65535].
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Feb 24, 2016 at 4:01 AM, David Fetter <david@fetter.org> wrote:
I'm thinking that both the GUC check and the configure one should
restrict it to [1024..65535].
Doesn't sound like a good idea to me. If somebody has a reason they
want to do that, they shouldn't have to hack the source code and
recompile to make it work.
--
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 Fri, Feb 26, 2016 at 04:55:23PM +0530, Robert Haas wrote:
On Wed, Feb 24, 2016 at 4:01 AM, David Fetter <david@fetter.org> wrote:
I'm thinking that both the GUC check and the configure one should
restrict it to [1024..65535].Doesn't sound like a good idea to me. If somebody has a reason they
want to do that, they shouldn't have to hack the source code and
recompile to make it work.
I'm not sure I understand a use case here.
On *n*x, we already disallow running as root pretty aggressively,
using the "have to hack the source code and recompile" level of effort
you aptly described. This is just cleanup work on that project, as I
see it.
What am I missing?
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> writes:
On Fri, Feb 26, 2016 at 04:55:23PM +0530, Robert Haas wrote:
On Wed, Feb 24, 2016 at 4:01 AM, David Fetter <david@fetter.org> wrote:
I'm thinking that both the GUC check and the configure one should
restrict it to [1024..65535].
Doesn't sound like a good idea to me. If somebody has a reason they
want to do that, they shouldn't have to hack the source code and
recompile to make it work.
I'm not sure I understand a use case here.
On *n*x, we already disallow running as root pretty aggressively,
using the "have to hack the source code and recompile" level of effort
you aptly described. This is just cleanup work on that project, as I
see it.
What am I missing?
You're assuming that every system under the sun prevents non-root
processes from opening ports below 1024. I do not know if that's
true, and even if it is, it doesn't seem to me that it's our job
to enforce it. I agree with Robert --- restricting to [1,65535]
is plenty good enough.
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
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: tested, failed
Tested, I think it`s rather important to make cleanup work on that project.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/22/16 6:24 PM, Jim Nasby wrote:
On 2/5/16 10:08 AM, David Fetter wrote:
On Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote:
I just discovered that ./configure will happily accept
'--with-pgport=' (I
was actually doing =$PGPORT, and didn't realize $PGPORT was empty).
What you
end up with is a compile error in guc.c, with no idea why it's
broken. Any
reason not to have configure or at least make puke if pgport isn't
valid?That seems like a good idea.
Patch attached. I've verified it with --with-pgport=, =0, =77777 and =1.
It catches what you'd expect it to.
Your code and comments suggest that you can specify the port to
configure by setting PGPORT, but that is not the case.
test == is not portable (bashism).
Error messages should have consistent capitalization.
Indentation in configure is two spaces.
As the comment states, it doesn't catch things like --with-pgport=1a in
configure, but the compile error you get with that isn't too hard to
figure out, so I think it's OK.
Passing a non-integer as argument will produce an error message like
(depending on shell)
./configure: line 3107: test: 11a: integer expression expected
but will not actually abort configure.
It would work more robustly if you did something like this
elif test "$default_port" -ge "1" -a "$default_port" -le "65535"; then
:
else
AC_MSG_ERROR([port must be between 1 and 65535])
fi
but that still leaks the shell's error message.
There is also the risk of someone specifying a number with a leading
zero, which C would interpret as octal but the shell would not.
To make this really robust, you might need to do pattern matching on the
value.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2/26/16 9:34 AM, Ivan Kartyshov wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: tested, failedTested, I think it`s rather important to make cleanup work on that project.
Did you mean to mark all those items as tested, failed?
On another note, the other use case for allowing 1-1024 is if you run
with listen_address=''.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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 2/26/16 9:29 PM, Peter Eisentraut wrote:
To make this really robust, you might need to do pattern matching on the
value.
Yeah, and I don't see any reasonable way to do that... we don't require
sed or the like, do we?
I'll look at the other things you mentioned.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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 2016-02-27 14:15:45 -0600, Jim Nasby wrote:
Yeah, and I don't see any reasonable way to do that... we don't require sed
or the like, do we?
We actually do. Check the bottom of configure.in.
--
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, Feb 27, 2016 at 3:15 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 2/26/16 9:29 PM, Peter Eisentraut wrote:
To make this really robust, you might need to do pattern matching on the
value.Yeah, and I don't see any reasonable way to do that... we don't require sed
or the like, do we?I'll look at the other things you mentioned.
Jim, if you want this in 9.6, we need an update, like, RSN.
Otherwise, I'm going to mark it Returned with Feedback, and you can
resubmit for 9.7.
--
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 2/26/16 9:29 PM, Peter Eisentraut wrote:
Your code and comments suggest that you can specify the port to
configure by setting PGPORT, but that is not the case.test == is not portable (bashism).
Error messages should have consistent capitalization.
Indentation in configure is two spaces.
As the comment states, it doesn't catch things like --with-pgport=1a in
configure, but the compile error you get with that isn't too hard to
figure out, so I think it's OK.Passing a non-integer as argument will produce an error message like
(depending on shell)./configure: line 3107: test: 11a: integer expression expected
but will not actually abort configure.
It would work more robustly if you did something like this
elif test "$default_port" -ge "1" -a "$default_port" -le "65535"; then
:
else
AC_MSG_ERROR([port must be between 1 and 65535])
fibut that still leaks the shell's error message.
There is also the risk of someone specifying a number with a leading
zero, which C would interpret as octal but the shell would not.
All issues should now be addressed.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
Attachments:
configure_port-2.patchtext/plain; charset=UTF-8; name=configure_port-2.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/configure b/configure
index b3f3abe..e7bddba 100755
--- a/configure
+++ b/configure
@@ -3099,6 +3099,16 @@ cat >>confdefs.h <<_ACEOF
_ACEOF
+# It's worth testing for this because it creates a very confusing error
+if test "$default_port" = ""; then
+ as_fn_error $? "invalid empty string supplied with --with-pgport" "$LINENO" 5
+elif test ! `echo $default_port | sed -e 's/[0-9]//g'` = ''; then
+ as_fn_error $? "invalid port specification; must be a number" "$LINENO" 5
+elif test ! `echo $default_port | sed -e 's/^0//g'` = $default_port; then
+ as_fn_error $? "illegal leading 0 specified with --with-pgport" "$LINENO" 5
+elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then
+ as_fn_error $? "port must be between 1 and 65535" "$LINENO" 5
+fi
#
# '-rpath'-like feature can be disabled
diff --git a/configure.in b/configure.in
index 0bd90d7..db6e2a0 100644
--- a/configure.in
+++ b/configure.in
@@ -164,6 +164,16 @@ but it's convenient if your clients have the right default compiled in.
AC_DEFINE_UNQUOTED(DEF_PGPORT_STR, "${default_port}",
[Define to the default TCP port number as a string constant.])
AC_SUBST(default_port)
+# It's worth testing for this because it creates a very confusing error
+if test "$default_port" = ""; then
+ AC_MSG_ERROR([invalid empty string supplied with --with-pgport])
+elif test ! `echo $default_port | sed -e 's/[[0-9]]//g'` = ''; then
+ AC_MSG_ERROR([invalid port specification; must be a number])
+elif test ! `echo $default_port | sed -e 's/^0//g'` = $default_port; then
+ AC_MSG_ERROR([illegal leading 0 specified with --with-pgport])
+elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then
+ AC_MSG_ERROR([port must be between 1 and 65535])
+fi
#
# '-rpath'-like feature can be disabled
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Looks good to me. It only allows valid number between 1 and 65535, disallows leading zero, empty string, or non-digit chars. Error messages looks good.
Marking this Ready for Committer.
--
Alex
The new status of this patch is: Ready for Committer
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
All issues should now be addressed.
Pushed with some more tweaking: the test syntax wasn't terribly portable,
and the error messages weren't at all consistent.
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