optimize sol derivation path allocation
What the developer wrote
Authored by ww3512687
optimize sol derivation path allocation
What changed, and why it matters
This commit refactors how a Solana cryptocurrency derivation path is stored while parsing a USB request. Previously, the code dynamically allocated a small heap buffer inside a helper function and returned it to the caller, which then had to free it. Now the caller provides a fixed-size stack buffer, and the helper just fills it. The change removes a potential memory leak and a use-after-free risk if the caller forgot to free the buffer, and it avoids relying on dynamic allocation for a small fixed-size string. There is no direct evidence in the commit of an exploitable vulnerability being fixed, but the change is a defensive hardening improvement.
Treat as a hardening/cleanup change rather than an urgent security fix. Review the surrounding USB protocol handler for additional memory-management issues and ensure BUFFER_SIZE_32 remains sufficient for all supported Solana derivation depths. Consider adding explicit truncation checks on snprintf return values for defense in depth.
Security signals we found
Eliminates heap allocation for a fixed-size derivation-path buffer
Removes manual free responsibility from caller, reducing memory leak / use-after-free risk
Switches helper return type from pointer to bool to enforce explicit success/failure handling
No new input validation or length checks added beyond existing depth/component checks
Evidence from the diff
The patch changes ParseSolDerivationPath from returning a heap-allocated char* (via SRAM_MALLOC(BUFFER_SIZE_32)) to writing into a caller-supplied buffer with a caller-supplied size. The caller, GetDeviceUsbPubkeyService, now declares char path[BUFFER_SIZE_32] on the stack and passes it along with sizeof(path). Error paths inside the helper no longer need to SRAM_FREE(path) before returning, and the cleanup block no longer frees it. The helper still validates depth and hardened components. The snprintf calls now use pathSize instead of BUFFER_SIZE_32, which is correct because the buffer is exactly that size. This eliminates a dynamic allocation failure path and reduces lifetime-management bugs, but the commit does not show any additional bounds checking beyond what the fixed size already provided.
Changed components
src/webusb_protocol/general/eapdu_services/service_trans_usb_pubkey.cParseSolDerivationPathGetDeviceUsbPubkeyServiceInspect captured patch +12 / −20
diff --git a/src/webusb_protocol/general/eapdu_services/service_trans_usb_pubkey.c b/src/webusb_protocol/general/eapdu_services/service_trans_usb_pubkey.c
index cd81095..bbee0e5 100644
--- a/src/webusb_protocol/general/eapdu_services/service_trans_usb_pubkey.c
+++ b/src/webusb_protocol/general/eapdu_services/service_trans_usb_pubkey.c
@@ -25,22 +25,17 @@ static bool ParseCoinType(const uint8_t *data, uint32_t len, uint32_t *coinType)
return true;
}
-static char *ParseSolDerivationPath(const uint8_t *data, uint32_t len)
+static bool ParseSolDerivationPath(const uint8_t *data, uint32_t len, char *path, size_t pathSize)
{
- if (data == NULL || len < 1) {
- return NULL;
+ if (data == NULL || path == NULL || pathSize == 0 || len < 1) {
+ return false;
}
uint8_t depth = data[0];
uint32_t expectedLen = 1 + (depth * 4);
if (len < expectedLen) {
- return NULL;
- }
-
- char *path = (char *)SRAM_MALLOC(BUFFER_SIZE_32);
- if (path == NULL) {
- return NULL;
+ return false;
}
path[0] = '\0';
@@ -55,19 +50,18 @@ static char *ParseSolDerivationPath(const uint8_t *data, uint32_t len)
bool isHardened = (component & 0x80000000) != 0;
if (!isHardened) {
- SRAM_FREE(path);
- return NULL;
+ return false;
}
component &= 0x7FFFFFFF;
if (strlen(path) == 0) {
- snprintf(path, BUFFER_SIZE_32, "%u'", component);
+ snprintf(path, pathSize, "%u'", component);
} else {
- snprintf(path + strlen(path), BUFFER_SIZE_32 - strlen(path), "/%u'", component);
+ snprintf(path + strlen(path), pathSize - strlen(path), "/%u'", component);
}
}
- return path;
+ return true;
}
void GetDeviceUsbPubkeyService(EAPDURequestPayload_t *payload)
@@ -76,7 +70,7 @@ void GetDeviceUsbPubkeyService(EAPDURequestPayload_t *payload)
cJSON *root = NULL;
char *json_str = NULL;
uint32_t coinType = 0;
- char *path = NULL;
+ char path[BUFFER_SIZE_32] = {0};
char *pubKey = NULL;
result = (EAPDUResponsePayload_t *)SRAM_MALLOC(sizeof(EAPDUResponsePayload_t));
@@ -101,8 +95,9 @@ void GetDeviceUsbPubkeyService(EAPDURequestPayload_t *payload)
goto create_response;
}
- path = ParseSolDerivationPath(payload->data + COIN_TYPE_SIZE, payload->dataLen - COIN_TYPE_SIZE);
- if (path == NULL) {
+ if (!ParseSolDerivationPath(payload->data + COIN_TYPE_SIZE,
+ payload->dataLen - COIN_TYPE_SIZE,
+ path, sizeof(path))) {
cJSON_AddStringToObject(root, "error", "Failed to parse derivation path");
goto create_response;
}
@@ -146,9 +141,6 @@ cleanup:
if (root != NULL) {
cJSON_Delete(root);
}
- if (path != NULL) {
- SRAM_FREE(path);
- }
if (result != NULL) {
SRAM_FREE(result);
}
Why this scored 33/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.