search

microsoft / snmalloc

Message passing based allocator
MIT License
1.58k stars 109 forks source link

aligned_alloc requirements update. #681

Closed devnexen closed 1 month ago

devnexen commented 2 months ago

in addition, specs requires size to be a multiple of alignment.

mjp41 commented 2 months ago

@bsdjhb and @jrtc27 is this change acceptable to you.

Allocate size bytes of uninitialized storage whose alignment is specified by alignment. The size parameter must be an integral multiple of alignment. https://en.cppreference.com/w/c/memory/aligned_alloc

So this seems reasonable to check.

snmalloc doesn't actually care, and will do the correct thing if that is not the case, e.g.

aligned_alloc(96,64) will actually map to an allocation of 128 bytes with that alignment.

I do wonder if there should be a mitigations feature enforce libc spec, so that uses know when they are violating the spec, but for performance sensitive use cases, we do not need to take the instructions for checking these conditions. E.g. add something to:

https://github.com/microsoft/snmalloc/blob/fb776da909eeb68835294e351ecaf3ae270b6891/src/snmalloc/ds_core/mitigations.h#L68-L211

@nwf do you have thoughts on this.

jrtc27 commented 2 months ago

No objection to enforcing that. The OpenZFS case that motivated removing the overly-strict requirement does aligned_alloc(align, roundup2(sizeof(*zfs), align)); so conforms to that requirement in the standard.

mjp41 commented 2 months ago

So I dug a bit deeper into this. We did use to have this assertion and removed it here:

https://github.com/microsoft/snmalloc/pull/658

It seems that the spec was loosened by

N2072

This seems to be in the the N2310 draft for C17. So the spec does not require an allocator to fail on this case. I would sooner leave the check out as it will be faster.

@devnexen did you have a strong reason for wanting this?

jrtc27 commented 2 months ago

Looking at a C23 draft there (N3220) there is no restriction given for size, even (inheriting the same behaviour for 0 as malloc as that's hoisted out into the section introduction). So yeah whilst this restriction is fine for C11 it is wrong for newer standards.

devnexen commented 2 months ago

I would not mind then to enable this additional check only for C11 ?

jrtc27 commented 2 months ago

You have no idea what C standard the caller is compiled for.

devnexen commented 2 months ago

true true ... ok gonna drop this check then.