Horton

Thread

Commits

  1. Import changes from IMath versions (1.3, 1.29].

  2. Cherry-pick security-relevant fixes from upstream imath library.

  1. Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-03T03:57:04Z

    pgcrypto bundles a copy of the imath library for arbitrary-precision integer
arithmetic in non-SSL builds.  Upstream fixed buffer overflows through the
years, and commit 8b59672 brought those fixes into PostgreSQL.  In master, I
would like to fully resynchronize with fresh imath 1.29.  We're better off
naively tracking upstream than reactively maintaining a twelve-year-old
snapshot of upstream.

imath1.29-raw-sync-v1.patch is the result of copying new imath.c and imath.h
into place, removing "#ifdef __cplusplus" blocks that upset pgindent, running
pgindent, and filtering through "unexpand -t4 --first-only".
imath1.29-pgedits-v1.patch then restores PostgreSQL-specific changes.  I would
squash these together for eventual commit, since the intermediate state is
broken, but it should ease review to see them separately.  I did not keep the
INVERT_COMPARE_RESULT() change from c87cb5f; the domain of the comparisons in
question is {-1,0,1}, controlled entirely by code in imath.c.

Upstream has fixed bugs over the years, but I am not specifically aware of any
represented fix here that affects pgcrypto.  Most suspicious to me are the
division fixes, which could affect our pgp_elgamal_{en,de}crypt().  You can
examine https://github.com/creachadair/imath/blob/master/ChangeLog for all
changes between imath-1.3 and imath-1.29.

Like PostgreSQL, imath now assumes C99.  Unlike PostgreSQL, it has adopted
mixed declarations and code; our -Wdeclaration-after-statement would add
sixty-two warnings.  If the compiler supports -Wdeclaration-after-statement, I
add -Wno-declaration-after-statement for imath.c.

Thanks,
nm

  2. Re: Synchronize with imath upstream

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-02-03T06:01:51Z

    >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:

 Noah> If the compiler supports -Wdeclaration-after-statement, I add
 Noah> -Wno-declaration-after-statement for imath.c.

I found it much simpler to strip out -Wdeclaration-after-statement
instead:

$(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS))

-- 
Andrew (irc:RhodiumToad)

  3. Re: Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-03T06:20:48Z

    On Sun, Feb 03, 2019 at 06:01:51AM +0000, Andrew Gierth wrote:
> >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:
> 
>  Noah> If the compiler supports -Wdeclaration-after-statement, I add
>  Noah> -Wno-declaration-after-statement for imath.c.
> 
> I found it much simpler to strip out -Wdeclaration-after-statement
> instead:
> 
> $(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS))

The -Wno-declaration-after-statement approach takes eight lines of code, and
the filter-out approach takes one.  On the other hand, using $(filter-out)
changes any runs of whitespace to single spaces ("$(filter-out foo,a    b c)"
yields "a b c").  We do risk that with CPPFLAGS and LDFLAGS in a few places.
I don't want to proliferate that practice, because it changes semantics of
CFLAGS containing -DFOO="arbitrary    text".

  4. Re: Synchronize with imath upstream

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-02-03T07:07:36Z

    >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:

 >> I found it much simpler to strip out -Wdeclaration-after-statement
 >> instead:
 >> 
 >> $(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS))

 Noah> The -Wno-declaration-after-statement approach takes eight lines
 Noah> of code, and the filter-out approach takes one. On the other
 Noah> hand, using $(filter-out) changes any runs of whitespace to
 Noah> single spaces ("$(filter-out foo,a b c)" yields "a b c"). We do
 Noah> risk that with CPPFLAGS and LDFLAGS in a few places. I don't want
 Noah> to proliferate that practice, because it changes semantics of
 Noah> CFLAGS containing -DFOO="arbitrary text".

In that case I propose that we avoid the whole issue by removing
-Wdeclaration-after-statement entirely.

So far, expressed opinions have run about 4:2 in favour of allowing
mixed declarations and code.

-- 
Andrew (irc:RhodiumToad)

  5. Re: Synchronize with imath upstream

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-02-03T07:14:13Z

    >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:

 Noah> -  # -Wdeclaration-after-statement isn't applicable for C++
 Noah> +  # -Wdeclaration-after-statement isn't applicable for C++.  Specific C files
 Noah> +  # disable it, so AC_SUBST the negative form.
 Noah> +  PERMIT_DECLARATION_AFTER_STATEMENT=
 Noah> +  if test x"$save_CFLAGS" != "$CFLAGS"; then

Missing "x" here?

+  if test x"$save_CFLAGS" != x"$CFLAGS"; then

-- 
Andrew (irc:RhodiumToad)

  6. Re: Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-03T08:11:24Z

    On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote:
> In that case I propose that we avoid the whole issue by removing
> -Wdeclaration-after-statement entirely.

Some folks who skip $SUBJECT will be interested in your proposal.  If you wish
to pursue that proposal, please start a new thread.

On Sun, Feb 03, 2019 at 07:14:13AM +0000, Andrew Gierth wrote:
> >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:
> 
>  Noah> -  # -Wdeclaration-after-statement isn't applicable for C++
>  Noah> +  # -Wdeclaration-after-statement isn't applicable for C++.  Specific C files
>  Noah> +  # disable it, so AC_SUBST the negative form.
>  Noah> +  PERMIT_DECLARATION_AFTER_STATEMENT=
>  Noah> +  if test x"$save_CFLAGS" != "$CFLAGS"; then
> 
> Missing "x" here?
> 
> +  if test x"$save_CFLAGS" != x"$CFLAGS"; then

I'll incorporate your fix.  Thanks.

  7. Re: Synchronize with imath upstream

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-03T15:31:26Z

    Noah Misch <noah@leadboat.com> writes:
> The -Wno-declaration-after-statement approach takes eight lines of code, and
> the filter-out approach takes one.  On the other hand, using $(filter-out)
> changes any runs of whitespace to single spaces ("$(filter-out foo,a    b c)"
> yields "a b c").  We do risk that with CPPFLAGS and LDFLAGS in a few places.
> I don't want to proliferate that practice, because it changes semantics of
> CFLAGS containing -DFOO="arbitrary    text".

I don't particularly buy that argument, because CPPFLAGS is where any -D
switches ought to be put.  So we've already exposed ourselves to this
risk, in the unlikely scenario where it's not hypothetical.

			regards, tom lane

  8. Re: Synchronize with imath upstream

    David Fetter <david@fetter.org> — 2019-02-03T17:36:50Z

    On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote:
> >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:
> 
>  >> I found it much simpler to strip out -Wdeclaration-after-statement
>  >> instead:
>  >> 
>  >> $(RYU_OBJS): override CFLAGS := $(filter-out -Wdeclaration-after-statement,$(CFLAGS))
> 
>  Noah> The -Wno-declaration-after-statement approach takes eight lines
>  Noah> of code, and the filter-out approach takes one. On the other
>  Noah> hand, using $(filter-out) changes any runs of whitespace to
>  Noah> single spaces ("$(filter-out foo,a b c)" yields "a b c"). We do
>  Noah> risk that with CPPFLAGS and LDFLAGS in a few places. I don't want
>  Noah> to proliferate that practice, because it changes semantics of
>  Noah> CFLAGS containing -DFOO="arbitrary text".
> 
> In that case I propose that we avoid the whole issue by removing
> -Wdeclaration-after-statement entirely.
> 
> So far, expressed opinions have run about 4:2 in favour of allowing
> mixed declarations and code.

+1 for this.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

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

  9. Re: Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-04T03:12:17Z

    On Sun, Feb 03, 2019 at 10:31:26AM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > The -Wno-declaration-after-statement approach takes eight lines of code, and
> > the filter-out approach takes one.  On the other hand, using $(filter-out)
> > changes any runs of whitespace to single spaces ("$(filter-out foo,a    b c)"
> > yields "a b c").  We do risk that with CPPFLAGS and LDFLAGS in a few places.
> > I don't want to proliferate that practice, because it changes semantics of
> > CFLAGS containing -DFOO="arbitrary    text".
> 
> I don't particularly buy that argument, because CPPFLAGS is where any -D
> switches ought to be put.  So we've already exposed ourselves to this
> risk, in the unlikely scenario where it's not hypothetical.

The $(filter-out) corruption is unlikely to matter, indeed.  The question is
whether to use eight lines of code to inject -Wno-declaration-after-statement
or one line to remove -Wdeclaration-after-statement using $(filter-out).  I
see negligible drawbacks on either side; both approaches are tolerable.  The
above-described hypothetical problem tips the scale in favor of
-Wno-declaration-after-statement.

  10. Re: Synchronize with imath upstream

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-05T23:47:50Z

    On 2019-Feb-03, David Fetter wrote:

> On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote:
> > >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:

> > So far, expressed opinions have run about 4:2 in favour of allowing
> > mixed declarations and code.
> 
> +1 for this.

Just curious, why do you care?  If you want to use such constructs in
extension code, you can add it in your makefiles.

I'm -1 for this myself.  I think there are a few places that could
benefit from it, but my fear is that many *more* places would get worse.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: Synchronize with imath upstream

    Andres Freund <andres@anarazel.de> — 2019-02-06T14:29:15Z

    
On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>On 2019-Feb-03, David Fetter wrote:
>
>> On Sun, Feb 03, 2019 at 07:07:36AM +0000, Andrew Gierth wrote:
>> > >>>>> "Noah" == Noah Misch <noah@leadboat.com> writes:
>
>> > So far, expressed opinions have run about 4:2 in favour of allowing
>> > mixed declarations and code.
>> 
>> +1 for this.
>
>Just curious, why do you care?  If you want to use such constructs in
>extension code, you can add it in your makefiles.
>
>I'm -1 for this myself.  I think there are a few places that could
>benefit from it, but my fear is that many *more* places would get
>worse.

Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  12. Re: Synchronize with imath upstream

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-06T15:15:24Z

    Andres Freund <andres@anarazel.de> writes:
> On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> I'm -1 for this myself.  I think there are a few places that could
>> benefit from it, but my fear is that many *more* places would get
>> worse.

> Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously.

I don't object to keeping imported code in a form that matches upstream
as best we can.  (Should we also exclude such files from pgindent'ing?)

But changing conventions for our own code is an entirely different matter.
In this case, I think that having some places use it while the bulk of
the code doesn't is just a bad idea from a stylistic-consistency
standpoint.  It's pretty much the same reason why we still aren't allowing
// comments --- there's no toolchain-based reason not to, but a mishmash of
comment styles would be ugly and hard to read.

			regards, tom lane

  13. Re: Synchronize with imath upstream

    Andres Freund <andres@anarazel.de> — 2019-02-06T16:49:19Z

    Hi,

On 2019-02-06 10:15:24 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> >> I'm -1 for this myself.  I think there are a few places that could
> >> benefit from it, but my fear is that many *more* places would get
> >> worse.
> 
> > Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously.
> 
> I don't object to keeping imported code in a form that matches upstream
> as best we can.  (Should we also exclude such files from pgindent'ing?)

I think pgindenting doesn't matter as much, as that's an automated
change. But as pgindent doesn't fix the position of declarations, that's
a significant manual process.


> But changing conventions for our own code is an entirely different matter.
> In this case, I think that having some places use it while the bulk of
> the code doesn't is just a bad idea from a stylistic-consistency
> standpoint.  It's pretty much the same reason why we still aren't allowing
> // comments --- there's no toolchain-based reason not to, but a mishmash of
> comment styles would be ugly and hard to read.

Consistency surely has value, but it shouldn't block making use of new
things that become available.  I don't feel extremely strongly about
allowing declarations after statements in C (while it's obviously
crucial for C++), but I do think it can be a noticably easier to reason
about the values of variables if they're declared closer to use, and
it's easier to make sure that a variable hasn't been used elsewhere that
way.

Greetings,

Andres Freund

  14. Re: Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-07T05:50:11Z

    On Wed, Feb 06, 2019 at 10:15:24AM -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On February 6, 2019 5:17:50 AM GMT+05:30, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> >> I'm -1 for this myself.  I think there are a few places that could
> >> benefit from it, but my fear is that many *more* places would get
> >> worse.
> 
> > Because of imported code like ryu and imath? And because it can make code considerably better when used judiciously.
> 
> I don't object to keeping imported code in a form that matches upstream
> as best we can.  (Should we also exclude such files from pgindent'ing?)

I think it depends on how much time one spends merging upstream changes versus
making PostgreSQL-specific edits.  For IMath, both amounts are too small to
get excited about.  Does pgindent materially complicate src/timezone merges?

> But changing conventions for our own code is an entirely different matter.
> In this case, I think that having some places use it while the bulk of
> the code doesn't is just a bad idea from a stylistic-consistency
> standpoint.  It's pretty much the same reason why we still aren't allowing
> // comments --- there's no toolchain-based reason not to, but a mishmash of
> comment styles would be ugly and hard to read.

(This debate never belonged in this thread, but it's too late now.)  I find
code easiest to follow when the declaration appears as late as possible.  I
would welcome mixed declarations and code, and I would not mourn the loss of
consistency.  In terms of consistency damage, this is similar to adding
psprintf() without exterminating palloc()+snprintf().  I'm glad we introduce
ways to write new, cleaner code despite the inconsistency with older code.

  15. Re: Synchronize with imath upstream

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-07T15:12:05Z

    Noah Misch <noah@leadboat.com> writes:
> On Wed, Feb 06, 2019 at 10:15:24AM -0500, Tom Lane wrote:
>> I don't object to keeping imported code in a form that matches upstream
>> as best we can.  (Should we also exclude such files from pgindent'ing?)

> I think it depends on how much time one spends merging upstream changes versus
> making PostgreSQL-specific edits.  For IMath, both amounts are too small to
> get excited about.  Does pgindent materially complicate src/timezone merges?

My practice with src/timezone is to pgindent the upstream code and then
diff it; given that extra step, it's not really any more complex (and
maybe less so, as this hides minor whitespace changes for instance).
There are some other deltas to worry about as well, see
src/timezone/README.

I have no particular opinion on whether pgindent should be part of the
mix for imath, but I do strongly recommend setting up and documenting a
reproducible import process, as I did for src/timezone.

			regards, tom lane

  16. Re: Synchronize with imath upstream

    Daniel Gustafsson <daniel@yesql.se> — 2019-02-07T20:23:43Z

    > On 7 Feb 2019, at 16:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> .. I do strongly recommend setting up and documenting a
> reproducible import process, as I did for src/timezone.

Strong +1 on this.

cheers ./daniel

  17. Re: Synchronize with imath upstream

    Michael Paquier <michael@paquier.xyz> — 2019-02-08T00:23:42Z

    On Thu, Feb 07, 2019 at 09:23:43PM +0100, Daniel Gustafsson wrote:
> On 7 Feb 2019, at 16:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> .. I do strongly recommend setting up and documenting a
>> reproducible import process, as I did for src/timezone.
> 
> Strong +1 on this.

+1.
--
Michael

  18. Re: Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-08T04:38:44Z

    On Thu, Feb 07, 2019 at 10:12:05AM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Wed, Feb 06, 2019 at 10:15:24AM -0500, Tom Lane wrote:
> >> I don't object to keeping imported code in a form that matches upstream
> >> as best we can.  (Should we also exclude such files from pgindent'ing?)
> 
> > I think it depends on how much time one spends merging upstream changes versus
> > making PostgreSQL-specific edits.  For IMath, both amounts are too small to
> > get excited about.  Does pgindent materially complicate src/timezone merges?
> 
> My practice with src/timezone is to pgindent the upstream code and then
> diff it; given that extra step, it's not really any more complex (and
> maybe less so, as this hides minor whitespace changes for instance).

Let's continue to pgindent src/timezone and IMath, then.

> There are some other deltas to worry about as well, see
> src/timezone/README.
> 
> I have no particular opinion on whether pgindent should be part of the
> mix for imath, but I do strongly recommend setting up and documenting a
> reproducible import process, as I did for src/timezone.

Good idea.  I've modified the imath.c header comment to take the form of an
import process; see attached imath1.29-pgedits-v2.patch.

  19. Re: Synchronize with imath upstream

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-08T04:56:18Z

    Noah Misch <noah@leadboat.com> writes:
> On Thu, Feb 07, 2019 at 10:12:05AM -0500, Tom Lane wrote:
>> I have no particular opinion on whether pgindent should be part of the
>> mix for imath, but I do strongly recommend setting up and documenting a
>> reproducible import process, as I did for src/timezone.

> Good idea.  I've modified the imath.c header comment to take the form of an
> import process; see attached imath1.29-pgedits-v2.patch.

Maybe easier to keep the instructions in a separate README file?
(I don't see a need to put a PG copyright in this file, when the
mods from upstream are this small.)

Looks good otherwise.

			regards, tom lane

  20. Re: Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-16T19:10:21Z

    On Thu, Feb 07, 2019 at 11:56:18PM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Thu, Feb 07, 2019 at 10:12:05AM -0500, Tom Lane wrote:
> >> I have no particular opinion on whether pgindent should be part of the
> >> mix for imath, but I do strongly recommend setting up and documenting a
> >> reproducible import process, as I did for src/timezone.
> 
> > Good idea.  I've modified the imath.c header comment to take the form of an
> > import process; see attached imath1.29-pgedits-v2.patch.
> 
> Maybe easier to keep the instructions in a separate README file?

Most imath.c patches have cause to update those lines, and patches to other
files almost never do.  Hence, I think hackers are more likely to find and
update those lines in imath.c.  I would choose a README if concerns weren't
concentrated in one file.

> (I don't see a need to put a PG copyright in this file, when the
> mods from upstream are this small.)

The tree has been inconsistent about that, which is okay.  I was mimicking
src/port/strlcpy.c.  Others, e.g. src/port/crypt.c, haven't added a copyright.

  21. Re: Synchronize with imath upstream

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-16T19:12:50Z

    Noah Misch <noah@leadboat.com> writes:
> On Thu, Feb 07, 2019 at 11:56:18PM -0500, Tom Lane wrote:
>> Maybe easier to keep the instructions in a separate README file?

> Most imath.c patches have cause to update those lines, and patches to other
> files almost never do.  Hence, I think hackers are more likely to find and
> update those lines in imath.c.  I would choose a README if concerns weren't
> concentrated in one file.

Fair enough, objection withdrawn.

			regards, tom lane

  22. Re: Synchronize with imath upstream

    Noah Misch <noah@leadboat.com> — 2019-02-16T21:40:23Z

    On Sat, Feb 16, 2019 at 02:12:50PM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Thu, Feb 07, 2019 at 11:56:18PM -0500, Tom Lane wrote:
> >> Maybe easier to keep the instructions in a separate README file?
> 
> > Most imath.c patches have cause to update those lines, and patches to other
> > files almost never do.  Hence, I think hackers are more likely to find and
> > update those lines in imath.c.  I would choose a README if concerns weren't
> > concentrated in one file.
> 
> Fair enough, objection withdrawn.

Pushed, but it broke dory.  Will fix.