diff options
| author | David Benjamin <davidben@chromium.org> | 2014-11-03 06:22:22 +0300 |
|---|---|---|
| committer | Adam Langley <agl@google.com> | 2014-11-06 04:44:43 +0300 |
| commit | a85093f5bb3e0595fd5e059cb5152df3e20d5c7d (patch) | |
| tree | 545cecc4d0cbb3d7f071c83f653a060d5f6fad33 /crypto/x509 | |
| parent | ab2815eaff6219ef57aedca2f7b1b72333c27fd0 (diff) | |
Fix error handling in X509_PURPOSE_add and X509_TRUST_add.
Two leaks can happen: if idx is -1, the newly allocated entry may not be freed.
Also, for X509_PURPOSE_add, if only one BUF_strdup succeeds, it will leak.
Restructure both so that the allocations happen ahead of time and are properly
cleaned up. This avoids leaving an existing entry in a half-broken state.
Found (sort of) by scan-build; because of all the indirections and DYNAMIC
flags, it doesn't actually realize the leak's been fixed.
Change-Id: I5521889bd14e007b3f62b6a4906d7c346698b48c
Reviewed-on: https://boringssl-review.googlesource.com/2209
Reviewed-by: Adam Langley <agl@google.com>
Diffstat (limited to 'crypto/x509')
| -rw-r--r-- | crypto/x509/x509_trs.c | 17 |
1 files changed, 13 insertions, 4 deletions
diff --git a/crypto/x509/x509_trs.c b/crypto/x509/x509_trs.c index 1a8ed49a..9b7cc9cf 100644 --- a/crypto/x509/x509_trs.c +++ b/crypto/x509/x509_trs.c @@ -168,6 +168,8 @@ int X509_TRUST_add(int id, int flags, int (*ck)(X509_TRUST *, X509 *, int), { int idx; X509_TRUST *trtmp; + char *name_dup; + /* This is set according to what we change: application can't set it */ flags &= ~X509_TRUST_DYNAMIC; /* This will always be set for application modified trust entries */ @@ -183,13 +185,18 @@ int X509_TRUST_add(int id, int flags, int (*ck)(X509_TRUST *, X509 *, int), trtmp->flags = X509_TRUST_DYNAMIC; } else trtmp = X509_TRUST_get0(idx); - /* OPENSSL_free existing name if dynamic */ - if(trtmp->flags & X509_TRUST_DYNAMIC_NAME) OPENSSL_free(trtmp->name); - /* dup supplied name */ - if(!(trtmp->name = BUF_strdup(name))) { + /* Duplicate the supplied name. */ + name_dup = BUF_strdup(name); + if (name_dup == NULL) { OPENSSL_PUT_ERROR(X509, X509_TRUST_add, ERR_R_MALLOC_FAILURE); + if (idx == -1) + OPENSSL_free(trtmp); return 0; } + + /* OPENSSL_free existing name if dynamic */ + if (trtmp->flags & X509_TRUST_DYNAMIC_NAME) OPENSSL_free(trtmp->name); + trtmp->name = name_dup; /* Keep the dynamic flag of existing entry */ trtmp->flags &= X509_TRUST_DYNAMIC; /* Set all other flags */ @@ -204,10 +211,12 @@ int X509_TRUST_add(int id, int flags, int (*ck)(X509_TRUST *, X509 *, int), if(idx == -1) { if(!trtable && !(trtable = sk_X509_TRUST_new(tr_cmp))) { OPENSSL_PUT_ERROR(X509, X509_TRUST_add, ERR_R_MALLOC_FAILURE); + trtable_free(trtmp); return 0; } if (!sk_X509_TRUST_push(trtable, trtmp)) { OPENSSL_PUT_ERROR(X509, X509_TRUST_add, ERR_R_MALLOC_FAILURE); + trtable_free(trtmp); return 0; } } |