514134 - usage of sprintf in Address book code

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Ludovic Hirlimann [:Usul]]

> 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]

The line number of nsVCardObj is probably not right today. Can you update which line/function you meant?

Comment 2

[Ludovic Hirlimann [:Usul]]

(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]

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]

Attachment: patch

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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]]

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]

Thanks.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2b7333a4cb28