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.

Sign up for GitHub

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

Ban duplicates of parity-uil-mem from being linked into the same program #363

Merged
merged 5 commits into from
Mar 25, 2020
Merged

Ban duplicates of parity-uil-mem from being linked into the same program #363

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
395a6f9
Prevent multiple versions of parity-uil-mem from being linked into th…
ordian Mar 22, 2020
b6418a5
fmt
ordian Mar 22, 2020
e3519de
parity-util-mem: add a warning about defining global allocs
ordian Mar 23, 2020
496a362
parity-util-mem: document empty build script
ordian Mar 24, 2020
23c6c39
parity-util-mem: extend the warning based on David comments
ordian Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions parity-util-mem/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog].
[Keep a Changelog]: http://keepachangelog.com/en/1.0.0/

## [Unreleased]
### Breaking
- Prevent multiple versions from being linked into the same program. [#363](https://github.com/paritytech/parity-common/pull/363)

## [0.6.0] - 2020-03-13
- Updated dependencies. [#361](https://github.com/paritytech/parity-common/pull/361)
Expand Down
4 changes: 4 additions & 0 deletions parity-util-mem/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ description = "Collection of memory related utilities"
license = "MIT/Apache-2.0"
edition = "2018"

# Prevent multiple versions from being linked into the same program.
ordian marked this conversation as resolved.
Show resolved Hide resolved
links = "parity-util-mem-ban-duplicates"
build = "build.rs"

[dependencies]
cfg-if = "0.1.10"
dlmalloc = { version = "0.1.3", features = ["global"], optional = true }
Expand Down
4 changes: 4 additions & 0 deletions parity-util-mem/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. What does "defining a global allocator outside this library" mean?
  2. Why is it ok for a no-std consumer to do it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #364 and paritytech/polkadot#922.

  1. Defining a global allocator not via a feature of parity-util-mem.
  2. The problem only occurs when we use malloc_usable_size from 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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](… …)."

Copy link
Member Author

@ordian ordian Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Features

- estimate-heapsize : Do not use allocator, but `size_of` or `size_of_val`.
Expand Down
1 change: 1 addition & 0 deletions parity-util-mem/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fn main() {}