BUG #16953: OOB access while converting "interval" to char

Hi,

On Wed, Apr 07, 2021 at 09:09:25AM +0000, PG Bug reporting form wrote:

The following bug has been logged on the website:

Bug reference: 16953
Logged by: Theodor Arsenij Larionov-Trichkin
Email address: t.larionov@postgrespro.ru
PostgreSQL version: 13.2
Operating system: Ubuntu 20.04.2 LTS
Description:

9. Performing this query will result in OOB access of rm_months_lower array
and as a result crash: SELECT * from TO_CHAR(interval '-1Mon', 'rm');

Output:
[...]
terminated by signal 11: Segmentation fault
2021-04-07 12:08:01.013 MSK [33887] DETAIL: Failed process was running:
SELECT * from TO_CHAR(interval '-1Mon', 'rm');

Indeed, thanks a lot for the report!

It's because rm/RM are computed in a way that doesn't play nice with negative
values:

sprintf(s, "%*s", S_FM(n->suffix) ? 0 : -4,
rm_months_lower[MONTHS_PER_YEAR - tm->tm_mon]);

PFA a naive patch to fix this problem with some regression tests. I'm assuming
that -1 month should be january and not december. I had a quick look at the
rest of formatting.c and didn't spot any similar problem, but another pair of
eyes wouldn't hurt.

On Wed, Apr 07, 2021 at 08:08:56PM +0800, Julien Rouhaud wrote:

PFA a naive patch to fix this problem with some regression tests. I'm assuming
that -1 month should be january and not december. I had a quick look at the
rest of formatting.c and didn't spot any similar problem, but another pair of
eyes wouldn't hurt.

Assuming an absolute number is not really intuitive when it comes to a
negative number here, while counting backward feels more natural, so I
would vote for making -1 be December, -2 November, etc.

Let's also make the tests more extended. I would suggest a single
query with generate_series() from say -12 to 12, that checks the
output of both RM and rm for the full range of values supported by
tm_mon.
--
Michael

On Thu, Apr 08, 2021 at 07:17:50PM +0900, Michael Paquier wrote:

On Wed, Apr 07, 2021 at 08:08:56PM +0800, Julien Rouhaud wrote:

PFA a naive patch to fix this problem with some regression tests. I'm assuming
that -1 month should be january and not december. I had a quick look at the
rest of formatting.c and didn't spot any similar problem, but another pair of
eyes wouldn't hurt.

Assuming an absolute number is not really intuitive when it comes to a
negative number here, while counting backward feels more natural, so I
would vote for making -1 be December, -2 November, etc.

Honestly I might as well have flipped a coin here, so I'm fine either way.

Let's also make the tests more extended. I would suggest a single
query with generate_series() from say -12 to 12, that checks the
output of both RM and rm for the full range of values supported by
tm_mon.

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.

I'll just wait a bit to see if anyone else has any opinion on whether -1 month
should be January or December.

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'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.
--
Michael

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.

On Fri, Apr 09, 2021 at 06:42:46PM +0800, Julien Rouhaud wrote:

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 did not take the time to look in details, but for reference I just
imagined that a formula like this one would give pretty much the
position in rm_months_upper:
M_PER_Y - ((tm_mon % M_PER_Y) + M_PER_Y) % M_PER_Y

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.

Yep. The regression tests show what I would expect. I'll check in
details later.
--
Michael

On Fri, Apr 09, 2021 at 07:58:51PM +0900, Michael Paquier wrote:

Yep. The regression tests show what I would expect. I'll check in
details later.

I have spent some time on that today and applied this patch down to
9.6, after adding more comments and simplifying a bit the calculation
method used. Instead of using the "mon" variable to store an
intermediate result, I have simplified things so as this only uses
tm->tm_mon, and "mon" for the position in the roman-numeral array is
adjusted only once.
--
Michael

On Mon, Apr 12, 2021 at 12:04:23PM +0900, Michael Paquier wrote:

On Fri, Apr 09, 2021 at 07:58:51PM +0900, Michael Paquier wrote:

Yep. The regression tests show what I would expect. I'll check in
details later.

I have spent some time on that today and applied this patch down to
9.6, after adding more comments and simplifying a bit the calculation
method used. Instead of using the "mon" variable to store an
intermediate result, I have simplified things so as this only uses
tm->tm_mon, and "mon" for the position in the roman-numeral array is
adjusted only once.

Thanks Michael! I thought we'd want to keep the original calculation for
consistency, but directly computing the needed offset is clearly simpler to
read!