Protobuf: Null pointer dereference in Arena.

[FSMaxB]

If the allocator from the ArenaOptions returns nullptr, the placement new after that will dereference a null pointer.

https://github.com/google/protobuf/blob/b61dd9d9a229c5a82788fdeefa344aa65affae09/src/google/protobuf/arena.cc#L119-L120

I'm currently dealing with this by throwing an exception from my allocator instead of returning nullptr, but I'm not really sure if your code is exception safe.

[acozzette]

Thanks for reporting this, but I believe it is working as intended because we do not use exceptions in C++, as per the Google C++ style guide (see here).

[FSMaxB]

I am aware that you aren't using any exceptions at Google, but that doesn't have anything to do with this null pointer dereference.

In google/protobuf/arena.h it says:
https://github.com/google/protobuf/blob/b61dd9d9a229c5a82788fdeefa344aa65affae09/src/google/protobuf/arena.h#L130-L135

Given that malloc may return nullptr if allocation fails, I would expect from that statement, that my custom allocator should do the same.

In any case if a nullptr is returned, it is dereferenced when doing the placement new. This has nothing to do with exceptions being enabled/disabled.

You might argue that malloc never returns nullptr because of overcommit, but:

1. This isn't always the case e.g.:
   
   
   
   • Overcommit could be disabled
   
   
   • Some platforms don't do overcommit in the first place.
   
   
   • Even with overcommit, malloc might still return nullptr if the requested size was too large.
   
   
2. Given that this pointer can be supplied by any user of the library using ArenaOptions, there can be many reasons why a nullptr might be returned, that don't relate to how standard malloc works.

I'm currently dealing with this by throwing an exception from my allocator instead of returning nullptr, but I'm not really sure if your code is exception safe.

The above statement was just a way of telling you what my workaround is. And I'm concerned about this workaround being bad, exactly because Google doesn't use exceptions. Because this means that Protobuf is written completely without exception safety in mind, making me question if my workaround is safe.

I'm not sure what code clang++/g++ emit for new if the allocation fails. I would guess that they call std::terminate, because breaking the guarantee that new never returns nullptr would be really bad!

Assuming that std::terminate is called when compiling with -fno-exceptions I can see two ways how this library could handle it if block_alloc returns nullptr:

1. Call std::terminate
2. Throw std::bad_alloc and rely on the compiler automatically turning it into std::terminate if exceptions are disabled at compile time. I would prefer this option because it takes all the users of the library into account, that don't compile their code with exceptions disabled. But this also means that exception safety should be guaranteed in codepaths that lead to block_alloc being called.

[acozzette]

Ah ok, I see what you're saying. It seems to me like the right solution is to update the comments to clarify that block_alloc should never return nullptr--really it should behave more like new than like malloc. We've made no attempt to make the code exception-safe, so I suspect that there's not going be a useful way to attempt to handle a std::bad_alloc exception in the context of an Arena. If you don't want to interfere with other code that handles exceptions, it might be best then to call std::terminate from within block_alloc if for some reason the allocation fails.

[FSMaxB]

I still think that protobuf should check and call std::terminate, because it is safer than relying on people to read the comment carefully.

And since this is only used for allocating blocks for the Arena, it won't be called very often, so the additional nullptr check isn't likely to cause any performance problems.

[xfxyjwf]

@FSMaxB With the current code, wouldn't it cause the program to terminate anyway when block_alloc() returns nullptr?

[FSMaxB]

@xfxyjwf Not necessarily. The placement new in line 120 will call the constructor of Block with a nullptr.
https://github.com/google/protobuf/blob/b61dd9d9a229c5a82788fdeefa344aa65affae09/src/google/protobuf/arena.cc#L125-L126

This then tries to write a pointer to offsetof(Block, next_), wherever that might be.

Most systems do have the zero page mapped in such a way that an interrupt is called and the program terminated.

But this is not the case on all systems (especially on systems without an MMU, but also if no interrupt is set up). Meaning that the program might keep on running while being in an invalid state. Even worse: After that the memory beginning at 0 + something small will be used for regular allocations, overwriting everything on it's way.

See the Section Is there life after jumping to 0x0 in this article describing a GRUB vulnerability for example (GRUB is running without virtual memory enabled, so no termination on nullptr dereference there, even on x86_64).

[chrisosaurus]

@xfxyjwf relying on undefined behaviour to cause a segfault and thus terminate is not usually good practice.