Last Comment Bug 514134 - usage of sprintf in Address book code
: usage of sprintf in Address book code
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: 1.9.1 Branch
: All All
: -- normal (vote)
: Thunderbird 22.0
Assigned To: :aceman
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 117878
  Show dependency treegraph
 
Reported: 2009-09-02 00:36 PDT by Ludovic Hirlimann [:Usul]
Modified: 2013-03-27 07:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.10 KB, patch)
2013-03-17 06:11 PDT, :aceman
Pidgeot18: review+
neil: feedback+
Details | Diff | Review

Description Ludovic Hirlimann [:Usul] 2009-09-02 00:36:41 PDT
> 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.
Comment 1 :aceman 2013-03-15 05:21:00 PDT
The line number of nsVCardObj is probably not right today. Can you update which line/function you meant?
Comment 2 Ludovic Hirlimann [:Usul] 2013-03-15 05:31:14 PDT
(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.
Comment 3 :aceman 2013-03-15 05:36:07 PDT
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.
Comment 4 :aceman 2013-03-17 06:11:47 PDT
Created attachment 725871 [details] [diff] [review]
patch
Comment 5 Mike Conley (:mconley) - (Away until June 29th) 2013-03-23 11:56:01 PDT
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?
Comment 6 Joshua Cranmer [:jcranmer] 2013-03-26 10:03:51 PDT
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).
Comment 7 :aceman 2013-03-26 12:06:38 PDT
Thanks.
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-03-27 07:40:36 PDT
https://hg.mozilla.org/comm-central/rev/2b7333a4cb28

Note You need to log in before you can comment on or make changes to this bug.