fix: BIO resource leak in extractBioString() error paths#145
Conversation
timlegge
left a comment
timlegge
left a comment
There was a problem hiding this comment.
Looks fine prevents memory leak in error case
|
@toddr-bot rebase |
Rebase: fix: BIO resource leak in extractBioString() error pathsBranch Diff: 1 file changed, 10 insertions(+), 3 deletions(-) Review feedback was analyzed and applied. Actions
CICI will be checked asynchronously. Automated by Kōan |
toddr-bot
force-pushed
the
koan.toddr.bot/fix-extractbiostring-leak
branch
from
835febe to
5562c11
Compare
|
view comment from #145 (comment) |
Refactored `extractBioString()` to use the `THROW`/`goto err` cleanup pattern per reviewer request: - Replaced duplicated `BIO_free(); croakSsl()` error handling in two error paths with `THROW()` macro + centralized `err:` cleanup label - This follows the same pattern used by `rsa_crypt()`, `sign()`, `verify()`, `generate_key()`, and `_new_key_from_params()` throughout `RSA.xs` - Added `int error = 0` (required by the `THROW` macro) - Retained the `BIO_get_mem_data() < 0` guard and the unchecked `BIO_set_close()` from the original fix - The `err:` label frees the BIO then calls `CHECK_OPEN_SSL(0)` which croaks with the OpenSSL error, matching the idiom in `rsa_crypt()`
Rebase: fix: BIO resource leak in extractBioString() error pathsBranch Diff: 1 file changed, 11 insertions(+), 3 deletions(-) Review feedback was analyzed and applied. ChangesRefactored
Actions
CICI will be checked asynchronously. Automated by Kōan |
| if (datasize < 0) { | ||
| BIO_free(p_stringBio); | ||
| croakSsl(__FILE__, __LINE__); | ||
| } |
There was a problem hiding this comment.
Could probably be simplified to read by using a macro for BIO_free & croakSsl so we do avoid introducing boilerplate and complexity to read / maintain
|
@timlegge please review one more time after that small refactor to make the code easier to read / maintain using a shared block for the exception |
|
|
||
| datasize = BIO_get_mem_data(p_stringBio, &datap); | ||
| THROW(datasize >= 0); | ||
|
|
There was a problem hiding this comment.
strictly speaking this should be > 0 - 0 is returned if p_stringBio is null but 299 should fail in that case
| sv = newSVpv(datap, datasize); | ||
|
|
||
| CHECK_OPEN_SSL(BIO_set_close(p_stringBio, BIO_CLOSE) == 1); | ||
| BIO_set_close(p_stringBio, BIO_CLOSE); |
There was a problem hiding this comment.
We lost the success check here - should wrap probably it in a throw but the p_stringBio was successfully set and the BIO is freed next so likely okay
|
View comment from #145 (comment) and adjust accordingly |
CHECK_OPEN_SSL calls croakSsl() which longjmps, skipping BIO_free(). Replace with explicit error checks that free the BIO before croaking. Also add a guard on BIO_get_mem_data() return value — a negative return would be misinterpreted as a huge size_t by newSVpv(). The BIO_set_close() check is dropped since it just sets a flag before the immediately following BIO_free(), and cannot meaningfully fail on a mem BIO. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactored `extractBioString()` to use the `THROW`/`goto err` cleanup pattern per reviewer request: - Replaced duplicated `BIO_free(); croakSsl()` error handling in two error paths with `THROW()` macro + centralized `err:` cleanup label - This follows the same pattern used by `rsa_crypt()`, `sign()`, `verify()`, `generate_key()`, and `_new_key_from_params()` throughout `RSA.xs` - Added `int error = 0` (required by the `THROW` macro) - Retained the `BIO_get_mem_data() < 0` guard and the unchecked `BIO_set_close()` from the original fix - The `err:` label frees the BIO then calls `CHECK_OPEN_SSL(0)` which croaks with the OpenSSL error, matching the idiom in `rsa_crypt()`
Done. Here's the summary for the commit message: - Changed `BIO_get_mem_data()` guard from `datasize >= 0` to `datasize > 0` per @timlegge's review: `BIO_get_mem_data()` returns 0 when the BIO is NULL, which should be treated as an error (the NULL case would already be caught by the `BIO_flush()` check above, but `> 0` is strictly more correct)
Rebase: fix: BIO resource leak in extractBioString() error pathsBranch Diff: 1 file changed, 11 insertions(+), 3 deletions(-) Review feedback was analyzed and applied. ChangesDone. Here's the summary for the commit message:
Actions
CICI will be checked asynchronously. Automated by Kōan |
toddr-bot
force-pushed
the
koan.toddr.bot/fix-extractbiostring-leak
branch
from
6c6bc97 to
67513ea
Compare
3 participants
What
Fix BIO memory leak in
extractBioString()whenBIO_flush()orBIO_get_mem_data()fails.Why
CHECK_OPEN_SSLcallscroakSsl()which longjmps out of the function, skipping theBIO_free()at the end. This is the same pattern that was fixed inrsa_crypt(),sign(),verify(), andgenerate_key()—extractBioString()was the last holdout.Also,
BIO_get_mem_data()can return -1 on error. When passed assize_ttonewSVpv(), this would be reinterpreted as a huge positive value.How
CHECK_OPEN_SSLwith explicit checks thatBIO_free()beforecroakSsl()BIO_get_mem_data()return valueBIO_set_close()error check — it just sets a flag before the immediately followingBIO_free()and cannot meaningfully fail on a mem BIOTesting
All 507 tests pass on OpenSSL 3.5.1.
🤖 Generated with Claude Code
Quality Report
Changes: 1 file changed, 10 insertions(+), 3 deletions(-)
Code scan: clean
Tests: passed (OK)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline