paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Handle zero-sized allocations properly #3561

Open cmichi opened 5 years ago

cmichi commented 5 years ago

Escalated from the discussion here.

Right now the allocator always uses at least 8 bytes when allocating (see here), even when alloc(0) is called. Instead the case of zero-sized allocations should be handled specially, without actually allocating space (which is done at the moment).

One idea to explore is to return a "sentinel pointer"; something like sentinel_ptr = align_to_8_bytes(max_heap_size + 1) would be guaranteed to not be a valid pointer and could be used. dealloc needs to be aware of this "sentinel pointer" and handle it specially (by not actually freeing).

If you want to work on this, it's best to talk to @pepyakin for more details.

pepyakin commented 5 years ago

FWIW, there are more options for sentiel value:

  1. A commonly used approach is to use the alignment of the value. One downside of this approach, is that rustc/lld puts stack at the beginning of the linear memory and stack grows downwards. When the stack is about to overflow, then weird things can happen, like the address of a local variable can be equal to the sentiel address of a zero-sized allocation. I am not sure if this should be a concern, since the probability of experiencing this issue is really low: e.g. reference equality is not common in rust, this happens only if the stack is about to overflow but it doesn't overflow, the compared values should align correctly, etc. Moreover, UB can be spawn if the address of the last word on the stack (i.e. 0 aka NULL), so this is not a big stretch. Nevertheless, if we decide to go with this approach, a comment on this should be left to greatly relief the lives of the auditors.
  2. We can use 0 value for the sentiel. In this case, we would need to change our GlobalAlloc implementation, since returning NULL pointer from there denotes OOM condition and thus will lead to abort and panic which is not desirable since 0-allocation is pretty legit. Besides that, sometimes we return pointers to the allocated data from the Substrate Runtime Interfae host, so we need to ensure that these values are not used for things like from_raw_parts or similar.
  3. We can allocate 8 bytes (or however many we use for overaligning) at the beginning of the heap and use that as a special sentiel value.
ghost commented 1 year ago

@cmichi , dead link: https://github.com/paritytech/substrate/blob/master/core/executor/src/allocator.rs#L87

Is this issue still actual?

ghost commented 1 year ago

(this does not look like a bug, more of a flaw )

cmichi commented 1 year ago

@abebeos This issue is four years old. I would be surprised if it's still relevant.

I am no longer involved with Substrate, best to check who modified relevant code for this issue last and ask that person about it.

ghost commented 1 year ago

This issue is four years old. I would be surprised if it's still relevant.

Yes, I always enter projects via the "oldies". At least the minimum I do is triggering a cleanup.

I am no longer involved with Substrate, best to check who modified relevant code for this issue last and ask that person about it.

k

https://github.com/paritytech/substrate/commits/933e9c53a0e9064f2bf80508d60b16d99e3e3bd0/client/allocator/src

@gilescope , @bkchr , @kianenigma

bkchr commented 1 year ago

@abebeos so what is your question here? If this is still relevant?

ghost commented 1 year ago

@abebeos so what is your question here? If this is still relevant?

Yes, few comments up I reported a dead link and asked: https://github.com/paritytech/substrate/issues/3561#issuecomment-1440318041

Link should point here

https://github.com/paritytech/substrate/blob/56de870f943679fede8443d06d3aa96a9f7321d6/client/allocator/src/freeing_bump.rs#L105

(thinking about it, this zero-alloc looks like a standard problem. A standard solution should exist)