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

Add (very basic) support for replace #1

Closed
wants to merge 6 commits into from

Conversation

henrikhorluck
Copy link

@henrikhorluck henrikhorluck commented Jun 21, 2023

  • Expose captures
  • Add very basic replacement support

See fish-shell/fish-shell#9854

@henrikhorluck henrikhorluck marked this pull request as ready for review June 23, 2023 23:42
@zanchey zanchey requested a review from ridiculousfish June 24, 2023 22:11
Copy link
Member

@ridiculousfish ridiculousfish left a comment

Choose a reason for hiding this comment

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

This is amazing!! Thank you!

I have some simplifying suggestions and a few nits.

src/regex_impl.rs Outdated Show resolved Hide resolved
src/regex_impl.rs Outdated Show resolved Hide resolved
src/regex_impl.rs Outdated Show resolved Hide resolved
src/regex_impl.rs Outdated Show resolved Hide resolved
src/regex_impl.rs Outdated Show resolved Hide resolved
henrikhorluck and others added 3 commits June 26, 2023 01:57
- There is no reason for the replacement to share lifetime with the
  subject, because the replacement is not present in the return value,
  even if no replacement occured
This fixes the following issue in replace_impl: the call to
try_reserve passed in a difference of capacities, but try_reserve
expects a difference between the desired capacity and the length.
Because the initial capacity was nonzero but the length was zero, this
caused us to reserve less capacity than we ought to have, leading to an
OOB write.

Fix this by reworking replace_impl to have less unsafe code. Now we zero
initialize the buffer, but we also prefer a stack buffer so we may save
an allocation - probably a wash overall.

Add a test for this case.
@ridiculousfish
Copy link
Member

Merged as 7d78cb0 - thank you!!

Note there was an OOB write in the overflow case - see 813a426

@henrikhorluck
Copy link
Author

Note there was an OOB write in the overflow case - see 813a426

oof, that's on me for not properly testing this, and misreading the documentation. I think I had too much faith in fish's match-tests, and wrongly assumed a larger replace was actually tested.

@henrikhorluck henrikhorluck deleted the feat/replace branch June 29, 2023 19:36
@henrikhorluck
Copy link
Author

@ridiculousfish I also think you merged this to the wrong repository, you merged it to https://github.com/ridiculousfish/rust-pcre2 instead of https://github.com/fish-shell/rust-pcre2

@henrikhorluck henrikhorluck restored the feat/replace branch June 29, 2023 19:38
@ridiculousfish
Copy link
Member

Whoops! Thanks, merged to https://github.com/fish-shell/rust-pcre2

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