514136 - Increase the size of the char array for imap command tag

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Ludovic Hirlimann [:Usul]]

the first imap one is safe as long
as we don't send more than 1 billion commands to the imap server (though it
should be fixed to use nsCString)

Comment 1

[:aceman]

This line number also seems to be outdated. Which function is meant?

Also, do we need to care about billion commands? My IMAP server rejects uploading even 100 messages (supposedly rate limiting) :)

Comment 2

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman from comment #1)
> This line number also seems to be outdated. Which function is meant?
> 
> Also, do we need to care about billion commands? My IMAP server rejects
> uploading even 100 messages (supposedly rate limiting) :)

Neil ?

Comment 3

[[e-mail]]

It's IncrementCommandTagNumber(), and it's unrelated to the number of messages. Also, the code won't actually overflow, as due to alignment requirements, there's actually 12 characters of space allocated to that 32-bit number ;-)

Comment 4

[:aceman]

But can we rely on that? The code is:
char m_currentServerCommandTag[10];   // enough for a billion
int  m_currentServerCommandTagNumber;

So if int is 32-bit long, the number 2147483648 is 10 chars long and the trailing \0 has no space.
So do we increase it to 11 (if there is alignment, we will lose no additional memory)?

Comment 5

[[e-mail]]

The point is that the compiler will generate identical code for 9, 10, 11 or 12 because it has to round the array up to 12 anyway. Only code that explicitly checks the array length (via sizeof or templates) would know the difference.

Comment 6

[[e-mail]]

Also the code uses %ld so you'd actually see -2147483648 ;-)

Comment 7

[:aceman]

So use nsCString.AppendInt() as the description requests and be done with it? :)
Is the function speed sensitive?

Comment 8

[[e-mail]]

No, nsCString would be overkill.

Comment 9

[:aceman]

Then why do you oppose char[11] so that is it obvious to anyone and does not increase memory usage?

Comment 10

[[e-mail]]

(In reply to aceman from comment #9)
> Then why do you oppose char[11] so that is it obvious to anyone and does not
> increase memory usage?

It was not my intent to oppose it; I was merely correcting your example which was using a number that probably does not fit in an int (are there platforms with 64-bit ints? why is that an int, and then cast to a long for the sprintf?)

Comment 11

[:aceman]

Attachment: patch

Comment 12

[[e-mail]]

Comment on attachment 725808 [details] [diff] [review]
patch

>+  sprintf(m_currentServerCommandTag,"%d", ++m_currentServerCommandTagNumber);
Nit: while you're editing this, space after the first comma please.

>+  // 11 = enough memory for the decimal representation of MAX_INT + trailing nul
>+  char m_currentServerCommandTag[11];
I guess after you reach MAX_INT, ++m_currentServerCommandTagNumber becomes undefined behaviour.

>   int  m_currentServerCommandTagNumber;
Maybe we should switch to uint32_t to avoid the problem ;-)

Comment 13

[:aceman]

(In reply to [e-mail] from comment #12)
> Comment on attachment 725808 [details] [diff] [review]
> patch
> 
> >+  sprintf(m_currentServerCommandTag,"%d", ++m_currentServerCommandTagNumber);
> Nit: while you're editing this, space after the first comma please.
> 
> >+  // 11 = enough memory for the decimal representation of MAX_INT + trailing nul
> >+  char m_currentServerCommandTag[11];
> I guess after you reach MAX_INT, ++m_currentServerCommandTagNumber becomes
> undefined behaviour.
Yes, but that is a different problem:)

> >   int  m_currentServerCommandTagNumber;
> Maybe we should switch to uint32_t to avoid the problem ;-)
I thought about that, but then how do we guarantee "%d" is 32 bit? I'd imagine that int and "%d" would mathc in size on all archs/compilers. But I do not know if uin32_t and "%d" always match.

Comment 14

[[e-mail]]

That's easy; even on ILP64, uint32_t gets promoted to unsigned int.

Comment 15

[:aceman]

Attachment: patch v2

Comment 16

[[e-mail]]

Comment on attachment 725873 [details] [diff] [review]
patch v2

>+  sprintf(m_currentServerCommandTag, "%d", ++m_currentServerCommandTagNumber);
I guess this needs to be %u now... r=me with that fixed.

Comment 17

[:aceman]

Attachment: patch v3

Thanks.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/5fa1d0a57bcb