Topic: Memory Leak? CKey::SetCompactSignature (Read 850 times)
Thanks
I created a pull request with this patch in it.
Indeed, looks that way, good catch.
You should really create this as a github issue so that it doesn't get lost in the avalanche here.
You should really create this as a github issue so that it doesn't get lost in the avalanche here.
bool CKey::SetCompactSignature(uint256 hash, const std::vector& vchSig)
{
if (vchSig.size() != 65)
return false;
int nV = vchSig[0];
if (nV<27 || nV>=35)
return false;
ECDSA_SIG *sig = ECDSA_SIG_new();
BN_bin2bn(&vchSig[1],32,sig->r);
BN_bin2bn(&vchSig[33],32,sig->s);
EC_KEY_free(pkey);
pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
if (nV >= 31)
{
SetCompressedPubKey();
nV -= 4;
}
if (ECDSA_SIG_recover_key_GFp(pkey, sig, (unsigned char*)&hash, sizeof(hash), nV - 27, 0) == 1)
{
fSet = true;
ECDSA_SIG_free(sig);
return true;
}
ECDSA_SIG_free(sig); // Should this be added to fix leak?
return false;
}
{
if (vchSig.size() != 65)
return false;
int nV = vchSig[0];
if (nV<27 || nV>=35)
return false;
ECDSA_SIG *sig = ECDSA_SIG_new();
BN_bin2bn(&vchSig[1],32,sig->r);
BN_bin2bn(&vchSig[33],32,sig->s);
EC_KEY_free(pkey);
pkey = EC_KEY_new_by_curve_name(NID_secp256k1);
if (nV >= 31)
{
SetCompressedPubKey();
nV -= 4;
}
if (ECDSA_SIG_recover_key_GFp(pkey, sig, (unsigned char*)&hash, sizeof(hash), nV - 27, 0) == 1)
{
fSet = true;
ECDSA_SIG_free(sig);
return true;
}
ECDSA_SIG_free(sig); // Should this be added to fix leak?
return false;
}
In CKey::SetCompactSignature line 370 of key.cpp I believe a call to ECDSA_SIG_free(sig) is needed.
If ECDSA_SIG_recover_key_GFp fails, then ECDSA_SIG_free() is never called on the ECDSA_SIG_new() allocated on line 353.
I suspect that this doesn't fail very often, but could be an attack vector.
Not being an expert in the OpenSSL API I am not sure if this memory is freed someplace else.
If ECDSA_SIG_recover_key_GFp fails, then ECDSA_SIG_free() is never called on the ECDSA_SIG_new() allocated on line 353.
I suspect that this doesn't fail very often, but could be an attack vector.
Not being an expert in the OpenSSL API I am not sure if this memory is freed someplace else.
Jump to: