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

[Hotfix] solution for issue with v1.8.7 #106

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Conversation

jbygdell
Copy link
Contributor

@jbygdell jbygdell commented Jan 8, 2024

v1.8.7 introduced a bug where decryption would deadlock if the available read buffer was 0. Causing the stream to read the same byte over and over again and never reaching the end of the file.

This PR fixes the issue found in 1.8.7 and adds a decryption test for decrypting an entire file.

To test this without having to wait for ever set the test timeout to 60s by adding `-timeout 60s` on the command line.
@jbygdell jbygdell added bug Something isn't working go Pull requests that update Go code labels Jan 8, 2024
@jbygdell jbygdell requested review from pontus, blankdots and a team January 8, 2024 09:53
@jbygdell jbygdell self-assigned this Jan 8, 2024
@pontus
Copy link
Contributor

pontus commented Jan 8, 2024

Thanks for the fix.

Having roughly the same code repeated for the different canRead cases looks a bit ugly to me, but we're in this mess because I didn't have them duplicated so I guess it's fine. (For reference; my fix that I didn't have time to fix proper tests for before was to add a check on buffer.Len() at the end of ensureBuffer and return io.EOF if it hadn't succeeded in ensuring the buffer had content to read, but either way is good I guess.

Copy link
Contributor

@pontus pontus left a comment

Choose a reason for hiding this comment

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

Looks good, see specific comment about stream position and return.

jbygdell and others added 2 commits January 10, 2024 10:45
The root caouse was that we didn't signal EOF once we read everything.
Kudos to Pontus that actually figured it out.

Co-authored-by: Pontus Freyhult <[email protected]>
@jbygdell jbygdell force-pushed the hotfix/prevent_lockup branch from 7541e61 to 65b1a14 Compare January 10, 2024 09:45
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1ce976c) 66.29% compared to head (65b1a14) 66.46%.
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   66.29%   66.46%   +0.17%     
==========================================
  Files           6        6              
  Lines        1178     1184       +6     
==========================================
+ Hits          781      787       +6     
  Misses        278      278              
  Partials      119      119              
Flag Coverage Δ
unittests 66.46% <100.00%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pontus pontus merged commit 8d167fe into master Jan 11, 2024
@pontus pontus deleted the hotfix/prevent_lockup branch January 11, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants