Date: Fri, 9 Apr 2021 18:42:46 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: t.larionov@postgrespro.ru, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16953: OOB access while converting "interval" to char
Message-ID: <20210409104246.vkeef6rbyevq46f3@nol>
References: <16953-f255a18f8c51f1d5@postgresql.org>
 <20210407120856.3rjxxnnzzxn74vyv@nol>
 <YG7YTjuZXylrjGAv@paquier.xyz>
 <20210408110403.wr3d552xilqsx4us@nol>
 <YG7q8YcvFcXCAbAH@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="xgzd36pwpb7r5dov"
Content-Disposition: inline
In-Reply-To: <YG7q8YcvFcXCAbAH@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20210409104246.vkeef6rbyevq46f3%40nol>
Precedence: bulk


--xgzd36pwpb7r5dov
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--xgzd36pwpb7r5dov
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="v2-fix_rm.diff"

diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 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.out
index 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.sql
index 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);

--xgzd36pwpb7r5dov--