Miod Vallat <m...@online.fr> writes: > As promised, here is a new diff. Bob Beck suggested introducing wrappers > to the time-related functions, so that the error path becomes easier to > understand; this makes the diff to these functions much simpler > indeed.
That's helpful. It definitely makes it easier to read. > > 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 15 May 2014 18:30:29 -0000 > @@ -208,20 +208,15 @@ ASN1_GENERALIZEDTIME_set(ASN1_GENERALIZE > return ASN1_GENERALIZEDTIME_adj(s, t, 0, 0); > } > > -ASN1_GENERALIZEDTIME * > -ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, time_t t, int offset_day, > - long offset_sec) > +static ASN1_GENERALIZEDTIME * > +ASN1_GENERALIZEDTIME_adj_internal(ASN1_GENERALIZEDTIME *s, time_t t, > + int offset_day, long offset_sec) > { > 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); > - > ts = gmtime_r(&t, &data); > if (ts == NULL) > return (NULL); > @@ -249,4 +244,25 @@ ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZE > s->length = strlen(p); > s->type = V_ASN1_GENERALIZEDTIME; > return (s); > +} > + > +ASN1_GENERALIZEDTIME * > +ASN1_GENERALIZEDTIME_adj(ASN1_GENERALIZEDTIME *s, time_t t, int offset_day, > + long offset_sec) > +{ > + ASN1_GENERALIZEDTIME *tmp = NULL, *ret; > + > + if (s == NULL) { > + tmp = M_ASN1_GENERALIZEDTIME_new(); > + if (tmp == NULL) > + return NULL; > + s = tmp; > + } > + > + ret = ASN1_GENERALIZEDTIME_adj_internal(s, t, offset_day, offset_sec); > + if (ret == NULL && tmp != NULL) > + M_ASN1_GENERALIZEDTIME_free(tmp); > + > + return ret; > + > } > 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 15 May 2014 18:30:29 -0000 > @@ -120,8 +120,8 @@ ASN1_TIME_check(ASN1_TIME *t) > } > > /* Convert an ASN1_TIME structure to GeneralizedTime */ > -ASN1_GENERALIZEDTIME * > -ASN1_TIME_to_generalizedtime(ASN1_TIME *t, ASN1_GENERALIZEDTIME **out) > +static ASN1_GENERALIZEDTIME * > +ASN1_TIME_to_generalizedtime_internal(ASN1_TIME *t, ASN1_GENERALIZEDTIME > **out) > { > ASN1_GENERALIZEDTIME *ret; > char *str; > @@ -131,13 +131,7 @@ ASN1_TIME_to_generalizedtime(ASN1_TIME * > if (!ASN1_TIME_check(t)) > return NULL; > > - if (!out || !*out) { > - if (!(ret = ASN1_GENERALIZEDTIME_new ())) > - return NULL; > - if (out) > - *out = ret; > - } else > - ret = *out; > + ret = *out; > > /* If already GeneralizedTime just copy across */ > if (t->type == V_ASN1_GENERALIZEDTIME) { > @@ -152,12 +146,32 @@ ASN1_TIME_to_generalizedtime(ASN1_TIME * > /* 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); > + *out = NULL; > return NULL; > } > + return ret; > +} > + > +ASN1_GENERALIZEDTIME * > +ASN1_TIME_to_generalizedtime(ASN1_TIME *t, ASN1_GENERALIZEDTIME **out) > +{ > + ASN1_GENERALIZEDTIME *tmp = NULL, *ret; > + > + if (!out || !*out) { > + if (!(tmp = ASN1_GENERALIZEDTIME_new())) > + return NULL; > + out = &tmp; So we no longer return the newly-allocated ASN1_GENERALIZEDTIME through *out? This might be a problem, since callers seem to use the value in *out instead of storing the return value. > + } > + > + ret = ASN1_TIME_to_generalizedtime_internal(t, out); > + if (ret == NULL && tmp != NULL) > + ASN1_GENERALIZEDTIME_free(tmp); > + > return ret; > } > > 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 15 May 2014 18:30:29 -0000 > @@ -149,19 +149,15 @@ ASN1_UTCTIME_set(ASN1_UTCTIME *s, time_t > return ASN1_UTCTIME_adj(s, t, 0, 0); > } > > -ASN1_UTCTIME * > -ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t t, int offset_day, long offset_sec) > +static ASN1_UTCTIME * > +ASN1_UTCTIME_adj_internal(ASN1_UTCTIME *s, time_t t, int offset_day, > + long offset_sec) > { > 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); > - > ts = gmtime_r(&t, &data); > if (ts == NULL) > return (NULL); > @@ -191,6 +187,25 @@ ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t > s->length = strlen(p); > s->type = V_ASN1_UTCTIME; > return (s); > +} > + > +ASN1_UTCTIME * > +ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t t, int offset_day, long offset_sec) > +{ > + ASN1_UTCTIME *tmp = NULL, *ret; > + > + if (s == NULL) { > + tmp = M_ASN1_UTCTIME_new(); > + if (tmp == NULL) > + return NULL; > + s = tmp; > + } > + > + ret = ASN1_UTCTIME_adj_internal(s, t, offset_day, offset_sec); > + if (ret == NULL && tmp != NULL) > + M_ASN1_UTCTIME_free(tmp); > + > + return ret; > } > > 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 15 May 2014 18:30:29 -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,34 @@ mime_hdr_new(char *name, char *value) > *p = c; > } > } > - } else tmpval = NULL; > - mhdr = malloc(sizeof(MIME_HEADER)); > - if (!mhdr) { > - OPENSSL_free(tmpname); > - return NULL; > } > + mhdr = malloc(sizeof(MIME_HEADER)); > + if (!mhdr) > + goto err; > 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 +884,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 15 May 2014 18:30:29 -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 */