Last Comment Bug 514136 - Increase the size of the char array for imap command tag
: Increase the size of the char array for imap command tag
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (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:42 PDT by Ludovic Hirlimann [:Usul]
Modified: 2013-03-19 14:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.74 KB, patch)
2013-03-16 15:29 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v2 (1.84 KB, patch)
2013-03-17 06:47 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v3 (1.84 KB, patch)
2013-03-17 14:57 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Ludovic Hirlimann [:Usul] 2009-09-02 00:42:04 PDT
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 2013-03-15 05:24:17 PDT
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] 2013-03-15 05:31:57 PDT
(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 neil@parkwaycc.co.uk 2013-03-15 05:40:49 PDT
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 2013-03-15 05:52:09 PDT
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 neil@parkwaycc.co.uk 2013-03-15 07:09:26 PDT
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 neil@parkwaycc.co.uk 2013-03-15 07:11:21 PDT
Also the code uses %ld so you'd actually see -2147483648 ;-)
Comment 7 :aceman 2013-03-15 14:28:29 PDT
So use nsCString.AppendInt() as the description requests and be done with it? :)
Is the function speed sensitive?
Comment 8 neil@parkwaycc.co.uk 2013-03-15 14:44:21 PDT
No, nsCString would be overkill.
Comment 9 :aceman 2013-03-15 14:59:37 PDT
Then why do you oppose char[11] so that is it obvious to anyone and does not increase memory usage?
Comment 10 neil@parkwaycc.co.uk 2013-03-15 15:44:50 PDT
(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 2013-03-16 15:29:27 PDT
Created attachment 725808 [details] [diff] [review]
patch
Comment 12 neil@parkwaycc.co.uk 2013-03-17 03:43:09 PDT
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 2013-03-17 05:06:12 PDT
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2013-03-17 05:52:46 PDT
That's easy; even on ILP64, uint32_t gets promoted to unsigned int.
Comment 15 :aceman 2013-03-17 06:47:45 PDT
Created attachment 725873 [details] [diff] [review]
patch v2
Comment 16 neil@parkwaycc.co.uk 2013-03-17 10:02:12 PDT
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 2013-03-17 14:57:13 PDT
Created attachment 725935 [details] [diff] [review]
patch v3

Thanks.
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-03-19 14:50:59 PDT
https://hg.mozilla.org/comm-central/rev/5fa1d0a57bcb

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