Re: [HACKERS] Sanity checking for ./configure options?

2016-03-14 Thread Tom Lane 
Jim Nasby  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

Re: [HACKERS] Sanity checking for ./configure options?

2016-03-14 Thread Alex Shulgin 
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

Re: [HACKERS] Sanity checking for ./configure options?

2016-03-13 Thread Jim Nasby 

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])
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.


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
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

-- 
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] Sanity checking for ./configure options?

2016-03-11 Thread Robert Haas 
On Sat, Feb 27, 2016 at 3:15 PM, Jim Nasby  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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-27 Thread Andres Freund 
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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-27 Thread Jim Nasby 

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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-27 Thread Jim Nasby 

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, failed

Tested, 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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-26 Thread Peter Eisentraut 
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, =7 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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-26 Thread Ivan Kartyshov 
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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-26 Thread Tom Lane 
David Fetter  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  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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-26 Thread David Fetter 
On Fri, Feb 26, 2016 at 04:55:23PM +0530, Robert Haas wrote:
> On Wed, Feb 24, 2016 at 4:01 AM, David Fetter  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  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-26 Thread Robert Haas 
On Wed, Feb 24, 2016 at 4:01 AM, David Fetter  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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-23 Thread David Fetter 
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, =7 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  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-23 Thread Jim Nasby 

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, =7 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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-23 Thread Alvaro Herrera 
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, =7 and =1. It
> catches what you'd expect it to.

Does it work to specify port numbers below 1024?

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] Sanity checking for ./configure options?

2016-02-22 Thread Jim Nasby 

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, =7 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
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

-- 
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] Sanity checking for ./configure options?

2016-02-05 Thread David Fetter 
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  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@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

[HACKERS] Sanity checking for ./configure options?

2016-02-03 Thread Jim Nasby 
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