Skip to content

Free data if sk_OPENSSL_STRING_push fails.#26227

Closed
fwh-dc wants to merge 2 commits into
openssl:masterfrom
fwh-dc:fix-26203
Closed

Free data if sk_OPENSSL_STRING_push fails.#26227
fwh-dc wants to merge 2 commits into
openssl:masterfrom
fwh-dc:fix-26203

Conversation

@fwh-dc
Copy link
Copy Markdown
Contributor

@fwh-dc fwh-dc commented Dec 20, 2024

Fixes #26203

Checked additional calls to sk_OPENSSL_STRING_push() and found another case. A reviewer could do the same task and verify that I didn't miss another case.

@t8m
Copy link
Copy Markdown
Member

t8m commented Dec 20, 2024

Please note that sk__push will return -1 when sk is NULL on versions earlier than 3.3.0.

@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) tests: exempted The PR is exempt from requirements for testing branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 (EOL) branch: 3.4 Applies to openssl-3.4 labels Dec 20, 2024
@fwh-dc
Copy link
Copy Markdown
Contributor Author

fwh-dc commented Dec 20, 2024 •
edited
Loading

Please note that sk__push will return -1 when sk is NULL on versions earlier than 3.3.0.

Should I modify the check to != 1? or make a change that targets the earlier versions?

@fwh-dc fwh-dc mentioned this pull request Dec 21, 2024
Closed
@paulidale
Copy link
Copy Markdown
Contributor

paulidale commented Dec 30, 2024

Should I modify the check to != 1? or make a change that targets the earlier versions?

I'd suggest <= 0 for all versions.

@fwh-dc
Copy link
Copy Markdown
Contributor Author

fwh-dc commented Dec 30, 2024

Should I modify the check to != 1? or make a change that targets the earlier versions?

I'd suggest <= 0 for all versions.

Done.

@fwh-dc fwh-dc mentioned this pull request Jan 1, 2025
Closed
@fwh-dc fwh-dc requested review from paulidale and t8m January 3, 2025 20:27
@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 Jan 6, 2025
Sashan
Sashan approved these changes Jan 6, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@Sashan Sashan 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 to me. thanks.

@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 7, 2025
@openssl-machine
Copy link
Copy Markdown
Collaborator

openssl-machine commented Jan 7, 2025

This pull request is ready to merge

@t8m t8m removed branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) branch: 3.2 Applies to openssl-3.2 (EOL) labels Jan 8, 2025
@t8m t8m removed the branch: 3.3 Applies to openssl-3.3 (EOL) label Jan 8, 2025
openssl-machine pushed a commit that referenced this pull request Jan 8, 2025
@fwh-dc @t8m
Free data if sk_OPENSSL_STRING_push fails.
42fe40b 
Fixes #26203

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #26227)

(cherry picked from commit 2457fc4)
openssl-machine pushed a commit that referenced this pull request Jan 8, 2025
@fwh-dc @t8m
Free data if sk_OPENSSL_STRING_push fails.
2457fc4 
Fixes #26203

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #26227)
@t8m
Copy link
Copy Markdown
Member

t8m commented Jan 8, 2025

Merged to the master and 3.4 branches. Does not apply cleanly to older branches but not important enough for backporting IMO.
Thank you for your contribution.

@t8m t8m closed this Jan 8, 2025
Sashan pushed a commit to Sashan/openssl that referenced this pull request Apr 23, 2025
@fwh-dc @Sashan
Free data if sk_OPENSSL_STRING_push fails.
ffa2f17 
Fixes openssl#26203

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#26227)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Sashan Sashan Sashan approved these changes

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.4 Applies to openssl-3.4 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

There is a memory leak defect at line 125 in the file /openssl/crypto/x509/by_store.c.

5 participants

@fwh-dc @t8m @paulidale @openssl-machine @Sashan