624419 - Potential off-by-one error in MimeHeaders_build_heads_list()

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jason Oster (:Parasyte)]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20110105 Firefox-4.0/4.0b9pre
Build Identifier: 

This is a follow-up to Bug 574961, where a similar bug is being fixed for a top crasher in MimeHeaders_get()

The code can, in rare cases, read a byte that is outside of allocated memory.  See attached patch.

Reproducible: Always

Comment 1

[Jason Oster (:Parasyte)]

Attachment: Proposed patch

Comment 2

[David :Bienvenu]

Comment on attachment 502532 [details] [diff] [review]
Proposed patch

This doesn't look right, since we're checking against end-1, not just end:
-  for (s = hdrs->all_headers; s <= end-1; s++)
+  for (s = hdrs->all_headers; s < end-1; s++)
   {
-    if (s <= (end-1) && s[0] == '\r' && s[1] == '\n') /* CRLF -> LF */
+    if (s < (end-1) && s[0] == '\r' && s[1] == '\n') /* CRLF -> LF */

let me know if I'm missing something...

Comment 3

[Jason Oster (:Parasyte)]

(In reply to comment #2)
> Comment on attachment 502532 [details] [diff] [review]
> Proposed patch
> 
> This doesn't look right, since we're checking against end-1, not just end:
> -  for (s = hdrs->all_headers; s <= end-1; s++)
> +  for (s = hdrs->all_headers; s < end-1; s++)
>    {
> -    if (s <= (end-1) && s[0] == '\r' && s[1] == '\n') /* CRLF -> LF */
> +    if (s < (end-1) && s[0] == '\r' && s[1] == '\n') /* CRLF -> LF */
> 
> let me know if I'm missing something...

You are correct, the condition in the for loop should be s <= end-1, which would allow the if statement at line 262 to pick up a line break in the last character within the buffer.  The quoted if-statement is correct, though.

Comment 4

[Jason Oster (:Parasyte)]

Attachment: Attempt v2

Or, rather, s < end.  Logically the same, but a little more clear.

Comment 5

[David :Bienvenu]

Comment on attachment 502887 [details] [diff] [review]
Attempt v2

thx, this looks OK - this code gets a pretty thorough workout from our existing tests, and it's hard to write a test that crashes when reading one byte past the end of a buffer, so I don't think we need additional tests. I'll land this, along with a bit of nearby formatting cleanup.

Comment 6

[David :Bienvenu]

fix checked in - http://hg.mozilla.org/comm-central/rev/be18bde0f11c, thx for the patch, Jason.