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

Fix async engine pause dead lock in error case. #4020

Closed
wants to merge 1 commit into from

Conversation

EmericBr
Copy link
Contributor

@EmericBr EmericBr commented Jul 26, 2017

In 'crypto/rand/ossl_rand.c', a call to
'ASYNC_unblock_pause()' is missing in an error case.

This bug also affects 1.1.0f 'crypto/rand/md_rand.c'

CLA: trivial

Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 26, 2017
Copy link
Contributor

@kaduk kaduk left a comment

Choose a reason for hiding this comment

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

Formality: I agree this is trivial.
@EmericBr if you believe this change is trivial (not copyrightable) and do not wish to upload a CLA, you should amend the commit message to include a line (at the end) "CLA: trivial" and force-push.

In 'crypto/rand/ossl_rand.c', a call to
'ASYNC_unblock_pause()' is missing in an error case.

CLA: trivial
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

I agree it's trivial; just need @EmericBr to agree :)

@richsalz richsalz added 1.1.0 branch: master Merge to master branch labels Jul 26, 2017
@richsalz
Copy link
Contributor

(Please note that we mean "trivial" in the copyright sense. This is a real bug, and I am sure that it took some head-scratching [or -banging] to fix :)

@EmericBr
Copy link
Contributor Author

Hi,

Yes it is trivial, i just don't know how to amend "CLA: trivial" in a pull request.

R,
Emeric

@richsalz
Copy link
Contributor

Thanks! In your local branch do git commit --amend and add "CLA: trivial" to the commit text (not the title). And then push the updated commit by doing git push -f github
And then @kaduk can merge. :)

@EmericBr
Copy link
Contributor Author

According to what i see on my repo, it is already done:

haproxytech@799dfc7

@richsalz
Copy link
Contributor

Thanks again! (Our script has a bug :(

@richsalz richsalz removed the hold: cla required The contributor needs to submit a license agreement label Jul 26, 2017
@kaduk kaduk self-assigned this Jul 26, 2017
@kaduk kaduk added the approval: done This pull request has the required number of approvals label Jul 26, 2017
levitte pushed a commit that referenced this pull request Jul 26, 2017
In 'crypto/rand/ossl_rand.c', a call to
'ASYNC_unblock_pause()' is missing in an error case.

CLA: trivial

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #4020)
@kaduk kaduk removed the 1.1.0 label Jul 26, 2017
@kaduk
Copy link
Contributor

kaduk commented Jul 26, 2017

git cherry-pick was not willing to follow the md_rand rename, so removing the 1.1.0 label.

e4b1601 on master; closing

@kaduk kaduk closed this Jul 26, 2017
@richsalz
Copy link
Contributor

perhaps by hand? this seems like a bad bug.

@kaduk
Copy link
Contributor

kaduk commented Jul 26, 2017

Yeah, I've got it staged and am doing a build sanity-test before making a new PR.

kaduk pushed a commit to kaduk/openssl that referenced this pull request Jul 26, 2017
In 'crypto/rand/ossl_rand.c', a call to
'ASYNC_unblock_pause()' is missing in an error case.

CLA: trivial

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from openssl#4020)

(cherry picked from commit e4b1601)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants