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

RESOLVED FIXED in 3.15

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Christophe JAILLET, Assigned: Cykesiopka)

Tracking

unspecified
3.15
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

972 bytes, patch
Wan-Teh Chang
: review+
Wan-Teh Chang
: checked-in+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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).
You mean http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/certcgi/certcgi.c#1476 ?
Assignee: nobody → nobody
Component: Untriaged → Libraries
Product: Firefox → NSS
Version: 14 Branch → unspecified
(Reporter)

Comment 2

6 years ago
Yes, that's it.
(Assignee)

Comment 3

5 years ago
wtc: The line still seems present in NSS, should it be - instead of = as Christophe suggested?
Flags: needinfo?(wtc)

Comment 4

5 years ago
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(wtc)
(Assignee)

Comment 5

5 years ago
Created attachment 745609 [details] [diff] [review]
Proposed Patch v1

This patch changes the lines to match those in Comment 4.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Attachment #745609 - Flags: review?(wtc)

Comment 6

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

r=wtc.

https://hg.mozilla.org/projects/nss/rev/0babcec0543a
Attachment #745609 - Flags: review?(wtc)
Attachment #745609 - Flags: review+
Attachment #745609 - Flags: checked-in+

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.15
(Assignee)

Comment 7

5 years ago
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?
Flags: needinfo?(wtc)

Comment 8

5 years ago
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|.
Flags: needinfo?(wtc)
(Assignee)

Comment 9

5 years ago
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...
You need to log in before you can comment on or make changes to this bug.