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

2016-10-07 Thread Robert Haas 
On Wed, Oct 5, 2016 at 2:09 PM, Andres Freund  wrote:
> On 2016-10-04 21:40:29 -0400, Tom Lane wrote:
>> Andres Freund  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


-- 
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] Move allocation size overflow handling to MemoryContextAllocExtended()?

2016-10-05 Thread Andres Freund 
On 2016-10-04 21:40:29 -0400, Tom Lane wrote:
> Andres Freund  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.

Greetings,

Andres Freund


-- 
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] Move allocation size overflow handling to MemoryContextAllocExtended()?

2016-10-04 Thread Tom Lane 
Andres Freund  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.  The problem with trying to centralize
overflow handling is that there are too many different cases.  calloc()
handles exactly one case, where the request is to be a product of exactly
two inputs that are individually not capable of overflowing.  As best I
can follow what you've got in mind here, it would be equally likely to
cover only a subset of cases.

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] Move allocation size overflow handling to MemoryContextAllocExtended()?

2016-10-04 Thread Andres Freund 
On 2016-10-05 09:38:15 +0900, Michael Paquier wrote:
> On Wed, Oct 5, 2016 at 8:56 AM, Andres Freund  wrote:
> > That made me wonder if it's not actually a mistake for
> > MemoryContextAllocExtended() size parameter to be declared
> > Size/size_t. That prevents it from detecting such overflows, forcing
> > code like the above on callsites.
> >
> > Comments?
> 
> 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.

And queue != (Size) queue will cause errors like
/home/andres/src/postgresql/src/include/lib/simplehash.h:182:44:
warning: self-comparison always evaluates to false [-Wtautological-compare]

> And I can see that your patch is using uint32 for SH_TYPE->size.

But it multiples the size with sizeof(elemement)...

Greetings,

Andres Freund


-- 
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] Move allocation size overflow handling to MemoryContextAllocExtended()?

2016-10-04 Thread Michael Paquier 
On Wed, Oct 5, 2016 at 8:56 AM, Andres Freund  wrote:
> That made me wonder if it's not actually a mistake for
> MemoryContextAllocExtended() size parameter to be declared
> Size/size_t. That prevents it from detecting such overflows, forcing
> code like the above on callsites.
>
> Comments?

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. And I can see that your patch is using uint32 for
SH_TYPE->size.
 --
Michael


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

[HACKERS] Move allocation size overflow handling to MemoryContextAllocExtended()?

2016-10-04 Thread Andres Freund 
Hi,

I was working on making sure that allocations for [1] don't overflow
size_t for large hash tables, when created on 32bit systems.  I started
to write code like

if (sizeof(SH_CONTAINS) * (uint64) tb->size) !=
   sizeof(SH_CONTAINS) * (size_t) tb->size))
{
elog(ERROR, "hash table too large for a 32 bit system");
}

that could code potentially, although somewhat unlikely, be trigger on a
32bit system.


That made me wonder if it's not actually a mistake for
MemoryContextAllocExtended() size parameter to be declared
Size/size_t. That prevents it from detecting such overflows, forcing
code like the above on callsites.

Comments?

- Andres

[1] 
http://archives.postgresql.org/message-id/20160727004333.r3e2k2y6fvk2ntup%40alap3.anarazel.de


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