Ban duplicates of parity-uil-mem from being linked into the same program #363
Conversation
ordian
force-pushed
the
ao-deduplicate-parity-util-mem
branch
2 times, most recently
from
c97538a
to
b6418a5
Compare
parity-util-mem/README.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| Collection of memory related utilities. | |||
|
|
|||
| ## WARNING | |||
|
|
|||
| Defining a global allocator outside of this library is only safe for no-std environments or when `estimate-heapsize` is used. | |||
There was a problem hiding this comment.
I think this should be expanded on.
If I understand the issue correctly, the risk for UB occurs when #[global_allocator] is set in more than one place? But this is not possible I think? I actually tried recently, I wanted to use the OS allocator in a benchmark and had to disable parity-util-mem before it worked. I must be missing something, but I think this is the place where it should be explained.
- What does "defining a global allocator outside this library" mean?
- Why is it ok for a
no-stdconsumer to do it?
There was a problem hiding this comment.
See #364 and paritytech/polkadot#922.
- Defining a global allocator not via a feature of parity-util-mem.
- The problem only occurs when we use
malloc_usable_sizefrom allocators.rs, which we don't in no-std consumers.
I'm happy to expand the warning, just let me know what would be the best wording here.
There was a problem hiding this comment.
It seems like we need a whole paragraph here and I'm still too confused to write it. :/
In your "turd" example you have two parity-util-mem, one that pulls in an allocator and one that doesn't: why is this UB? I'd have thought that "cargo features are additive" would cause the second copy to be compiled with the same feature set by the first crate?
"Defining a global allocator not via a feature of parity-util-mem." – this is what happened for the parachain WASM upgrade test right? But how did that even compile? Because the crate itself doesn't pull in parity-util-mem?
But when compiled as a dependency where parity-util-mem was also present it would have failed no?
Anyway maybe something like this: "When parity-util-mem is used as a dependency with any of the allocation features enabled, it must be the sole place where a global allocator is set and it must be present in the dependency tree with a single version.
The only exception to this rule is when used in a no-std context or when the estimate-heapsize feature is used. Unless heeded you risk UB; see discussion [here](… …)."
There was a problem hiding this comment.
In your "turd" example you have two parity-util-mem, one that pulls in an allocator and one that doesn't: why is this UB? I'd have thought that "cargo features are additive" would cause the second copy to be compiled with the same feature set by the first crate?
Because cargo allows to have multiple versions of the same crate. So one (incompatible with the other) version doesn't define a global allocator (and expects to use the system allocator), so it expect a malloc_usable_size from the system allocator, and another version defines jemalloc. You see why it could be a problem? I'm not even sure why it compiles, but it certainly shouldn't (hence the PR).
"Defining a global allocator not via a feature of parity-util-mem." – this is what happened for the parachain WASM upgrade test right? But how did that even compile? Because the crate itself doesn't pull in parity-util-mem?
But when compiled as a dependency where parity-util-mem was also present it would have failed no?
This particular case is safe because it's only defined for no-std.
|
I wonder how all these intricacies will work when things like this stabilizes. |
|
If only GlobalAlloc and Alloc had a |
For sure, if you'd open an RFC for that, that'd be awesome! We'd only have to wait for it to be accepted, implemented stabilized 😅 |
|
Also see rust-lang/wg-allocators#17 |
* master: kvdb-rocksdb: optimize and rename iter_from_prefix (#365) bump parity-util-mem (#376) parity-util-mem: fix for windows (#375) keccak-hash: fix bench and add one for range (#372) [parity-crypto] Release 0.6.1 (#373) keccak-hash: bump version to 0.5.1 (#371) keccak-hash: add keccak256_range and keccak512_range functions (#370) Allow pubkey recovery for all-zero messages (#369) Delete by prefix operator in kvdb (#360) kvdb: no overlay (#313) Ban duplicates of parity-uil-mem from being linked into the same program (#363) Use correct license ID (#362) Memtest example for Rocksdb (#349) Prep for release (#361) parity-util-mem: prepare release for 0.5.2 (#359) travis: test parity-util-mem on android (#358) parity-util-mem: update mimalloc feature (#352) kvdb: remove parity-bytes dependency (#351) parity-util-mem: use malloc for usable_size on android (#355) CI: troubleshoot macOS build (#356)
* master: (56 commits) primitive-types: add no_std support for serde feature (#385) Add Rocksdb Secondary Instance Api (#384) kvdb-rocksdb: update rocksdb to 0.14 (#379) prepare releases for a few crates (#382) uint: fix UB in uint::from_big_endian (#381) Fix limit prefix delete case (#368) Add arbitrary trait implementation (#378) kvdb-rocksdb: optimize and rename iter_from_prefix (#365) bump parity-util-mem (#376) parity-util-mem: fix for windows (#375) keccak-hash: fix bench and add one for range (#372) [parity-crypto] Release 0.6.1 (#373) keccak-hash: bump version to 0.5.1 (#371) keccak-hash: add keccak256_range and keccak512_range functions (#370) Allow pubkey recovery for all-zero messages (#369) Delete by prefix operator in kvdb (#360) kvdb: no overlay (#313) Ban duplicates of parity-uil-mem from being linked into the same program (#363) Use correct license ID (#362) Memtest example for Rocksdb (#349) ...
Part of paritytech/polkadot#922.
This will ensure in compile time that there won't be duplicates of parity-util-mem 0.7+ (e.g. 0.7 and 0.8).
This doesn't prevent programs from having parity-util-mem 0.6 and 0.7 as dependencies though.
Closes #364.