Re: Move allocation size overflow handling to MemoryContextAllocExtended()?

On Wed, Oct 5, 2016 at 2:09 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-10-04 21:40:29 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>> > On 2016-10-05 09:38:15 +0900, Michael Paquier wrote:
>> >> The existing interface of MemoryContextAlloc do not care much about
>> >> anything except Size, so I'd just give the responsability to the
>> >> caller to do checks like queue != (Size) queue when queue is a uint64
>> >> for example.
>>
>> > Well, that duplicates the check and error message everywhere.
>>
>> It seems like you're on the edge of reinventing calloc(), which is not an
>> API that anybody especially likes.
>
> I'm not sure how doing an s/Size/uint64/ in a bunch of APIs does
> that. Because that'd allow us to to throw an error in a number of useful
> cases where we currently can't.
>
> I'm mostly concerned that there's a bunch of cases on 32bit platforms
> where size_t is trivially overflowed. And being a bit more defensive
> against that seems like a good idea. It took about a minute (10s of that
> due to a typo) to find something that looks borked to me:
> bool
> spi_printtup(TupleTableSlot *slot, DestReceiver *self)
> {
>         if (tuptable->free == 0)
>         {
>                 /* Double the size of the pointer array */
>                 tuptable->free = tuptable->alloced;
>                 tuptable->alloced += tuptable->free;
>                 tuptable->vals = (HeapTuple *) repalloc_huge(tuptable->vals,
>                                                                           tuptable->alloced * sizeof(HeapTuple));
>         }
> seems like it could overflow quite easily on a 32bit system.
>
>
> People don't think about 32bit size_t a whole lot anymore, so I think by
> defaulting to 64bit calculations for this kind of thing, we'll probably
> prevent a number of future bugs.

I think you're right, but I also think that if we start using uint64
rather than Size for byte-lengths, it will spread like kudzu through
the whole system and we'll lose the documentation benefit of having
sizes be called "Size".  Since descriptive type names are a good
thing, I don't like that very much.  One crazy idea is to change Size
to always be 64 bits and fix all the places where we translate between
Size and size_t.  But I'm not sure that's a good idea, either.  This
might be one of those cases where it's best to just accept that we're
going to miss some things and fix them as we find them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company