Miod Vallat <[email protected]> writes:
> ... or, in other words, try to fix most memory leak upon failure.
> This kind of change is difficult to test, the more eyes reviewing it,
> the better.
Well, I'll try to take a stab at it then.
>
> Miod
>
> Index: a_gentm.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_gentm.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 a_gentm.c
> --- a_gentm.c 19 Apr 2014 11:43:07 -0000 1.17
> +++ a_gentm.c 14 May 2014 21:18:03 -0000
> @@ -212,41 +212,48 @@ ASN1_GENERALIZEDTIME *
> ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, time_t t, int offset_day,
> long offset_sec)
> {
> + ASN1_GENERALIZEDTIME *ret;
> char *p;
> struct tm *ts;
> struct tm data;
> size_t len = 20;
>
> - if (s == NULL)
> - s = M_ASN1_GENERALIZEDTIME_new();
> - if (s == NULL)
> - return (NULL);
> + if (s == NULL) {
> + ret = M_ASN1_GENERALIZEDTIME_new();
> + if (ret == NULL)
> + return (NULL);
> + } else
> + ret = s;
>
> ts = gmtime_r(&t, &data);
> if (ts == NULL)
> - return (NULL);
> + goto err;
>
> if (offset_day || offset_sec) {
> if (!OPENSSL_gmtime_adj(ts, offset_day, offset_sec))
> - return NULL;
> + goto err;
> }
>
> - p = (char *)s->data;
> - if ((p == NULL) || ((size_t)s->length < len)) {
> + p = (char *)ret->data;
> + if ((p == NULL) || ((size_t)ret->length < len)) {
> p = malloc(len);
> if (p == NULL) {
> ASN1err(ASN1_F_ASN1_GENERALIZEDTIME_ADJ,
> ERR_R_MALLOC_FAILURE);
> - return (NULL);
> + goto err;
> }
> - if (s->data != NULL)
> - free(s->data);
> - s->data = (unsigned char *)p;
> + if (ret->data != NULL)
> + free(ret->data);
> + ret->data = (unsigned char *)p;
> }
You might be able to use ASN1_STRING_set(ret, NULL, len) to handle
resizing / allocating the string the same way that it's used in
ASN1_TIME_to_generalizedtime(). Alternately, you might want to at least
use 'p = realloc(ret->data, len)' instead of malloc + free.
>
> snprintf(p, len, "%04d%02d%02d%02d%02d%02dZ", ts->tm_year + 1900,
> ts->tm_mon + 1, ts->tm_mday, ts->tm_hour, ts->tm_min, ts->tm_sec);
> - s->length = strlen(p);
> - s->type = V_ASN1_GENERALIZEDTIME;
> - return (s);
> + ret->length = strlen(p);
> + ret->type = V_ASN1_GENERALIZEDTIME;
> + return (ret);
> +err:
> + if (ret != s)
> + M_ASN1_GENERALIZEDTIME_free(ret);
> + return NULL;
> }
> Index: a_time.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_time.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 a_time.c
> --- a_time.c 21 Apr 2014 00:52:00 -0000 1.17
> +++ a_time.c 14 May 2014 21:18:03 -0000
> @@ -123,7 +123,7 @@ ASN1_TIME_check(ASN1_TIME *t)
> ASN1_GENERALIZEDTIME *
> ASN1_TIME_to_generalizedtime(ASN1_TIME *t, ASN1_GENERALIZEDTIME **out)
> {
> - ASN1_GENERALIZEDTIME *ret;
> + ASN1_GENERALIZEDTIME *ret = NULL;
> char *str;
> int newlen;
> int i;
> @@ -132,33 +132,41 @@ ASN1_TIME_to_generalizedtime(ASN1_TIME *
> return NULL;
>
> if (!out || !*out) {
> - if (!(ret = ASN1_GENERALIZEDTIME_new ()))
> + if (!(ret = ASN1_GENERALIZEDTIME_new()))
> return NULL;
> - if (out)
> - *out = ret;
> } else
> ret = *out;
>
> /* If already GeneralizedTime just copy across */
> if (t->type == V_ASN1_GENERALIZEDTIME) {
> if (!ASN1_STRING_set(ret, t->data, t->length))
> - return NULL;
> - return ret;
> + goto err;
> + goto done;
> }
>
> /* grow the string */
> if (!ASN1_STRING_set(ret, NULL, t->length + 2))
> - return NULL;
> + goto err;
> /* ASN1_STRING_set() allocated 'len + 1' bytes. */
> newlen = t->length + 2 + 1;
> str = (char *)ret->data;
> + /* XXX ASN1_TIME is not Y2050 compatible */
> i = snprintf(str, newlen, "%s%s", (t->data[0] >= '5') ? "19" : "20",
> (char *) t->data);
> if (i == -1 || i >= newlen) {
> - ASN1_STRING_free(ret);
> + M_ASN1_GENERALIZEDTIME_free(ret);
> + if (out && *out == ret)
> + *out = NULL;
> return NULL;
> }
The fact that *out can be released here seems a bit unexpected, but I
can't figure out how to get i >= newlen for this case anyway.
> +done:
> + if (out)
> + *out = ret;
> return ret;
> +err:
> + if (out != NULL && ret != *out)
> + ASN1_GENERALIZEDTIME_free(ret);
> + return NULL;
> }
>
> int
> Index: a_utctm.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_utctm.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 a_utctm.c
> --- a_utctm.c 21 Apr 2014 11:23:09 -0000 1.22
> +++ a_utctm.c 14 May 2014 21:18:03 -0000
> @@ -152,34 +152,37 @@ ASN1_UTCTIME_set(ASN1_UTCTIME *s, time_t
> ASN1_UTCTIME *
> ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t t, int offset_day, long offset_sec)
> {
> + ASN1_UTCTIME *ret;
> char *p;
> struct tm *ts;
> struct tm data;
> size_t len = 20;
>
> - if (s == NULL)
> - s = M_ASN1_UTCTIME_new();
> - if (s == NULL)
> - return (NULL);
> + if (s == NULL) {
> + ret = M_ASN1_UTCTIME_new();
> + if (ret == NULL)
> + return (NULL);
> + } else
> + ret = s;
Looks like there's a bit of an oversight here -- ret isn't referred to
after this point in the function.
>
> ts = gmtime_r(&t, &data);
> if (ts == NULL)
> - return (NULL);
> + goto err;
>
> if (offset_day || offset_sec) {
> if (!OPENSSL_gmtime_adj(ts, offset_day, offset_sec))
> - return NULL;
> + goto err;
> }
>
> if ((ts->tm_year < 50) || (ts->tm_year >= 150))
> - return NULL;
> + goto err;
>
> p = (char *)s->data;
> if ((p == NULL) || ((size_t)s->length < len)) {
> p = malloc(len);
> if (p == NULL) {
> ASN1err(ASN1_F_ASN1_UTCTIME_ADJ, ERR_R_MALLOC_FAILURE);
> - return (NULL);
> + goto err;
> }
> if (s->data != NULL)
> free(s->data);
> @@ -191,6 +194,10 @@ ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t
> s->length = strlen(p);
> s->type = V_ASN1_UTCTIME;
> return (s);
> +err:
> + if (ret != s)
> + M_ASN1_UTCTIME_free(ret);
> + return NULL;
> }
>
> int
> Index: asn_mime.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/crypto/asn1/asn_mime.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 asn_mime.c
> --- asn_mime.c 26 Apr 2014 18:56:37 -0000 1.13
> +++ asn_mime.c 14 May 2014 21:18:03 -0000
> @@ -825,11 +825,12 @@ static MIME_HEADER *
> mime_hdr_new(char *name, char *value)
> {
> MIME_HEADER *mhdr;
> - char *tmpname, *tmpval, *p;
> + char *tmpname = NULL, *tmpval = NULL, *p;
> int c;
> +
> if (name) {
> if (!(tmpname = BUF_strdup(name)))
> - return NULL;
> + goto err;
> for (p = tmpname; *p; p++) {
> c = (unsigned char)*p;
> if (isupper(c)) {
> @@ -837,11 +838,10 @@ mime_hdr_new(char *name, char *value)
> *p = c;
> }
> }
> - } else
> - tmpname = NULL;
> + }
> if (value) {
> if (!(tmpval = BUF_strdup(value)))
> - return NULL;
> + goto err;
> for (p = tmpval; *p; p++) {
> c = (unsigned char)*p;
> if (isupper(c)) {
> @@ -849,32 +849,35 @@ mime_hdr_new(char *name, char *value)
> *p = c;
> }
> }
> - } else tmpval = NULL;
> - mhdr = malloc(sizeof(MIME_HEADER));
> + }
> + mhdr = malloc(sizeof(MIME_HEADER));
> if (!mhdr) {
> - OPENSSL_free(tmpname);
> - return NULL;
> + goto err;
> }
For what it's worth, these braces are redundant now.
> mhdr->name = tmpname;
> mhdr->value = tmpval;
> if (!(mhdr->params = sk_MIME_PARAM_new(mime_param_cmp))) {
> free(mhdr);
> - return NULL;
> + goto err;
> }
> return mhdr;
> +err:
> + free(tmpname);
> + free(tmpval);
> + return NULL;
> }
>
> static int
> mime_hdr_addparam(MIME_HEADER *mhdr, char *name, char *value)
> {
> - char *tmpname, *tmpval, *p;
> + char *tmpname = NULL, *tmpval = NULL, *p;
> int c;
> MIME_PARAM *mparam;
>
> if (name) {
> tmpname = BUF_strdup(name);
> if (!tmpname)
> - return 0;
> + goto err;
> for (p = tmpname; *p; p++) {
> c = (unsigned char)*p;
> if (isupper(c)) {
> @@ -882,22 +885,24 @@ mime_hdr_addparam(MIME_HEADER *mhdr, cha
> *p = c;
> }
> }
> - } else
> - tmpname = NULL;
> + }
> if (value) {
> tmpval = BUF_strdup(value);
> if (!tmpval)
> - return 0;
> - } else
> - tmpval = NULL;
> + goto err;
> + }
> /* Parameter values are case sensitive so leave as is */
> mparam = malloc(sizeof(MIME_PARAM));
> if (!mparam)
> - return 0;
> + goto err;
> mparam->param_name = tmpname;
> mparam->param_value = tmpval;
> sk_MIME_PARAM_push(mhdr->params, mparam);
> return 1;
> +err:
> + free(tmpname);
> + free(tmpval);
> + return 0;
> }
>
> static int
> Index: asn_pack.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/src/crypto/asn1/asn_pack.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 asn_pack.c
> --- asn_pack.c 12 May 2014 19:16:35 -0000 1.10
> +++ asn_pack.c 14 May 2014 21:18:03 -0000
> @@ -137,22 +137,26 @@ ASN1_pack_string(void *obj, i2d_of_void
> ASN1err(ASN1_F_ASN1_PACK_STRING,ERR_R_MALLOC_FAILURE);
> return NULL;
> }
> - if (oct)
> - *oct = octmp;
> } else
> octmp = *oct;
>
> if (!(octmp->length = i2d(obj, NULL))) {
> ASN1err(ASN1_F_ASN1_PACK_STRING,ASN1_R_ENCODE_ERROR);
> - return NULL;
> + goto err;
> }
> if (!(p = malloc (octmp->length))) {
> ASN1err(ASN1_F_ASN1_PACK_STRING,ERR_R_MALLOC_FAILURE);
> - return NULL;
> + goto err;
> }
> octmp->data = p;
> i2d (obj, &p);
> + if (oct)
> + *oct = octmp;
> return octmp;
> +err:
> + if (!oct || octmp != *oct)
> + ASN1_STRING_free(octmp);
> + return NULL;
> }
>
> #endif
> @@ -169,8 +173,6 @@ ASN1_item_pack(void *obj, const ASN1_ITE
> ASN1err(ASN1_F_ASN1_ITEM_PACK, ERR_R_MALLOC_FAILURE);
> return NULL;
> }
> - if (oct)
> - *oct = octmp;
> } else
> octmp = *oct;
>
> @@ -181,13 +183,19 @@ ASN1_item_pack(void *obj, const ASN1_ITE
>
> if (!(octmp->length = ASN1_item_i2d(obj, &octmp->data, it))) {
> ASN1err(ASN1_F_ASN1_ITEM_PACK, ASN1_R_ENCODE_ERROR);
> - return NULL;
> + goto err;
> }
> if (!octmp->data) {
> ASN1err(ASN1_F_ASN1_ITEM_PACK, ERR_R_MALLOC_FAILURE);
> - return NULL;
> + goto err;
> }
> + if (oct)
> + *oct = octmp;
> return octmp;
> +err:
> + if (!oct || octmp != *oct)
> + ASN1_STRING_free(octmp);
> + return NULL;
> }
>
> /* Extract an ASN1 object from an ASN1_STRING */
