usage of sprintf in Address book code

RESOLVED FIXED in Thunderbird 22.0

Status

MailNews Core
Address Book
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Usul, Assigned: aceman)

Tracking

1.9.1 Branch
Thunderbird 22.0

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

1.10 KB, patch
jcranmer
: review+
neil@parkwaycc.co.uk
: feedback+
Details | Diff | Splinter Review
> http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsVCardObj.cpp#1116

Now we have 64-bits being written to a 16-byte buffer. Since 2^64 - 1 takes up
21 characters including \0 this is not safe.

It might be a good idea to change the ValueItem union to using PRUint32 and
PRUint64 instead of unsigned int and unsigned long.
Flags: wanted-thunderbird3?
Flags: wanted-thunderbird3?
(Assignee)

Comment 1

5 years ago
The line number of nsVCardObj is probably not right today. Can you update which line/function you meant?
Assignee: nobody → acelists
(In reply to :aceman from comment #1)
> The line number of nsVCardObj is probably not right today. Can you update
> which line/function you meant?

I've cced you because I could find some sprintf in the file(In reply to Ludovic Hirlimann [:Usul] from comment #0)
> > http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsVCardObj.cpp#1116
> 
> Now we have 64-bits being written to a 16-byte buffer. Since 2^64 - 1 takes
> up
> 21 characters including \0 this is not safe.
> 
> It might be a good idea to change the ValueItem union to using PRUint32 and
> PRUint64 instead of unsigned int and unsigned long.

And cant' find that anymore - should have looked more before ccing sorry for the noise.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 3

5 years ago
There are still some but above the line you originally posted.
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsVCardObj.cpp#1077

I just wanted to be sure if those are those that you meant. As they are 16 byte as described.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 4

5 years ago
Created attachment 725871 [details] [diff] [review]
patch
Attachment #725871 - Flags: review?(mconley)
Attachment #725871 - Flags: feedback?(neil)
(Assignee)

Updated

5 years ago
Status: REOPENED → ASSIGNED

Updated

5 years ago
Attachment #725871 - Flags: feedback?(neil) → feedback+
Comment on attachment 725871 [details] [diff] [review]
patch

I'm not even sure what's happening here. Joshua, can you take a quick look at this?
Attachment #725871 - Flags: review?(mconley) → review?(Pidgeot18)
Comment on attachment 725871 [details] [diff] [review]
patch

Review of attachment 725871 [details] [diff] [review]:
-----------------------------------------------------------------

This code does have a very high WTF factor. I hope a new, sane VCard implementation will be in the works related to ensemble. :-/

This code is also wrong, but that's not your fault (it assumes sizeof(size_t) == sizeof(long) elsewhere).
Attachment #725871 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 7

5 years ago
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/2b7333a4cb28
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
You need to log in before you can comment on or make changes to this bug.