780386 - Strange code in source/security/nss/cmd/certcgi/certcgi.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Christophe JAILLET]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

I was grep for something else in libre office and found this, which is part of firefox



Expected results:

At line 1476 of source/security/nss/cmd/certcgi/certcgi.c, a assignment seems really strange to me :

		low_digit = *string = 'A';

According to surrounding code, I think it should be :
		low_digit = *string - 'A';

I'm not sure at all because I don't really understand what that code do (high_digit and low_digit seem to be unused).

Comment 1

[Matthias Versen [:Matti]]

You mean http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/certcgi/certcgi.c#1476 ?

Comment 2

[Christophe JAILLET]

Yes, that's it.

Comment 3

[:Cykesiopka]

wtc: The line still seems present in NSS, should it be - instead of = as Christophe suggested?

Comment 4

[Wan-Teh Chang]

Hi. Those two lines should read
    high_digit = *string - 'A' + 10;

    low_digit = *string - 'A' + 10;

We need to subtract 'A' from the hex digit character, and then add 10
(the value of the hex digit A).

Comment 5

[:Cykesiopka]

Attachment: Proposed Patch v1

This patch changes the lines to match those in Comment 4.

Comment 6

[Wan-Teh Chang]

Comment on attachment 745609 [details] [diff] [review]
Proposed Patch v1

r=wtc.

https://hg.mozilla.org/projects/nss/rev/0babcec0543a

Comment 7

[:Cykesiopka]

wtc: Sorry for not noticing this earlier, but I get compiler warnings that high_digit and low_digit in string_to_binary() are not used... Should they be removed?

Comment 8

[Wan-Teh Chang]

Cykesiopka: We should use high_digit and low_digit in
string_to_binary().

You can search for "low_digit" in the file to figure out how
high_digit and low_digit should be combined into a byte and
stored in rv->data[rv->len].

I also found that the caller of string_to_binary() only frees
the SECItem |temp|, but not the |temp->data| buffer, so there is
also a memory leak. We need to be careful there because in
the other branch, we have
    temp = (SECItem *) PORT_ZAlloc(sizeof(SECItem));
    ...
    temp->data = (unsigned char *)name;
so in that branch we should only free |temp|.

Comment 9

[:Cykesiopka]

Hmm, I would assume the high_digit and low_digit should be combined similar to this:
  rv->data[rv->len] = (hi_digit << 4) | low_digit;

Unfortunately I can't figure out where to free |temp->data|. |genName->name.OthName.name.data| is set to |temp->data|, but I don't know where that is used at all...

Of course, I might just be having some sort of fundamental misunderstanding, apologies...