Closed Bug 1669722 Opened 1 year ago Closed 1 year ago

Outdated version of linked-hash-map crate in mozilla-central has UB that triggers assertions when built by nightly Rust

Categories

(Toolkit :: Storage, defect)

Desktop
All
defect

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox83 --- fixed

People

(Reporter: unrelentingtech, Assigned: markh)

References

(Regressed 1 open bug, )

Details

Attachments

(1 file)

When built with nightly Rust, Firefox panics, seems like when doing Sync stuff (golden_gate/webext_storage/rusqlite/lru_cache/linked_hash_map):

Hit MOZ_CRASH(attempted to leave type linked_hash_map::Node<std::sync::Arc<str>, raw_statement::RawStatement> uninitialized, which is invalid) at /home/greg/.rustup/toolchains/nightly-x86_64-unknown-freebsd/lib/rustlib/src/rust/library/core/src/mem/mod.rs:659

See https://github.com/rust-lang/rust/issues/77585 — the undefined behavior that's now detected by nightly Rust has been fixed in the newest release of linked-hash-map, so Firefox should update.

Also in the next rusqlite version this dependency will change to a different, better maintained crate https://github.com/rusqlite/rusqlite/pull/811

Specifically, the linked-hash-map issues were fixed with https://github.com/contain-rs/linked-hash-map/commit/da9e3fb458d1a0f383b033ea41bdb466e3d96805 and https://github.com/contain-rs/linked-hash-map/pull/100, both of which are contained in v0.5.3 which was released back in May.

I'm going to put up a patch to vendor a newer application-services, which includes rusqlite 0.24.1 - which as mentioned above, no longer depends on linked-hash-map. However, that's still going to leave linked-hash-map 0.5.1 in the tree as it's used by serde_yaml and yaml-rust, via geckodriver which appears to be test-only. So this crash should go away, but it isn't being banished as much as would be ideal.

What about updating the vendored linked-hash.map to 0.5.3? That's a semver-compatible update.

Assignee: nobody → markh

(In reply to Ralf Jung from comment #3)

What about updating the vendored linked-hash.map to 0.5.3? That's a semver-compatible update.

As above, that would involve an update to the geckodriver package which I'm not the correct person to do.

Pushed by mhammond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41d48f6233e2
vendor a new app-services, which includes ruslite 0.24.1. r=eoger

Hello Mark !

Based on Comment 2, can this issue be set to NEW? Thank you !

Flags: needinfo?(markh)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(markh)
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

That does not seem to fix the bug though, if old linked-hash-map is still in the crate tree?

(In reply to Ralf Jung from comment #9)

That does not seem to fix the bug though, if old linked-hash-map is still in the crate tree?

That's going to depend on how that crate is now used, right? Comment 0 says "Firefox panics, seems like when doing Sync stuff" and I believe that will no longer be true. Best I can tell, "geckodriver" is used for testing/automation.

That's going to depend on how that crate is now used, right?

Well, yes, I guess. Using it with any type that has a "niche" (i.e., that has some forbidden bit-patterns) will cause a panic. To me it seems easier to update the crate than to figure out if there is any such user (never mind the fact that this update pulls in a UB fix, i.e., a fix for a rather severe bug).

(In reply to Ralf Jung from comment #11)

To me it seems easier to update the crate

Great! Please file a bug for updating geckodriver and I'll help find a reviewer for your patch.

I'm afraid I have no familiarity with the workflow here, I just assumed that it would be about as easy as cargo update for someone that knows how all this vendoring stuff works. If not, I am a bit worried about Firefox using outdated Rust crates -- but in the end that's obviously up to y'all. I think my time is more effectively spent on the Rust compiler and language itself.

Regressions: 1671170
You need to log in before you can comment on or make changes to this bug.