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.

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

The sync-read-txn feature of heed all the v0.10 is unsound #1755

Open
Kerollmops opened this issue Aug 23, 2023 · 3 comments
Open

The sync-read-txn feature of heed all the v0.10 is unsound #1755

Kerollmops opened this issue Aug 23, 2023 · 3 comments
Labels
Unsound Informational / Unsound

Comments

@Kerollmops
Copy link

A contributor recently discovered that heed's sync-read-txn feature is unsound and should not exist. This feature was implementing Sync on the RoTxn struct, but in fact, it should have only implemented Send. I explained the different implications in an issue comment.

I replaced this feature with the correct read-txn-no-tls, which makes the RoTxn: Send. According to the LMDB documentation and Howard Chu's comments, it is correct to do that. Unfortunately, this change is not yet available as I am working on a big refactor. However, it will be available in v0.20.0.

What should I do? Should I yank all the heed versions even if only a specific feature is affected?

@Shnatsel
Copy link
Member

The ideal solution is to backport this change and publish a new release in the v0.10.x series with the fix.

If that is not feasible (the fix depends on other changes made during the refactor), you could file a security advisory telling people not to use the feature. Sadly none of the audit tooling is feature-aware, so the warning will be shown regardless of whether the feature is actually enabled.

I believe yanking is overkill in this case, since it would break workflows even for people who are not affected by the issue.

@amousset amousset added the Unsound Informational / Unsound label Aug 28, 2023
@Kerollmops
Copy link
Author

Kerollmops commented Aug 29, 2023

Thank you very much for your answer 👌

The ideal solution is to backport this change and publish a new release in the v0.10.x series with the fix.

Replacing this feature with another with a new v0.11.1 version is feasible, or even patching the v0.10 series. However, isn't removing a cargo feature in a patch version breaking? It risks breaking the builds. What do you think?

@Shnatsel
Copy link
Member

I would prefer releasing a fixed 0.11.x and removing the broken 0.10.x feature at the same time. That way for users who are not affected everything works fine, and the affected users can upgrade to a fixed version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Unsound Informational / Unsound
Projects
None yet
Development

No branches or pull requests

3 participants