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

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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

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

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

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

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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Feb 24, 2016 at 4:01 AM, David Fetter <david(at)fetter(dot)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

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(at)fetter(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david(at)fetter(dot)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(at)fetter(dot)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

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.

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.

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

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

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.

On Sat, Feb 27, 2016 at 3:15 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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

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

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

Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)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