Skip to content

tanton_engine: possible public unsound api #2286

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

Merged
merged 1 commit into from
May 6, 2025

Conversation

charlesxsh
Copy link
Contributor

The original github repo seems like got deleted, not sure how to create the PR/issue at the upstream project

@djc
Copy link
Contributor

djc commented Apr 25, 2025

This is the 4th PR you've filed this month (#2262, #2263, #2284). How are you finding these?

Please note the failing CI checks.

@charlesxsh
Copy link
Contributor Author

This is the 4th PR you've filed this month (#2262, #2263, #2284). How are you finding these?

Please note the failing CI checks.

Thanks for the notifying. I will fix the linting ASAP. I am working on a research project that can automatically find these issues.

@djc djc changed the title possible public unsound api tanton_engine: possible public unsound api Apr 25, 2025
@charlesxsh
Copy link
Contributor Author

Do we have any update/plan on merging these PRs? @djc

@djc
Copy link
Contributor

djc commented May 2, 2025

I sent an email to the maintainer just now, I'm going to wait for 2 weeks before publishing this, per discussion in #1092.

@djc
Copy link
Contributor

djc commented May 2, 2025

Okay, we have approval from the author to publish. I think we want to substantially simplify the advisory, though:

  • Drop the long code samples
  • Use the functions key to highlight (some of) the unsafe functions you've found
  • Provide a high-level description of the issue (like "lack of bound checking")
  • Explicitly mention that this crate no longer appears to be maintained

@djc djc mentioned this pull request May 5, 2025
@charlesxsh
Copy link
Contributor Author

Okay, we have approval from the author to publish. I think we want to substantially simplify the advisory, though:

  • Drop the long code samples
  • Use the functions key to highlight (some of) the unsafe functions you've found
  • Provide a high-level description of the issue (like "lack of bound checking")
  • Explicitly mention that this crate no longer appears to be maintained

I updated, let me know if the updated version is ok.

@djc djc force-pushed the taton_engine branch from 7d6e169 to 08b8078 Compare May 6, 2025 08:25
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

I cleaned up the advisory for you. Please apply similar changes to your other PRs.

Comment on lines +9 to +12
"tanton_engine::Stack::offset" = ["1.0.0"]
"tanton_engine::ThreadStack::get" = ["1.0.0"]
"tanton_engine::RootMoveList::insert_score_depth" = ["1.0.0"]
"tanton_engine::RootMoveList::insert_score" = ["1.0.0"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Changed the version to only match the single current version, which is presumably the only one you've checked.

unaffected = []
```

# Unsound public API in unmaintained crate
Copy link
Contributor

Choose a reason for hiding this comment

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

Removed the use of Possible in the title, removed the crate name since it's already in the metadata and gets used in lots of places.

Comment on lines +24 to +27
- `Stack::offset()`
- `ThreadStack::get()`
- `RootMoveList::insert_score_depth()`
- `RootMoveList::insert_score()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Turned affected functions into a bullet list, used :: instead of . as is idiomatic in Rust and added () suffix to make it obvious that these are functions.

@djc djc merged commit 79680cf into rustsec:main May 6, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants