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 error handling in lhash contract #22154

Conversation

bernd-edlinger
Copy link
Member

When the realloc fails in contract, this not a fatal error, since the memory is only shrinked. It is also no option to exit the function at this point, since that would leave the hash table in an inconsistent state.

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

When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.
@bernd-edlinger bernd-edlinger added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Sep 20, 2023
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 20, 2023
@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing and removed approval: otc review pending This pull request needs review by an OTC member labels Sep 20, 2023
@@ -260,12 +260,12 @@ static void contract(OPENSSL_LHASH *lh)
if (n == NULL) {
/* fputs("realloc error in lhash", stderr); */
lh->error++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we even count this as an error condition?

@paulidale paulidale 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 Sep 21, 2023
@bernd-edlinger
Copy link
Member Author

Merged to all branches Thanks!.

openssl-machine pushed a commit that referenced this pull request Sep 21, 2023
When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22154)

(cherry picked from commit 5fbfd64)
openssl-machine pushed a commit that referenced this pull request Sep 21, 2023
When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22154)
openssl-machine pushed a commit that referenced this pull request Sep 21, 2023
When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22154)

(cherry picked from commit 5fbfd64)
bernd-edlinger added a commit to bernd-edlinger/openssl that referenced this pull request Sep 21, 2023
When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#22154)

(cherry picked from commit 5fbfd64)
@JerryDevis
Copy link
Contributor

JerryDevis commented Sep 22, 2023

since that would leave the hash table in an inconsistent state.

What kind of inconsistency will it lead to and what is the impact? @bernd-edlinger

@bernd-edlinger
Copy link
Member Author

The hash tree element list in np is lost when this function is aborted at this point,
due to the statement above with the eay - weird but comment:

np = lh->b[lh->p + lh->pmax - 1];
lh->b[lh->p + lh->pmax - 1] = NULL; /* 24/07-92 - eay - weird but :-( */

@bernd-edlinger
Copy link
Member Author

so it is more or less a memory leak + impossible to find some elements in the hash list that should be there.

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22154)

(cherry picked from commit 5fbfd64)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22154)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
When the realloc fails in contract, this not a fatal
error, since the memory is only shrinked. It is also no
option to exit the function at this point, since that
would leave the hash table in an inconsistent state.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22154)

(cherry picked from commit 5fbfd64)
Signed-off-by: fly2x <fly2x@hitls.org>
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 branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants