The attached patch fixes an overflow bug in DecodeInterval when applying
the units week, decade, century, and millennium. The overflow check logic
was modelled after the overflow check at the beginning of `int
tm2interval(struct pg_tm *tm, fsec_t fsec, Interval *span);` in timestamp.c.
This is my
Joseph Koshakow writes:
> [ v8-0001-Check-for-overflow-when-decoding-an-interval.patch ]
This isn't applying per the cfbot; looks like it got sideswiped
by 9e9858389. Here's a quick rebase. I've not reviewed it, but
I did notice (because git was in my face about this) that it's
got whitespace i
On Mon, Mar 21, 2022 at 8:31 PM Tom Lane wrote:
> This isn't applying per the cfbot; looks like it got sideswiped
> by 9e9858389. Here's a quick rebase. I've not reviewed it, but
> I did notice (because git was in my face about this) that it's
> got whitespace issues. Please try to avoid unnece
Joseph Koshakow writes:
> Sorry about that, I didn't have my IDE set up quite right and
> noticed a little too late that I had some auto-formatting turned
> on. Thanks for doing the rebase, did it end up fixing
> the whitespace issues? If not I'll go through the patch and try
> and fix them all.
Joseph Koshakow writes:
> * The existing code for rounding had a lot of int to double
> casting and vice versa. I *think* that doubles are able to completely
> represent the range of ints. However doubles are not able to represent
> the full range of int64. After making the change I started notici
I wrote:
> ... I almost feel that this is
> committable, but there is one thing that is bothering me. The
> part of DecodeInterval that does strange things with signs in the
> INTSTYLE_SQL_STANDARD case (starting about line 3400 in datetime.c
> before this patch, or line 3600 after) used to separa
On Fri, Apr 1, 2022 at 8:06 PM Tom Lane wrote:
>
> Joseph Koshakow writes:
> > * The existing code for rounding had a lot of int to double
> > casting and vice versa. I *think* that doubles are able to completely
> > represent the range of ints. However doubles are not able to represent
> > the f
On Sat, Apr 2, 2022 at 1:29 PM Joseph Koshakow wrote:
>
> On Fri, Apr 1, 2022 at 8:06 PM Tom Lane wrote:
> >
> > Joseph Koshakow writes:
> > > * The existing code for rounding had a lot of int to double
> > > casting and vice versa. I *think* that doubles are able to completely
> > > represent t
On Sat, Apr 2, 2022 at 2:22 PM Joseph Koshakow wrote:
>
> On Sat, Apr 2, 2022 at 1:29 PM Joseph Koshakow wrote:
> >
> > On Fri, Apr 1, 2022 at 8:06 PM Tom Lane wrote:
> > >
> > > Joseph Koshakow writes:
> > > > * The existing code for rounding had a lot of int to double
> > > > casting and vice
On Fri, Apr 1, 2022 at 8:06 PM Tom Lane wrote:
> I think the patch can be salvaged, though. I like the concept
> of converting all the sub-day fields to microseconds immediately,
> because it avoids a host of issues, so I don't want to give that up.
> What I'm going to look into is detecting the
Joseph Koshakow writes:
> Ok I actually remember now, the issue is with the rounding
> code in AdjustFractMicroseconds.
> ...
> I believe it's possible for `frac -= usec;` to result in a value greater
> than 1 or less than -1 due to the lossiness of int64 to double
> conversions.
I think it's not
Joseph Koshakow writes:
> I took a stab at this issue and the attached patch (which would be
> applied on top of your v10 patch) seems to fix the issue. Feel
> free to ignore it if you're already working on a fix.
You really only need to flip val/fval in one place. More to the
point, there's als
On Sat, Apr 2, 2022 at 3:08 PM Tom Lane wrote:
>
> Joseph Koshakow writes:
> > Ok I actually remember now, the issue is with the rounding
> > code in AdjustFractMicroseconds.
> > ...
> > I believe it's possible for `frac -= usec;` to result in a value greater
> > than 1 or less than -1 due to the
Joseph Koshakow writes:
> On Sat, Apr 2, 2022 at 3:08 PM Tom Lane wrote:
>> I think it's not, at least not for the interesting range of possible
>> values in this code. Given that abs(frac) < 1 to start with, the
>> abs value of usec can't exceed the value of scale, which is at most
>> USECS_PER
I wrote:
> Cool. I've pushed the patch.
Hmm ... buildfarm's not entirely happy [1][2][3]:
diff -U3
/home/nm/farm/gcc64/HEAD/pgsql.build/src/test/regress/expected/interval.out
/home/nm/farm/gcc64/HEAD/pgsql.build/src/test/regress/results/interval.out
--- /home/nm/farm/gcc64/HEAD/pgsql.build/src
On Sun, Apr 3, 2022 at 3:09 AM Tom Lane wrote:
>
> I wrote:
> > Cool. I've pushed the patch.
>
> Hmm ... buildfarm's not entirely happy [1][2][3]:
>
> diff -U3
> /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/regress/expected/interval.out
> /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/regress/r
Joseph Koshakow writes:
> On Sun, Apr 3, 2022 at 3:09 AM Tom Lane wrote:
>> Hmm ... buildfarm's not entirely happy [1][2][3]:
> I think I know that the issue is. It's with `ParseISO8601Number` and
> the minutes field "1.".
> Previously that function parsed the entire field into a single double,
I wrote:
> Joseph Koshakow writes:
>> I think I know that the issue is. It's with `ParseISO8601Number` and
>> the minutes field "1.".
>> Previously that function parsed the entire field into a single double,
>> so "1." would
>> be parsed into 1.0. Now we try to parse the integer and decimal parts
On Sun, Apr 3, 2022 at 12:03 PM Tom Lane wrote:
>
> I wrote:
> > Joseph Koshakow writes:
> >> I think I know that the issue is. It's with `ParseISO8601Number` and
> >> the minutes field "1.".
> >> Previously that function parsed the entire field into a single double,
> >> so "1." would
> >> be pa
Joseph Koshakow writes:
> On Sun, Apr 3, 2022 at 12:03 PM Tom Lane wrote:
>> Oh ... a bit of testing says that strtod() on an empty string
>> succeeds (returning zero) on Linux, but fails with EINVAL on
>> AIX. The latter is a lot less surprising than the former,
>> so we'd better cope.
> I'm n
On Sun, Apr 3, 2022 at 12:30 PM Tom Lane wrote:
>
> Joseph Koshakow writes:
> > So I think we need to check that endptr has moved both after
> > the call to strtoi64() and strtod().
>
> I'm not sure we need to do that explicitly, given that there's
> a check later as to whether endptr is pointing
On Sun, Apr 3, 2022 at 12:44 PM Joseph Koshakow wrote:
>
> On Sun, Apr 3, 2022 at 12:30 PM Tom Lane wrote:
> >
> > Joseph Koshakow writes:
> > > So I think we need to check that endptr has moved both after
> > > the call to strtoi64() and strtod().
> >
> > I'm not sure we need to do that explici
Joseph Koshakow writes:
> How does this patch look? I don't really have any way to test it on
> AIX.
That buildfarm machine is pretty slow, so I'm not in a hurry to test
it manually either. However, now that we realize the issue is about
whether strtod(".") produces EINVAL or not, I think we nee
On Sun, Apr 3, 2022 at 3:06 PM Tom Lane wrote:
> That buildfarm machine is pretty slow, so I'm not in a hurry to test
> it manually either. However, now that we realize the issue is about
> whether strtod(".") produces EINVAL or not, I think we need to fix
> all the places in datetime.c that are
Joseph Koshakow writes:
> The attached patch fixes an overflow bug in DecodeInterval when applying
> the units week, decade, century, and millennium. The overflow check logic
> was modelled after the overflow check at the beginning of `int
> tm2interval(struct pg_tm *tm, fsec_t fsec, Interval *spa
Tom Lane writes:
> Joseph Koshakow writes:
> > The attached patch fixes an overflow bug in DecodeInterval when applying
> > the units week, decade, century, and millennium. The overflow check logic
> > was modelled after the overflow check at the beginning of `int
> > tm2interval(struct pg_tm *tm
On Fri, Feb 11, 2022 at 4:58 PM Joseph Koshakow wrote:
>
> Tom Lane writes:
> > Joseph Koshakow writes:
> > > The attached patch fixes an overflow bug in DecodeInterval when applying
> > > the units week, decade, century, and millennium. The overflow check logic
> > > was modelled after the over
Hi,
On 2022-02-12 21:16:05 -0500, Joseph Koshakow wrote:
> I've attached the patch below.
Any reason for using int return types?
> +/* As above, but initial val produces years */
> +static int
> +AdjustYears(int val, struct pg_tm *tm, int multiplier)
> +{
> + int years;
On Sat, Feb 12, 2022 at 10:51 PM Andres Freund wrote:
> Any reason for using int return types?
>
> particularly since the pg_*_overflow stuff uses bool?
I chose int return types to keep all these methods
consistent with DecodeInterval, which returns a
non-zero int to indicate an error. Though I w
Actually found an additional overflow issue in
DecodeInterval. I've updated the patch with the
fix. Specifically it prevents trying to negate a field
if it equals INT_MIN. Let me know if this is best
put into another patch.
- Joe Koshakow
From 09eafa9b496c8461f2dc52ea62c9e833ab10a17f Mon Sep 17 00
Hi,
On 2022-02-13 09:35:47 -0500, Joseph Koshakow wrote:
> I chose int return types to keep all these methods
> consistent with DecodeInterval, which returns a
> non-zero int to indicate an error.
That's different, because it actually returns different types of errors. IMO
that difference is actu
Andres Freund writes:
> Do we want to consider backpatching these fixes?
I wasn't thinking that we should. It's a behavioral change and
people might not be pleased to have it appear in minor releases.
regards, tom lane
Attached is a new version switching from ints to bools, as requested.
- Joe Koshakow
From af8f030ad146602b4386f77b5664c6013743569b Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Fri, 11 Feb 2022 14:18:32 -0500
Subject: [PATCH] Check for overflow when decoding an interval
When decoding an i
On Sun, Feb 13, 2022 at 5:12 PM Joseph Koshakow wrote:
>
> Attached is a new version switching from ints to bools, as requested.
>
> - Joe Koshakow
Tom, Andres,
Thanks for your feedback! I'm not super familiar with the process,
should I submit this thread to the commitfest or just leave it as is
Hi,
On 2022-02-15 06:44:40 -0500, Joseph Koshakow wrote:
> Thanks for your feedback! I'm not super familiar with the process,
> should I submit this thread to the commitfest or just leave it as is?
Submit it to the CF - then we get an automatic test on a few platforms. I
think we can apply it qui
Ok, so I've attached a patch with my final unprompted changes. It
contains the following two changes:
1. I found some more overflows with the ISO8601 formats and have
included some fixes.
2. I reverted the overflow checks for the seconds field. It's actually a
bit more complicated than I thought.
Copying over a related thread here to have info all in one place.
It's a little fragmented so sorry about that.
On Fri, Feb 18, 2022 at 11:44 PM Tom Lane wrote:
>
> Joseph Koshakow writes:
> > When formatting the output of an Interval, we call abs() on the hours
> > field of the Interval. Calli
Attached is a patch of my first pass. The to_char method isn't finished
and I need to add a bunch of tests, but everything else is in place. It
ended up being a fairly large change in case anyone wants to take a look
the changes so far.
One thing I noticed is that interval.c has a ton of code copi
Joseph Koshakow writes:
> Attached is a patch of my first pass. The to_char method isn't finished
> and I need to add a bunch of tests, but everything else is in place. It
> ended up being a fairly large change in case anyone wants to take a look
> the changes so far.
Couple of quick comments:
*
On Sun, Feb 20, 2022 at 6:37 PM Tom Lane wrote:
> Couple of quick comments:
Thanks for the comments Tom, I'll work on fixing these and submit a
new patch.
> * The uses of tm2itm make me a bit itchy. Is that sweeping
> upstream-of-there overflow problems under the rug?
I agree, I'm not super ha
Hi All,
Sorry for the delay in the new patch, I've attached my most recent
patch to this email. I ended up reworking a good portion of my previous
patch so below I've included some reasons why, notes on my current
approach, and some pro/cons to the approach.
* The main reason for the rework had t
I just realized another issue today. It may have been obvious from one
of Tom's earlier messages, but I'm just now putting the pieces
together.
On Fri, Feb 18, 2022 at 11:44 PM Tom Lane wrote:
> Also, I notice that there's an overflow hazard upstream of here,
> in interval2tm:
>
> regression=# sel
42 matches
Mail list logo
