Miod Vallat <m...@online.fr> 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 */