Skip to content

shred: remove usage of Vec::set_len() #1738

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 3 commits into from
Feb 23, 2021
Merged

Conversation

Arcterus
Copy link
Collaborator

@Arcterus Arcterus commented Feb 19, 2021

This PR removes usage of Vec::set_len() (#1729). It also avoids allocating a new Vec every time BytesGenerator iterates, so it is most likely actually faster than before. Note that there is still quite a bit of room for improvement in terms of performance (e.g. copying entire patterns rather than pushing them to the Vec one byte at a time) and readability.

There is at least one more instance of Vec::set_len() that needs to be fixed (it's in od again). We may want to simply review our usage of unsafe in general, as I imagine some of the cruft that's built up over the years can be removed.

@Arcterus Arcterus added U - shred security Pull requests that address a security vulnerability labels Feb 19, 2021
@Arcterus
Copy link
Collaborator Author

Also, I noticed that this utility has no tests. I'm not sure I'll bother adding them in this PR, so I'll open up an issue later.

@Arcterus
Copy link
Collaborator Author

You may not want to merge this yet. I'm thinking about just trying to remove the allocation entirely since the max size is 512 bytes (with the way the code is currently set up).

@Arcterus
Copy link
Collaborator Author

The latest commit removes all Vec allocation in favor of using a fixed-size array and simply returning a slice with the latest block of bytes. I also took the liberty to convert the generation for pattern-based passes to copy the entire pattern at once rather than push one byte at a time.

@Arcterus
Copy link
Collaborator Author

Btw @sylvestre lmk if you’d prefer I rebase this, seeing as my third commit basically wiped out the changes from the first.

@sylvestre
Copy link
Contributor

Nah, we can use the squash feature of github :)

@sylvestre sylvestre merged commit 7341a1a into uutils:master Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability U - shred
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants