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

Set error if malloc returns NULL #5842

Closed
wants to merge 1 commit into from
Closed

Set error if malloc returns NULL #5842

wants to merge 1 commit into from

Conversation

richsalz
Copy link
Contributor

@richsalz richsalz commented Apr 2, 2018

This is a partial fix for #5841. I hope @FdaSilvaYY will finish off all the x509 onces as part of his PR #5837.

I am not going to backport:

  • Malloc failures will often result in program crashes anyway.
  • The error stuff is very different for 1.1.1
  • 1.0.2 is almost out of LTS lifetime
  • Few will stay on 1.1.0 when 1.1.1 and TLS 1.3 comes out
  • blah blah blah

@richsalz richsalz added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Apr 2, 2018
@richsalz richsalz self-assigned this Apr 2, 2018
@mspncp
Copy link
Contributor

mspncp commented Apr 2, 2018

I like your last argument 😉.

Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

Certainly the most boring and tiresome pull request I ever reviewed. Thank you very much for shouldering this hard diligence work (german: "Fleißarbeit")!

@mspncp
Copy link
Contributor

mspncp commented Apr 2, 2018

You will squash this all into a single commit, right?

@mspncp mspncp added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 2, 2018
@mspncp mspncp added this to the 1.1.1 milestone Apr 2, 2018
st->data = OPENSSL_zalloc(sizeof(void *) * num_alloc);
if (st->data == NULL)
if ((st->data = OPENSSL_zalloc(sizeof(void *) * num_alloc)) == NULL) {
/* STACKerr(STACK_F_SK_RESERVE, ERR_R_MALLOC_FAILURE); */
Copy link
Contributor

Choose a reason for hiding this comment

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

Errors from stack are reported by caller and stack user...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I say that this code is useless:
it has no value added, it just says that one of the many stack is KO, ... and please guess which one ;)
Actually, the current usage is to report this kind of failure just from the caller code, just after the failed call .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, fine-grain "what went wrong" and then pushing more errors as we pop up the stack is the way we do things.

@richsalz
Copy link
Contributor Author

richsalz commented Apr 3, 2018

Yes I will squash it before merging. Will wait a bit for @FdaSilvaYY to clarify his comment tho.

Almost all *alloc failures now set an error code.
@richsalz
Copy link
Contributor Author

richsalz commented Apr 3, 2018

Thanks for boring tolerance :)

@richsalz richsalz closed this Apr 3, 2018
@richsalz richsalz deleted the ssl-null-check branch April 3, 2018 15:51
levitte pushed a commit that referenced this pull request Apr 3, 2018
Almost all *alloc failures now set an error code.

Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #5842)
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.

3 participants