Is sdallocx safe to call with a null pointer? about jemallocator HOT 9 OPEN

The behavior of calling {GlobalAlloc, Alloc}::dealloc with a null pointer or with a size of 0 is undefined, so the bug that's causing that must be somewhere else. I'm going to leave this issue open until the documentation bug for jemalloc-sys is fixed.

We should probably also add an assume!(!ptr.is_null()) to {GlobalAlloc, Alloc}::dealloc.

from jemallocator.

According to jemalloc's docs:

• The dallocx() function causes the memory referenced by ptr to be made available for future allocations.

• The sdallocx() function is an extension of dallocx() with a size parameter to allow the caller to pass in the allocation size as an optimization. The minimum valid input size is the original requested size of the allocation, and the maximum valid input size is the corresponding value returned by nallocx() or sallocx().

so IIUC the following should work:

void* ptr = malloc(0); // Does this return NULL ? jemalloc docs don't say
sdallocx(ptr, 0);

yet that would always make the assert fail. So either this snippet is correct and the assert is incorrect, or the docs are incorrect, and this snippet would need to be written like this:

void* ptr = malloc(size);
if (size == 0) { free(ptr); } else { sdallocx(ptr, size); }

which IMO would make the API of jemalloc unnecessarily hard to use correctly.

@davidtgoldblatt could you clarify what the intent is here?

from jemallocator.

@davidMcneil

The malloc() and calloc() functions return a pointer to the allocated memory if successful; otherwise a NULL pointer is returned and errno is set to ENOMEM.

That is, jemalloc memory allocation functions only returns NULL if the allocation failed, and sdallocx does not support being called with NULL, so the error is in the comment in this crate. Calling sdallocx with NULL is not allowed, you need to call it with an allocation that succeeded.

from jemallocator.

Thanks for the response @gnzlbg! I am not using jemalloc-sys directly. The bad call to sdallocx is occurring in jemallocator's implementation of dealloc.

I will see if I can get a more minimal example replicating the issue.

from jemallocator.

@davidtgoldblatt could you clarify what the intent is here?

We've historically tried to avoid taking a strong stance on this sort of implementation-defined thing (e.g. return value of malloc(0)). Wanting C++ interop has made this harder and harder, to the point that I think we might just as well commit to malloc(0) being non-NULL (barring OOM).

I'm not sure anyone ever thought too hard about the sdallocx-with-zero case. We used to assert against it, but took it away (again, for C++ interop reasons). The more I think about it, this less I'm convinced that it was ever the right thing to do. If we give out non-NULL pointers in response to a request for a particular size, we ought to accept that size back again in sdallocx.

from jemallocator.

@davidtgoldblatt is there an issue in jemalloc upstream that we could cross-reference from here ?

In Rust, the behavior of requesting an allocation with zero-size is undefined, i.e., malloc(N) will always be called with N > 0, so if it returns a nullptr, then that must be because the allocation failed. Trying to deallocate such a nullptr (e.g. free(NULL), or sdallocx(NULL, N > 0)) is also UB in Rust, so these cases should never be triggered. That is, we will add asserts anyways to our wrappers to try to diagnose these issues in the Rust side.

from jemallocator.

@davidtgoldblatt is there an issue in jemalloc upstream that we could cross-reference from here ?

I don't think so; what specifically are you referring to though? (I think a catch-all "there should be better documentation around edge-cases" issue with a list would be useful; I don't think we have one at the moment).

from jemallocator.

I started a list at jemalloc/jemalloc#1716

from jemallocator.

Thanks, that was precisely what I was referring to.

from jemallocator.