Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Do not return buffers containing nullptr from internal allocations #20736

Closed
asfimport opened this issue Jan 3, 2019 · 8 comments
Closed

Comments

@asfimport
Copy link

When a 0-byte buffer is allocated, or at the start of a BufferBuilder, the buffer's data pointer can be null. This leads to passing null arguments (with zero sizes) to standard functions such as memset() and memcpy() in many places. UBSAN doesn't like it.

Since a null pointer often means "failed allocating" or "programmer error", we might want to use a non-null pointer to a static empty piece of data instead.

Reporter: Antoine Pitrou / @pitrou
Assignee: Antoine Pitrou / @pitrou

PRs and other links:

Note: This issue was originally created as ARROW-4150. Please see the migration documentation for further details.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
We have two possible solutions actually:

  • follow standard malloc() conventions and return a different pointer everytime, even for 0-size allocations; this means allocating actual memory (probably in 64-byte increments if we want the right alignment)
  • return the same pointer for all 0-size allocations, in which case we can simply return a statically-allocated piece of memory; this makes us non-malloc() compliant, but we don't pretend we are

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
Actually, the unique pointer requirement seems to be glibc-specific. The POSIX and C++ malloc specifications don't make such a requirement.

@asfimport
Copy link
Author

Wes McKinney / @wesm:
Where do you think the nullptr sanitization should take place? In the Buffer constructor?

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
I'm not proposing to sanitize the Buffer class itself, rather to make sure that our allocation and resize routines always return buffers with non-null data pointers.

@asfimport
Copy link
Author

Ben Kietzman / @bkietz:
But buffer.data() will need to recognize that its data pointer is 0xdeadbeef and return nullptr instead

@asfimport
Copy link
Author

Wes McKinney / @wesm:
I see what @pitrou is saying – we use memcpy and memcmp on buffers that originate from our memory allocation functions. The suggestion is not to have null pointers for memory whose origin we know. So we will ignore the issue for general arrow::Buffer

@asfimport
Copy link
Author

Wes McKinney / @wesm:
Issue resolved by pull request 3309
#3309

@asfimport
Copy link
Author

Wes McKinney / @wesm:
I renamed the issue to make it a bit more explicit what is going on

@asfimport asfimport added this to the 0.12.0 milestone Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants