Обсуждение: 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@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
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@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
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
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
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
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.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
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.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