From 404931be47f6fa5c7ba2d59ecaac444e90c86113 Mon Sep 17 00:00:00 2001 From: Toddr Bot Date: Fri, 27 Mar 2026 00:02:09 +0000 Subject: [PATCH 1/3] fix: BIO resource leak in extractBioString() error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- RSA.xs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/RSA.xs b/RSA.xs index 6bf64b3..5b778ce 100644 --- a/RSA.xs +++ b/RSA.xs @@ -293,14 +293,21 @@ SV* extractBioString(BIO* p_stringBio) { SV* sv; char* datap; - long datasize = 0; + long datasize; - CHECK_OPEN_SSL(BIO_flush(p_stringBio) == 1); + if (BIO_flush(p_stringBio) != 1) { + BIO_free(p_stringBio); + croakSsl(__FILE__, __LINE__); + } datasize = BIO_get_mem_data(p_stringBio, &datap); + if (datasize < 0) { + BIO_free(p_stringBio); + croakSsl(__FILE__, __LINE__); + } sv = newSVpv(datap, datasize); - CHECK_OPEN_SSL(BIO_set_close(p_stringBio, BIO_CLOSE) == 1); + BIO_set_close(p_stringBio, BIO_CLOSE); BIO_free(p_stringBio); return sv; } From 4f2f0a1410d780bb9e3f295ddabfb12c499e9cae Mon Sep 17 00:00:00 2001 From: Toddr Bot Date: Fri, 3 Apr 2026 11:15:17 +0000 Subject: [PATCH 2/3] rebase: apply review feedback on #145 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()` --- RSA.xs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/RSA.xs b/RSA.xs index 5b778ce..fbd966c 100644 --- a/RSA.xs +++ b/RSA.xs @@ -294,22 +294,23 @@ SV* extractBioString(BIO* p_stringBio) SV* sv; char* datap; long datasize; + int error = 0; - if (BIO_flush(p_stringBio) != 1) { - BIO_free(p_stringBio); - croakSsl(__FILE__, __LINE__); - } + THROW(BIO_flush(p_stringBio) == 1); datasize = BIO_get_mem_data(p_stringBio, &datap); - if (datasize < 0) { - BIO_free(p_stringBio); - croakSsl(__FILE__, __LINE__); - } + THROW(datasize >= 0); + sv = newSVpv(datap, datasize); BIO_set_close(p_stringBio, BIO_CLOSE); BIO_free(p_stringBio); return sv; + + err: + BIO_free(p_stringBio); + CHECK_OPEN_SSL(0); + return NULL; /* unreachable, CHECK_OPEN_SSL croaks */ } EVP_PKEY* _load_rsa_key(SV* p_keyStringSv, From 67513ea4543003187fa60241a3abb2d0a6c68922 Mon Sep 17 00:00:00 2001 From: Toddr Bot Date: Fri, 3 Apr 2026 13:57:40 +0000 Subject: [PATCH 3/3] rebase: apply review feedback on #145 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) --- RSA.xs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RSA.xs b/RSA.xs index fbd966c..270f17c 100644 --- a/RSA.xs +++ b/RSA.xs @@ -299,7 +299,7 @@ SV* extractBioString(BIO* p_stringBio) THROW(BIO_flush(p_stringBio) == 1); datasize = BIO_get_mem_data(p_stringBio, &datap); - THROW(datasize >= 0); + THROW(datasize > 0); sv = newSVpv(datap, datasize);