Re: [HACKERS] Minor binary-search int overflow in timezone code
Re: Tom Lane 2014-12-16 <14615.1418694...@sss.pgh.pa.us> > Jim Nasby writes: > > On 12/15/14, 1:39 PM, Christoph Berg wrote: > >> Well, if it's not interesting, let's just forget it. Sorry. > > > At the risk of sticking my head in the lions mouth... this is the kind of > > response that deters people from contributing anything to the project, > > including reviewing patches. A simple "thanks, but we feel it's already > > clear enough that there can't be anywhere close to INT_MAX timezones" would > > have sufficed. > > Yeah, I need to apologize. I was a bit on edge today due to the release > wrap (which you may have noticed wasn't going too smoothly), and should > not have responded like that. Hi, maybe I should apologize as well for submitting this right at the time of the release... > I also remain curious as to what sort of tool would complain about this > particular code and not the N other nearly-identical binary-search loops > in the PG sources, most of which deal with data structures potentially > far larger than the timezone data ... He said he found it in manual code review, not using a tool. But anyway, I do agree this is a very minor issue and there's much more interesting things to spend time on. I promise to send in more severe security issues next time :) Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor binary-search int overflow in timezone code
Jim Nasby writes: > On 12/15/14, 1:39 PM, Christoph Berg wrote: >> Well, if it's not interesting, let's just forget it. Sorry. > At the risk of sticking my head in the lions mouth... this is the kind of > response that deters people from contributing anything to the project, > including reviewing patches. A simple "thanks, but we feel it's already clear > enough that there can't be anywhere close to INT_MAX timezones" would have > sufficed. Yeah, I need to apologize. I was a bit on edge today due to the release wrap (which you may have noticed wasn't going too smoothly), and should not have responded like that. Having said that, though, the submission wasn't carefully thought through either. That problem was either not-an-issue or a potential security bug, and if the submitter hadn't taken the time to be sure which, reporting it in a public forum wasn't the way to proceed. I also remain curious as to what sort of tool would complain about this particular code and not the N other nearly-identical binary-search loops in the PG sources, most of which deal with data structures potentially far larger than the timezone data ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor binary-search int overflow in timezone code
On 12/15/14, 1:39 PM, Christoph Berg wrote: Re: Tom Lane 2014-12-15 <21813.1418655...@sss.pgh.pa.us> This is totally silly. The timecnt couldn't be anywhere near INT_MAX (in fact, it is not allowed to exceed TZ_MAX_TIMES, which is currently just 1200). And there are bunches of other instances of similar code in PG; shall we put equally wishy-washy comments on them all? Well, if it's not interesting, let's just forget it. Sorry. At the risk of sticking my head in the lions mouth... this is the kind of response that deters people from contributing anything to the project, including reviewing patches. A simple "thanks, but we feel it's already clear enough that there can't be anywhere close to INT_MAX timezones" would have sufficed. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor binary-search int overflow in timezone code
Re: Tom Lane 2014-12-15 <21813.1418655...@sss.pgh.pa.us> > This is totally silly. The timecnt couldn't be anywhere near INT_MAX (in > fact, it is not allowed to exceed TZ_MAX_TIMES, which is currently just > 1200). And there are bunches of other instances of similar code in PG; > shall we put equally wishy-washy comments on them all? Well, if it's not interesting, let's just forget it. Sorry. Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor binary-search int overflow in timezone code
Christoph Berg writes: > a fellow Debian Developer found a minor glitch in > src/timezone/localtime.c, where binary search is used. Now I don't > think there is an actual problem (unless there's > 2^30 timezones), > but it would at least make sense to mark the code as okayish so that > people running code scanners won't stumble over the issue again. > The attached patch added comments to address this. This is totally silly. The timecnt couldn't be anywhere near INT_MAX (in fact, it is not allowed to exceed TZ_MAX_TIMES, which is currently just 1200). And there are bunches of other instances of similar code in PG; shall we put equally wishy-washy comments on them all? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Minor binary-search int overflow in timezone code
Hi,
a fellow Debian Developer found a minor glitch in
src/timezone/localtime.c, where binary search is used. Now I don't
think there is an actual problem (unless there's > 2^30 timezones),
but it would at least make sense to mark the code as okayish so that
people running code scanners won't stumble over the issue again.
The attached patch added comments to address this.
Date: Sun, 30 Nov 2014 22:06:42 +0100
From: Niels Thykier
Reply-To: Niels Thykier , 771...@bugs.debian.org
To: Debian Bug Tracking System
Subject: [Pkg-postgresql-public] Bug#771580: postgresql-9.4: Minor binary-search
int overflow
Source: postgresql-9.4
Version: 9.4~rc1-1
Severity: minor
Hi,
I stumbled on the folowing snippet from src/timezone/localtime.c,
function pg_interpret_timezone_abbrev:
{
int lo = 0;
int hi = sp->timecnt;
while (lo < hi)
{
int mid = (lo + hi) >> 1;
^^^
This looks it is subject to a known int overflow, when (original) hi
is close to INT_MAX and the item being close to then end of the array.
~Niels
[The original report had a link here to the googleresearch blog , but
the PG list servers think it is spam :(]
diff --git a/src/timezone/localtime.c b/src/timezone/localtime.c
new file mode 100644
index 19a24e1..878e471
*** a/src/timezone/localtime.c
--- b/src/timezone/localtime.c
*** localsub(const pg_time_t *timep, long of
*** 1070,1076
while (lo < hi)
{
! int mid = (lo + hi) >> 1;
if (t < sp->ats[mid])
hi = mid;
--- 1070,1076
while (lo < hi)
{
! int mid = (lo + hi) >> 1; /* overflow unlikely */
if (t < sp->ats[mid])
hi = mid;
*** pg_next_dst_boundary(const pg_time_t *ti
*** 1423,1429
while (lo < hi)
{
! int mid = (lo + hi) >> 1;
if (t < sp->ats[mid])
hi = mid;
--- 1423,1429
while (lo < hi)
{
! int mid = (lo + hi) >> 1; /* overflow unlikely */
if (t < sp->ats[mid])
hi = mid;
*** pg_interpret_timezone_abbrev(const char
*** 1506,1512
while (lo < hi)
{
! int mid = (lo + hi) >> 1;
if (t < sp->ats[mid])
hi = mid;
--- 1506,1512
while (lo < hi)
{
! int mid = (lo + hi) >> 1; /* overflow unlikely */
if (t < sp->ats[mid])
hi = mid;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
