From: Julien Rouhaud <rjuju123@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: t.larionov@postgrespro.ru
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16953: OOB access while converting "interval" to char
Date: Fri, 9 Apr 2021 18:42:46 +0800
Message-ID: <20210409104246.vkeef6rbyevq46f3@nol> (raw)
In-Reply-To: <YG7q8YcvFcXCAbAH@paquier.xyz>
References: <16953-f255a18f8c51f1d5@postgresql.org>
<20210407120856.3rjxxnnzzxn74vyv@nol>
<YG7YTjuZXylrjGAv@paquier.xyz>
<20210408110403.wr3d552xilqsx4us@nol>
<YG7q8YcvFcXCAbAH@paquier.xyz>
On Thu, Apr 08, 2021 at 08:37:21PM +0900, Michael Paquier wrote:
> On Thu, Apr 08, 2021 at 07:04:03PM +0800, Julien Rouhaud wrote:
> > I'm fine with it too, although I'd probably go with [-13, 13] just to make sure
> > that there's isn't silly off-by-one mistake.
>
> Also, I guess that you'd just want to compile twice a modulo based on
> MONTHS_PER_YEAR to get the correct positive position in each array.
I'm not sure what you mean by that. We receive a pg_tm struct which can't
contain more than 12 in tm_mon right? And actually for intervals it will
reduce "12 months" to "1 year 0 month", so to_char previously didn't report
anything for 12 months either.
>
> > I'll just wait a bit to see if anyone else has any opinion on whether -1 month
> > should be January or December.
>
> Sure. If you can send an updated patch, that would be great.
Hearing no other opinion I went with -1 -> december and so on in attached v2.
I also fixed the "[-]12 months" case and updated the regression tests. Given
the extra code needed to deduce the correct array position I factorized DCH_RM
and DCH_rm.
Attachments:v2-fix_rm.diff (text/plain, 3.4K) downloaddiff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.cindex 783c7b5e7a..80138d6347 100644--- a/src/backend/utils/adt/formatting.c+++ b/src/backend/utils/adt/formatting.c@@ -3207,18 +3207,36 @@ DCH_to_char(FormatNode *node, bool is_interval, TmToChar *in, char *out, Oid col
s += strlen(s);
break;
case DCH_RM:
- if (!tm->tm_mon)- break;- sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,- rm_months_upper[MONTHS_PER_YEAR - tm->tm_mon]);- s += strlen(s);- break;+ /* FALLTHROUGH */
case DCH_rm:
- if (!tm->tm_mon)+ /*+ * For intervals, values like '12 month' will be reduced to 0+ * month and some years, which we want to process.+ */+ if (!tm->tm_mon && !tm->tm_year)
break;
- sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,- rm_months_lower[MONTHS_PER_YEAR - tm->tm_mon]);- s += strlen(s);+ else+ {+ int mon = tm->tm_mon;+ const char * const* months;++ if (n->key->id == DCH_RM)+ months = rm_months_upper;+ else+ months = rm_months_lower;++ if (mon == 0)+ {+ Assert(is_interval);+ mon = tm->tm_year >= 0 ? MONTHS_PER_YEAR : 1;+ }+ else if (mon < 0)+ mon = MONTHS_PER_YEAR + mon + 1;++ sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,+ months[MONTHS_PER_YEAR - mon]);+ s += strlen(s);+ }
break;
case DCH_W:
sprintf(s, "%d", (tm->tm_mday - 1) / 7 + 1);
diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.outindex 690656dfb2..7c77fcae71 100644--- a/src/test/regress/expected/timestamp.out+++ b/src/test/regress/expected/timestamp.out@@ -1907,6 +1907,39 @@ SELECT to_char(d, 'FF1 FF2 FF3 FF4 FF5 FF6 ff1 ff2 ff3 ff4 ff5 ff6 MS US')
7 78 789 7890 78901 789012 7 78 789 7890 78901 789012 789 789012
(4 rows)
+SELECT i, to_char(i * interval '1mon', 'rm'), to_char(i * interval '1mon', 'RM')+ FROM generate_series(-13, 13) i;+ i | to_char | to_char +-----+---------+---------+ -13 | xii | XII + -12 | i | I + -11 | ii | II + -10 | iii | III + -9 | iv | IV + -8 | v | V + -7 | vi | VI + -6 | vii | VII + -5 | viii | VIII+ -4 | ix | IX + -3 | x | X + -2 | xi | XI + -1 | xii | XII + 0 | | + 1 | i | I + 2 | ii | II + 3 | iii | III + 4 | iv | IV + 5 | v | V + 6 | vi | VI + 7 | vii | VII + 8 | viii | VIII+ 9 | ix | IX + 10 | x | X + 11 | xi | XI + 12 | xii | XII + 13 | i | I +(27 rows)+
-- timestamp numeric fields constructor
SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887);
make_timestamp
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sqlindex c43a1f2268..3ae9a52347 100644--- a/src/test/regress/sql/timestamp.sql+++ b/src/test/regress/sql/timestamp.sql@@ -319,6 +319,9 @@ SELECT to_char(d, 'FF1 FF2 FF3 FF4 FF5 FF6 ff1 ff2 ff3 ff4 ff5 ff6 MS US')
('2018-11-02 12:34:56.78901234')
) d(d);
+SELECT i, to_char(i * interval '1mon', 'rm'), to_char(i * interval '1mon', 'RM')+ FROM generate_series(-13, 13) i;+
-- timestamp numeric fields constructor
SELECT make_timestamp(2014, 12, 28, 6, 30, 45.887);
SELECT make_timestamp(-44, 3, 15, 12, 30, 15);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: rjuju123@gmail.com, michael@paquier.xyz, t.larionov@postgrespro.ru, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16953: OOB access while converting "interval" to char
In-Reply-To: <20210409104246.vkeef6rbyevq46f3@nol>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox