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

Remove unsafe constructs #3

Merged
merged 8 commits into from
Nov 26, 2019
Merged

Remove unsafe constructs #3

merged 8 commits into from
Nov 26, 2019

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Nov 24, 2019

qbsdiff uses unsafe code in various places to build uninitialised Vec<u8>s with a given size.
Unfortunately, this probably is undefined behaviour, so it should be removed.

  • bsdiff
    • use of unsafe removed;
    • no additional writes or allocations introduced;
    • unlike previously, it is now clear that no data leaks between successive (re)uses of the buffer.
  • bspatch:
    • Context::{buf,dlt} caches are now managed safely.
    • Some overhead is involved as they are initialized with 0-valued bytes when created and resized.
  • unsafe code is now forbidden within the crate.

@nbraud nbraud changed the title WiP: Remove unsafe constructs Remove unsafe constructs Nov 25, 2019
@nbraud
Copy link
Contributor Author

nbraud commented Nov 25, 2019

@hucsmn This should now be mergeable.
I expect the overhead in bspatch to be very slight, but there is nothing currently in place to measure the crate's performance.

Copy link
Owner

@hucsmn hucsmn left a comment

Choose a reason for hiding this comment

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

Thanks for your commits! Uninitialized vector may lead to UB if elements were readed before initialization. This is a premature optimization indeed, I would like to add some benchmarks later. For now, safe code is better.
Following codes could be simplified:

src/bspatch.rs Outdated Show resolved Hide resolved
src/bspatch.rs Outdated Show resolved Hide resolved
@nbraud
Copy link
Contributor Author

nbraud commented Nov 26, 2019

Done, thanks for the feedback <3

@hucsmn hucsmn merged commit 6cb4a41 into hucsmn:master Nov 26, 2019
@nbraud nbraud deleted the safety-dance branch November 26, 2019 10:19
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.

3 participants