Re: [PATCH] cleanup libssl/src/crypto/asn1/a_{int,enum}.c

2014-04-28 Thread Dirk Engling 
On 28.04.14 23:05, Miod Vallat wrote:

> I'm not too fond of this kind of change - the compiler does a good job
> at merging or optimizing temporary variables.

I disagree. I stopped writing code for the compiler when I got my first
pubic hair. Now I think that useless introduction of non-obviously named
temporary variables prevents reading and auditing code and is in the set
of issues easiest to fix. So with all due respect (and I actually DO
have a lot respect for your conservative attitude towards patches) I
think that simplifying expressions (with fixes that software can easily
verify producing equivalent code) helps the overall software security.

Regards,

  erdgeist

Re: [PATCH] cleanup libssl/src/crypto/asn1/a_{int,enum}.c

2014-04-28 Thread Miod Vallat 
> * remove unnecessary temp variable d
> * move loop counter j in for() header
> * fix prototype for memcpy
> * make calculation of actual length in BN_to_ASN1_ENUMERATED
>   more transparent
> 
> This code still looks rather odd, it uses a temporary buffer to first
> convert the number into a minimal little endian representation and then
> reverts the bytes to minimal big endian representation. A simple clz()
> could have revealed the number of bytes necessary to encode. I will fix
> this later.

I'm not too fond of this kind of change - the compiler does a good job
at merging or optimizing temporary variables.

On the other hand, I think this subset of your diff is worth putting in:

Index: a_enum.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_enum.c,v
retrieving revision 1.12
diff -u -p -r1.12 a_enum.c
--- a_enum.c21 Apr 2014 11:37:41 -  1.12
+++ a_enum.c28 Apr 2014 21:03:56 -
@@ -152,9 +152,9 @@ BN_to_ASN1_ENUMERATED(BIGNUM *bn, ASN1_E
else
ret->type = V_ASN1_ENUMERATED;
j = BN_num_bits(bn);
-   len = ((j == 0) ? 0 : ((j / 8) + 1));
-   if (ret->length < len + 4) {
-   unsigned char *new_data = realloc(ret->data, len + 4);
+   len = 4 + ((j == 0) ? 0 : ((j / 8) + 1));
+   if (ret->length < len) {
+   unsigned char *new_data = realloc(ret->data, len);
if (!new_data) {
ASN1err(ASN1_F_BN_TO_ASN1_ENUMERATED, 
ERR_R_MALLOC_FAILURE);
goto err;
Index: a_int.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_int.c,v
retrieving revision 1.18
diff -u -p -r1.18 a_int.c
--- a_int.c 21 Apr 2014 11:37:41 -  1.18
+++ a_int.c 28 Apr 2014 21:03:56 -
@@ -159,7 +159,7 @@ i2c_ASN1_INTEGER(ASN1_INTEGER *a, unsign
if (a->length == 0)
*(p++) = 0;
else if (!neg)
-   memcpy(p, a->data, (unsigned int)a->length);
+   memcpy(p, a->data, (size_t)a->length);
else {
/* Begin at the end of the encoding */
n = a->data + a->length - 1;
@@ -426,9 +426,9 @@ BN_to_ASN1_INTEGER(const BIGNUM *bn, ASN
else
ret->type = V_ASN1_INTEGER;
j = BN_num_bits(bn);
-   len = ((j == 0) ? 0 : ((j / 8) + 1));
-   if (ret->length < len + 4) {
-   unsigned char *new_data = realloc(ret->data, len + 4);
+   len = 4 + ((j == 0) ? 0 : ((j / 8) + 1));
+   if (ret->length < len) {
+   unsigned char *new_data = realloc(ret->data, len);
if (!new_data) {
ASN1err(ASN1_F_BN_TO_ASN1_INTEGER, 
ERR_R_MALLOC_FAILURE);
goto err;

[PATCH] cleanup libssl/src/crypto/asn1/a_{int,enum}.c

2014-04-22 Thread Dirk Engling 
* remove unnecessary temp variable d
* move loop counter j in for() header
* fix prototype for memcpy
* make calculation of actual length in BN_to_ASN1_ENUMERATED
  more transparent

This code still looks rather odd, it uses a temporary buffer to first
convert the number into a minimal little endian representation and then
reverts the bytes to minimal big endian representation. A simple clz()
could have revealed the number of bytes necessary to encode. I will fix
this later.

Index: a_enum.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_enum.c,v
retrieving revision 1.12
diff -u -r1.12 a_enum.c
--- a_enum.c21 Apr 2014 11:37:41 -  1.12
+++ a_enum.c23 Apr 2014 01:30:43 -
@@ -72,7 +72,6 @@
int j, k;
unsigned int i;
unsigned char buf[sizeof(long) + 1];
-   long d;

a->type = V_ASN1_ENUMERATED;
if (a->length < (int)(sizeof(long) + 1)) {
@@ -84,20 +83,18 @@
ASN1err(ASN1_F_ASN1_ENUMERATED_SET, ERR_R_MALLOC_FAILURE);
return (0);
}
-   d = v;
-   if (d < 0) {
-   d = -d;
+   if (v < 0) {
+   v = -v;
a->type = V_ASN1_NEG_ENUMERATED;
}

for (i = 0; i < sizeof(long); i++) {
-   if (d == 0)
+   if (v == 0)
break;
-   buf[i] = (int)d & 0xff;
-   d >>= 8;
+   buf[i] = (unsigned char)v;
+   v >>= 8;
}
-   j = 0;
-   for (k = i - 1; k >=0; k--)
+   for (j = 0, k = i - 1; k >=0; k--)
a->data[j++] = buf[k];
a->length = j;
return (1);
@@ -137,7 +134,7 @@
 BN_to_ASN1_ENUMERATED(BIGNUM *bn, ASN1_ENUMERATED *ai)
 {
ASN1_ENUMERATED *ret;
-   int len, j;
+   int len;

if (ai == NULL)
ret = M_ASN1_ENUMERATED_new();
@@ -151,10 +148,10 @@
ret->type = V_ASN1_NEG_ENUMERATED;
else
ret->type = V_ASN1_ENUMERATED;
-   j = BN_num_bits(bn);
-   len = ((j == 0) ? 0 : ((j / 8) + 1));
-   if (ret->length < len + 4) {
-   unsigned char *new_data = realloc(ret->data, len + 4);
+   len = BN_num_bits(bn);
+   len = 4 + ( len ? len / 8 + 1 : 0 );
+   if (ret->length < len) {
+   unsigned char *new_data = realloc(ret->data, len);
if (!new_data) {
ASN1err(ASN1_F_BN_TO_ASN1_ENUMERATED, 
ERR_R_MALLOC_FAILURE);
goto err;
Index: a_int.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/asn1/a_int.c,v
retrieving revision 1.18
diff -u -r1.18 a_int.c
--- a_int.c 21 Apr 2014 11:37:41 -  1.18
+++ a_int.c 23 Apr 2014 01:30:43 -
@@ -159,7 +159,7 @@
if (a->length == 0)
*(p++) = 0;
else if (!neg)
-   memcpy(p, a->data, (unsigned int)a->length);
+   memcpy(p, a->data, (size_t)a->length);
else {
/* Begin at the end of the encoding */
n = a->data + a->length - 1;
@@ -346,7 +346,6 @@
int j, k;
unsigned int i;
unsigned char buf[sizeof(long) + 1];
-   long d;

a->type = V_ASN1_INTEGER;
if (a->length < (int)(sizeof(long) + 1)) {
@@ -358,20 +357,18 @@
ASN1err(ASN1_F_ASN1_INTEGER_SET, ERR_R_MALLOC_FAILURE);
return (0);
}
-   d = v;
-   if (d < 0) {
-   d = -d;
+   if (v < 0) {
+   v = -v;
a->type = V_ASN1_NEG_INTEGER;
}

for (i = 0; i < sizeof(long); i++) {
-   if (d == 0)
+   if (v == 0)
break;
-   buf[i] = (int)d & 0xff;
-   d >>= 8;
+   buf[i] = (int)v & 0xff;
+   v >>= 8;
}
-   j = 0;
-   for (k = i - 1; k >= 0; k--)
+   for (j = 0, k = i - 1; k >= 0; k--)
a->data[j++] = buf[k];
a->length = j;
return (1);
@@ -411,7 +408,7 @@
 BN_to_ASN1_INTEGER(const BIGNUM *bn, ASN1_INTEGER *ai)
 {
ASN1_INTEGER *ret;
-   int len, j;
+   int len;

if (ai == NULL)
ret = M_ASN1_INTEGER_new();
@@ -425,10 +422,10 @@
ret->type = V_ASN1_NEG_INTEGER;
else
ret->type = V_ASN1_INTEGER;
-   j = BN_num_bits(bn);
-   len = ((j == 0) ? 0 : ((j / 8) + 1));
-   if (ret->length < len + 4) {
-   unsigned char *new_data = realloc(ret->data, len + 4);
+   len = BN_num_bits(bn);
+   len = 4 + ( len ? len / 8 + 1 : 0 );
+   if (ret->length < len) {
+   unsigned char *new_data = realloc(ret->data, len);
if (!new_data) {
ASN1err(ASN1_F_BN_TO_ASN1_INTEGER, 
ERR_R_MALLOC_FAILURE);
goto e