Bug in wasteful_vector page allocator

Thanks to Matthew Riley who noticed this issue and provided the initial proposal
for the fix.

There's a bug in the new allocator<T> implementation used by wasteful_vector. 
It inherits the base class' implementation of allocator and doesn't implement
allocate() so it goes to the heap instead of the PageAllocator -- the very thing
wasteful_vector was trying to avoid!  As a side effect it was also leaking heap
memory.

Thanks,
-Ivan
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1188

[Ivan Penkov]

.

[Mark Mentovai]

Good find!

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h
File src/common/memory.h (right):

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode1
src/common/memory.h:1: // Copyright (c) 2009, Google Inc.
The CL description is used as the check-in comment. Right now it just says “Bug
in wasteful_vector.” That’s not enough. The message you sent with your e-mail,
or an edited form of that message, would be more appropriate.

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode99
src/common/memory.h:99: bool OwnsPointer(const void* p) {
Comment that this is exposed for testing only.

Maybe even make it private and make the test a friend.

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode101
src/common/memory.h:101: while (header) {
Optional but generally clearer for iterations like this: use a “for” loop.

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode103
src/common/memory.h:103: if ((p > current) && (p < current + header->num_pages *
page_size_))
p >= current, right?

[glandium]

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h
File src/common/memory.h (right):

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode161
src/common/memory.h:161: inline pointer allocate(size_type n, const void* = 0) {
ouch.

[Ivan Penkov]

.

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h
File src/common/memory.h (right):

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode1
src/common/memory.h:1: // Copyright (c) 2009, Google Inc.
On 2013/05/23 02:37:47, Mark Mentovai wrote:
> The CL description is used as the check-in comment. Right now it just says
“Bug
> in wasteful_vector.” That’s not enough. The message you sent with your e-mail,
> or an edited form of that message, would be more appropriate.

Done.

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode99
src/common/memory.h:99: bool OwnsPointer(const void* p) {
On 2013/05/23 02:37:47, Mark Mentovai wrote:
> Comment that this is exposed for testing only.
> 
> Maybe even make it private and make the test a friend.

Done.

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode101
src/common/memory.h:101: while (header) {
On 2013/05/23 02:37:47, Mark Mentovai wrote:
> Optional but generally clearer for iterations like this: use a “for” loop.

Done.

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode103
src/common/memory.h:103: if ((p > current) && (p < current + header->num_pages *
page_size_))
On 2013/05/23 02:37:47, Mark Mentovai wrote:
> p >= current, right?

Each set of pages starts with a header, so the "user" pointer (the one returned
by Alloc()) will always come after the page header, hence using '>' is correct. 
Anyways, I think using >= is a little cleaner, in the sense that the page header
is also owned by the allocator, so I'm updating it to use >=.

Done.

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h
File src/common/memory.h (right):

https://breakpad.appspot.com/599002/diff/10001/src/common/memory.h#newcode103
src/common/memory.h:103: if ((p > current) && (p < current + header->num_pages *
page_size_))
Ivan Penkov wrote:
> On 2013/05/23 02:37:47, Mark Mentovai wrote:
> > p >= current, right?
> 
> Each set of pages starts with a header, so the "user" pointer (the one
returned
> by Alloc()) will always come after the page header, hence using '>' is
correct. 
> Anyways, I think using >= is a little cleaner, in the sense that the page
header
> is also owned by the allocator, so I'm updating it to use >=.
> 
> Done.

If you wanted to skip the header, you’d use >= current + sizeof(*header). That
would be fine too, and you can make that change if you like. As it was, you were
saying that the first byte of the header wasn’t “owned” but the other bytes of
the header were.

[Mark Mentovai]

LGTM