encoding conversion functions versus zero-length inputs

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: encoding conversion functions versus zero-length inputs
Date: 2009-02-28 17:20:04
Message-ID: 21789.1235841604@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

The REL7_4 members of the buildfarm are all red this morning,
with this symptom in initdb:

creating template1 database in /usr/src/pg/build-farm-2.17/build/REL7_4_STABLE/pgsql.18854/src/test/regress/./tmp_check/data/base/1... ok
initializing pg_shadow... ok
enabling unlimited row size for system tables... ok
initializing pg_depend... ok
creating system views... ok
loading pg_description... ok
creating conversions... ERROR: invalid memory alloc request size 0

This is coming from Heikki's patch of yesterday to check whether
a proposed conversion function accepts the indicated encoding IDs.
He has it passing a zero-length input string, which is a case that
the encoding conversion machinery normally short-circuits; and sure
enough some of the converters fall over if they're actually called on
such an input. The one that dies first here is koi8r_to_win1251,
which contains

buf = palloc(len * ENCODING_GROWTH_RATE);
koi8r2mic(src, buf, len);
mic2win1251(buf, dest, strlen((char *) buf));

Pre-8.0, we had palloc throwing an error for a zero-size request, thus
this doesn't work. It's worse than that though --- if the combination
of that palloc and the subsequent strlen call hasn't already set off
alarm bells in your mind, then you haven't been writing C long enough.
Sure enough, koi8r2mic null-terminates its result, so the strlen is
correct, but the palloc isn't leaving space for a trailing null.
It manages to fail to fail in normal usage (len > 0) because
ENCODING_GROWTH_RATE is ridiculously overstated, but on zero-length
input there is actually a memory clobber happening here in the 8.x

regression=# CREATE CONVERSION foo FOR 'KOI8R' TO 'WIN1251' FROM koi8r_to_win1251;
WARNING: detected write past chunk end in PortalHeapMemory 400f3be0

My first thought about fixing this was just to alter the check patch to
pass a length-one instead of length-zero test string, but I now think
that that's just hiding our heads in the sand; the right fix is to go
around and make all these palloc's "len * ENCODING_GROWTH_RATE + 1"
so that they are honestly accounting for the terminating null. It's
a bit more tedious but it's the right fix.

regards, tom lane


Browse pgsql-hackers by date

  From Date Subject
Next Message James Pye 2009-02-28 18:12:30 Re: xpath processing brain dead
Previous Message Andrew Dunstan 2009-02-28 14:53:47 Re: xpath processing brain dead