Re: pg_control is missing a field for LOBLKSIZE

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jun 4, 2014 at 1:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> BTW, just comparing the handling of TOAST_MAX_CHUNK_SIZE and LOBLKSIZE,
>> I noticed that the tuptoaster.c functions are reasonably paranoid about
>> checking that toast chunks are the expected size, but the large object
>> functions are not: the latter have either no check at all, or just an
>> Assert that the size is not more than expected. So we could provide at
>> least a partial guard against a wrong LOBLKSIZE configuration by making
>> all the large-object functions throw elog(ERROR) if the length of a LO
>> chunk is more than LOBLKSIZE. Unfortunately, length *less* than LOBLKSIZE
>> is an expected case, so this would only help in one direction. Still,
>> it'd be an easy and back-patchable change that would provide at least some
>> defense, so I'm thinking of doing it.

> This seems like a pretty weak argument for adding run-time overhead.
> Maybe it can be justified on the grounds that it would catch corrupted
> TOAST data, but I've never heard of anyone changing LOBLKSIZE and see
> no reason to get agitated about it.

One if-test per fetched tuple hardly seems likely to add measurable
overhead. As for "never heard of", see today's thread in pgsql-general:
http://www.postgresql.org/message-id/flat/CAGou9Mg=9qPYTdh18NDO3LTJtwQN8uRdTwABfkcyMRUt6D_fJw(at)mail(dot)gmail(dot)com
There was a similar gripe a few months ago:
http://www.postgresql.org/message-id/CACg6vWXy_84ShCCXzNCsz9xLfWnx5sZvQRU6aNcrR0c3XW1Bxg@mail.gmail.com

There are at least two places in inv_api.c where we have
"Assert(pagelen <= LOBLKSIZE)" that is protecting a subsequent memcpy
into a local variable of size LOBLKSIZE, so that the only thing standing
between us and a stack-smash security issue that's trivially exploitable
in production builds is that on-disk data conforms to our expectation
about LOBLKSIZE. I think it's definitely worth promoting these checks
to regular runtime-if-test-and-elog.

regards, tom lane