Date: Tue, 10 Jan 2017 02:50:57 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Daniel Verite <daniel@manitou-mail.org>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Craig Ringer <craig@2ndquadrant.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Robert Haas <robertmhaas@gmail.com>,
	Jim Nasby <Jim.Nasby@bluetreble.com>,
	Ronan Dunklau <ronan.dunklau@dalibo.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: pg_dump / copy bugs with "big lines" ?
Message-ID: <20170110055057.utnaxegvbuwslpex@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="yyabnzzuclxzmixj"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1706e85e-60d2-494e-8a64-9af1e1b2186e@manitou-mail.org>
User-Agent: NeoMutt/20161104 (1.7.1)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--yyabnzzuclxzmixj
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Daniel Verite wrote:

> My tests are OK too but I see an issue with the code in
> enlargeStringInfo(), regarding integer overflow.
> The bit of comment that says:
> 
>   Note we are assuming here that limit <= INT_MAX/2, else the above
>   loop could overflow.
> 
> is obsolete, it's now INT_MAX instead of INT_MAX/2.

I would keep this comment but use UINT_MAX/2 instead.

> There's a related problem here:
> 	newlen = 2 * str->maxlen;
> 	while (needed > newlen)
> 		newlen = 2 * newlen;
> str->maxlen is an int going up to INT_MAX so [2 * str->maxlen] now
> *will* overflow when [str->maxlen > INT_MAX/2].
> Eventually it somehow works because of this:
> 	if (newlen > limit)
> 		newlen = limit;
> but newlen is wonky (when resulting from int overflow)
> before being brought back to limit.

Yeah, you're right.  We also need to cast "needed" to Size in the while
test; and the repalloc_huge() call no longer needs a cast.

I propose the attached.

Not sure if we also need to cast the assignment to str->maxlen in the
last line.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--yyabnzzuclxzmixj
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: attachment; filename="fix-stringinfo.patch"
Content-Transfer-Encoding: quoted-printable

commit 1327c2eea9c7ca074d88bb167bd4c35338d2de0b=1B[m
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Tue Jan 10 02:46:42 2017 -0300
CommitDate: Tue Jan 10 02:46:42 2017 -0300

    Fix overflow check in StringInfo
=20=20=20=20
    A thinko I introduced in fa2fa9955280.  Also, amend a similarly broken
    comment.
=20=20=20=20
    Report and patch by Daniel V=E9rit=E9.
    Discussion: https://postgr.es/m/1706e85e-60d2-494e-8a64-9af1e1b2186e@ma=
nitou-mail.org

diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index bdc204e..11d751a 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -313,19 +313,19 @@ enlargeStringInfo(StringInfo str, int needed)
 	 * for efficiency, double the buffer size each time it overflows.
 	 * Actually, we might need to more than double it if 'needed' is big...
 	 */
-	newlen =3D 2 * str->maxlen;
-	while (needed > newlen)
+	newlen =3D 2 * (Size) str->maxlen;
+	while ((Size) needed > newlen)
 		newlen =3D 2 * newlen;
=20
 	/*
 	 * Clamp to the limit in case we went past it.  Note we are assuming here
-	 * that limit <=3D INT_MAX/2, else the above loop could overflow.  We will
+	 * that limit <=3D UINT_MAX/2, else the above loop could overflow.  We wi=
ll
 	 * still have newlen >=3D needed.
 	 */
 	if (newlen > limit)
 		newlen =3D limit;
=20
-	str->data =3D (char *) repalloc_huge(str->data, (Size) newlen);
+	str->data =3D (char *) repalloc_huge(str->data, newlen);
=20
 	str->maxlen =3D newlen;
 }

--yyabnzzuclxzmixj
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--yyabnzzuclxzmixj--