814870 - Invalid read of size 1 in PR_ParseTimeStringToExplodedTime (prtime.c:1450)

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Partial log of running TB "make mozmill" test under valgrind

Invalid read of size 1 in PR_ParseTimeStringToExplodedTime (prtime.c:1450)

(This bug is very different from 
Bug 806293 - Use of uninitialised value of size 4 in
PR_ParseTimeStringToExplodedTime probably created by nsByteArray::GrowBuffer 


I noticed this particular problem in the log of valgrind run of thunderbird which is attached. (The log is partital: it is cut short since the test is still running.)
You can find out how I ran this valgrind session near the beginning of the log file. I dumped the content of a shell script that runs this using "set -xv".

See Bug 803816 - Valgrind warnings about uninitialised memory use (Thunderbird)
for how I came to run TB under valgrind and how I solved some issues.

However, the problem of PR_ParseTimeStringToExplodedTime I report here
is not exactly an uninitialized value issue, though.

The problem reported here is a simple failure to check the upper-bound
of an allocated area, and access beyond the end.

I quote the essential part of the log near the end.

The functin is declared as follows:

PR_IMPLEMENT(PRStatus)
PR_ParseTimeStringToExplodedTime(
        const char *string,
        PRBool default_to_gmt,
        PRExplodedTime *result)
{
	[...]
}

The problem is caused by the simple failure to check the upper bound
of passed char array ("string" above) in the routine when it is scanned.  
(We immediately notice the upper bound is NOT passed to this function.)

The proper fix would be to pass the upper bound to the function, and
the function should check it in accessing the char array.

Reasoning:

I noticed that the routine is reading beyond the allocated block
which is passed as "*string". 
"0 bytes after a block ... allo'd" raised the flag.
     
> ==17463== Invalid read of size 1
> ==17463==    at 0x4716075: PR_ParseTimeStringToExplodedTime (prtime.c:1450)
 [...]
>==17463==  Address 0x14d87cc8 is 0 bytes after a block of size 1,024 alloc'd

The code on line 1459 of prtime.c is a condition expression of a while loop
in which the pointer to scan the array is incremented WITHOUT CHECKING
THE UPPER-BOUND AT ALL!

1457	          /* skip over uninteresting chars. */
 1458	        SKIP_MORE:
 1459	          while (*rest &&
 1460	                         (*rest == ' ' || *rest == '\t' ||
 1461	                          *rest == ',' || *rest == ';' || *rest == '/' ||
 1462	                          *rest == '(' || *rest == ')' || *rest == '[' || *rest == ']'))
 1463	                rest++;


Big problem! 
You will get a C- for a systems programming course with the code above.


Excerpt of problem from the attached valgrind log 

==17463== Invalid read of size 1
==17463==    at 0x4716075: PR_ParseTimeStringToExplodedTime (prtime.c:1450)
==17463==    by 0x47171B0: PR_ParseTimeString (prtime.c:1642)
==17463==    by 0x5C5B55C: nsParseMailMessageState::FinalizeHeaders() (nsParseMailbox.cpp:1587)
==17463==    by 0x5C5C22B: nsParseMailMessageState::ParseFolderLine(char const*, unsigned int) (nsParseMailbox.cpp:683)
==17463==    by 0x5C5698B: nsMsgMailboxParser::HandleLine(char*, unsigned int) (nsParseMailbox.cpp:488)
==17463==    by 0x5B8C0DB: nsMsgLineBuffer::ConvertAndSendBuffer() (nsMsgLineBuffer.cpp:233)
==17463==    by 0x5B8C1E4: nsMsgLineBuffer::BufferInput(char const*, int) (nsMsgLineBuffer.cpp:170)
==17463==    by 0x5C6A26B: nsMsgLocalMailFolder::AddMessageBatch(unsigned int, char const**, nsIArray**) (nsLocalMailFolder.cpp:3576)
==17463==    by 0x609C78F: NS_InvokeByIndex_P (in /TB-NEW/TB-3HG/objdir-tb3/mozilla/toolkit/library/libxul.so)
==17463==    by 0x57DD619: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3081)
==17463==    by 0x57E4ED7: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==17463==    by 0x6767315: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==17463==    by 0x6758A10: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2334)
==17463==    by 0x6766339: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:326)
==17463==    by 0x67674D2: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:384)
==17463==    by 0x6729D79: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:109)
==17463==    by 0x6767315: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==17463==    by 0x6767AE9: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==17463==    by 0x67A88D0: js::BaseProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:266)
==17463==    by 0x6816156: js::Wrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:300)
==17463==    by 0x6817D19: js::CrossCompartmentWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:633)
==17463==    by 0x67AE0B3: js::Proxy::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:2466)
==17463==    by 0x67AE10A: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3026)
==17463==    by 0x676759C: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==17463==    by 0x6758A10: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2334)
==17463==    by 0x6766339: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:326)
==17463==    by 0x67674D2: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:384)
==17463==    by 0x6767AE9: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==17463==    by 0x67A88D0: js::BaseProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:266)
==17463==    by 0x6816156: js::Wrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:300)
==17463==    by 0x6817D19: js::CrossCompartmentWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:633)
==17463==    by 0x67AE0B3: js::Proxy::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:2466)
==17463==    by 0x67AE10A: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3026)
==17463==    by 0x676759C: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==17463==    by 0x6758A10: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2334)
==17463==    by 0x6766339: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:326)
==17463==    by 0x6766585: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:515)
==17463==    by 0x57CB935: XPCJSStackFrame::Release() (XPCStack.cpp:90)
==17463==  Address 0x14d87cc8 is 0 bytes after a block of size 1,024 alloc'd
==17463==    at 0x40271C4: malloc (vg_replace_malloc.c:270)
==17463==    by 0x4709EE7: PR_Malloc (prmem.c:435)
==17463==    by 0x5B8BEFC: nsByteArray::GrowBuffer(unsigned int, unsigned int) (nsMsgLineBuffer.cpp:40)


By the way, the difference of this log from the older logs in 
Bug 803816 is that I added the following
options to valgrind.
	 --malloc-fill=0xA5 --free-fill=0xC3 

This changge may have helped me to uncover the issue in a clear light.

In Bug 806293, the uninitialized value could have contained anything
(possibly zero), and so the code may have taken an early return. But
now the uninitialized heap area contains one of the above non-zero
values, and so the routine may enter the loop and went over the
upper-bound eventually.

I think the routine needs a fix (by passing the upper bound and check
it within the function properly.)

Shortening this function consisting of about 700 lines of code (YES,
SEVEN HUNDRED LINES for a SINGLE FUNCTION. Ouch.) ought to be on a
todo list for the maintainer IMHO.

I could not figure out easily how the check needs to be done, and what
the function needs to do when it hits the upperbound (say, in the
problematic while-loop, for example).

TIA

Comment 1

[:aceman]

This function also seems to reside in Gecko Core, http://mxr.mozilla.org/comm-central/source/mozilla/nsprpub/pr/src/misc/prtime.c

Comment 2

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #1)
> This function also seems to reside in Gecko Core,
> http://mxr.mozilla.org/comm-central/source/mozilla/nsprpub/pr/src/misc/
> prtime.c

Thank you. Sorry I was not clear enough,
this is the file which thunderbird in comm-central is using, I believe.

The particular function has a different indentation style from the rest of the file and not following the first mode line:
/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I would suggest that the first patch is to re-indent the whole file before any major surgery is performed to ease the future editing efforts.
Otherwise, it would be difficult for someone using emacs to fix this file.

Comment 3

[ISHIKAWA, Chiaki]

Attachment: excerpt from mozmill run of local DEBUG BUILD of TB

I am attaching a log, "make mozmill" of debug build of thunderbird,
that shows that somehow 
PR_ParseTimeStringToExplodedTime()
called from
PR_ParseTimeString()
is scanning seemingly bogus memory area (not sure why this happens, but
it is filled with hex 0xda.)
Because of the bogus data, the routine is scanning past the passed buffer
area (PR_ParseTimeStringToExplodedTime() has a built-in limit of 1000.)

What I did to capture the log was:

- There was a insufficient check of this 1000 limit: I inserted a set of checks
to make this limit-check more stringent.
When this limit is reached during scan, the memory area up to 1000 is dumped.
(In the log, it shows 0xda values). and the function return an error value.

- I inserted NS_ASSERTION() on the caller side of PR_ParseTimeString() 
to track down where such call is taking place. PR_ParseTimeString() returns
error value, but I found in many places, the returned value is ignored :-(
Anyway, when the error value is returned, NS_ASSERTION() is called so that
we can have the stack trace.

e.g.: (Note due to my insertion of NS_ASSERTION() checks, line numbers may be
slightly off from the pristine comm-central source files.)
 
TEST-START | /TB-NEW/TB-3HG/new-src/mail/test/mozmill/message-header/test-message-header.js | test_add_contact_from_context_menu
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "Controller.keypress()"}
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "Controller.keypress()"}
dumping string:
<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda> ...
   .... [omission] ...
<0xda><0xda><0xda>
WARNING: PR_ParseTimeStringToExplodedTime scanned more than 1000 chars at line 1633
PR_ParseTimeString failed.
###!!! ASSERTION: PR_ParaseTimeString failed.: 'PR_SUCCESS == timeStatus', file /TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp, line 1603
nsParseMailMessageState::FinalizeHeaders() (/TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp:1620)
nsParseMailMessageState::ParseFolderLine(char const*, unsigned int) (/TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp:684)
nsMsgMailboxParser::HandleLine(char*, unsigned int) (/TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp:488)
nsMsgLineBuffer::ConvertAndSendBuffer() (/TB-NEW/TB-3HG/new-src/mailnews/base/util/nsMsgLineBuffer.cpp:233)
nsMsgLineBuffer::BufferInput(char const*, int) (/TB-NEW/TB-3HG/new-src/mailnews/base/util/nsMsgLineBuffer.cpp:170)
  [...rest omitted]

see full log attached for details.


In order to produce this log, I needed to modify prtime.c
(comm-central/mozilla/nsprpub/pr/src/misc/prtime.c)
and the caller side,
/TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp

I am attaching a patch in the follow-up posts.

I wonder if this should be considered a security bug or not...

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Patch to mailnews/local/src/nsParseMailbox.cpp

This is the patch to nsParseMailBox.cpp.
In this particular file, the return value of PR_ParseTime is checked for success.
But the failure occurs when a bogus buffer area is passed.
This origin of bogus buffer area needs to be investigated.

I can understand a failure due to a malformed date header line, but
being passed a totally messed up bogus memory buffer area suggests that
something is quite wrong.

BTW, the error shown in the log is the only such failure during "make mozmill".
I suspect that the bogus memory area is a previously allocated area.
I say this because memcheck does't complain about invalid address: in an earlier run last year, memcheck of valgrind complained about invalid address, but this time, it seems the buffer area is a part of a larger allocated area and thus memcheck didn't complain. This could be related to many factors such as [many source changes, as well as my adding the following before running "make mozmill" test. 
--- quote ---
# This may slightly cleans up memcheck session log.
# # see https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_Valgrind#Tips_for_improving_performance_and_accuracy_of_Valgrind%27s.c2.a0Memcheck_tool
#
G_SLICE=always-malloc
export G_SLICE
--- end quote ---

Comment 5

[ISHIKAWA, Chiaki]

Attachment: indent patch to comm-central/mozilla/nsprpub/pr/src/misc/prtime.c

prtime.c does not follow the indentation guidline despite the
initial line at the beginning:
/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- *

So I re-indented the whole file and removed trailing blanks before
working on the local fix.

Here is the whitespace-only change of pritme.c to fix the indentation and
removing the trailing blanks.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Final (current) patch to prtime.c (created against the re-indented version.)

This is a patch to prtime.c created against the re-indented prtime.c with the trailing blanks removed.

There are additional changes to use a macro to make the
code a little more readable in the first half and
an addition of "CET" for timezone.

But please ignore them for the moment.

The main change regarding this bug is found at dumpstring() and
changes to
 PR_ParseTimeStringToExplodedTime()
and
 PR_ParseTimeString()

There are more checks for the original 1000-limit in the big loop
and dumping of the passed buffer when this upper limit is reached to
find out what goes on.

Hope these uploads help us to figure out the cause of the bug.

Comment 7

[ISHIKAWA, Chiaki]

I am afraid that "diff" feature of the bugzilla display may not work well for the
patch uploaded, but the problem stands. 

For some reason, TB passes a lower-level routine to parse time string, totally bogus memory area, and causes the lower-level routine which lacked proper 
bound-checking to access "invalid" memory beyond the area to be scanned.

TIA

Comment 8

[:aceman]

Could this be a security problem?

Comment 9

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #8)
> Could this be a security problem?
I am afraid that, based on the reason this bogus buffer is passed, especially if the reason could be manipulated by external facto, i.e., the header of incoming e-mail, then at least there maybe a chance of DoS (maybe a random crash due to invalid memory area...)
As far as I can tell, only reading is performed, and so the problem is that of
segfault due to the invalid memory access.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting security-sensitive (s-s) first based on Chiaki and aceman's concerns in the previous comments. Please feel free to open up if this is not s-s.

Comment 11

[Wan-Teh Chang]

Comment on attachment 684863 [details]
Partial log of running TB "make mozmill" test under valgrind

Thank you for the bug report.

This valgrind log file shows all the memory errors come from the
PR_ParseTimeString call in nsParseMailMessageState::FinalizeHeaders()
at nsParseMailbox.cpp:1587.

I took a quick look at how PR_ParseTimeStringToExplodedTime reads its
|string| input argument (using the |rest| local variable). I didn't
finish the review, but in the code I looked at, the function always
checks the current character is not '\0' (used to terminate a C string)
before reading the next character. This made me suspect that maybe
the input is not a null-terminated C string.

Here is an excerpt of the relevant source code in nsParseMailbox.cpp:

1585         if (date)
1586         {  // Date:
1587           PRTime resultTime;
1588           PRStatus timeStatus = PR_ParseTimeString (date->value, false, &resultTime);

(I am looking at the current version of the file, so the line number of
the PR_ParseTimeString call has changed by one.)

So the next question is: is date->value a null-terminated C string?

|date| is declared as:

1272   struct message_header *date;

struct message_header is defined as:

40 /* Used for the various things that parse RFC822 headers...
41  */
42 typedef struct message_header
43 {
44   const char *value; /* The contents of a header (after ": ") */
45   int32_t length;      /* The length of the data (it is not NULL-terminated.) */
46 } message_header;

Note the |length| struct member. This is strong evidence that date->value
is not a null-terminated C string. If so, date->value must be converted to
a null-terminated C string before being passed to PR_ParseTimeString.

PR_ParseTimeString is a C function and expects that the |string| input
argument is a null-terminated C string.

Comment 12

[Wan-Teh Chang]

I suggest that someone investigate the code in nsParseMailbox.cpp.

I saw some code in nsParseMailbox.cpp that suggests date->value may be
null-terminated:

1101     if (header)
1102     {
1103       value = colon + 1;
1104       // eliminate trailing blanks after the colon
1105       while (*value == ' ' || *value == '\t')
1106         value++;
1107 
1108       header->value = value;
1109       header->length = buf - header->value - writeOffset;
1110     }
1111     if (*buf == '\r' || *buf == '\n')
1112     {
1113       char *last = buf - writeOffset;
1114       char *saveBuf = buf;
1115       if (*buf == '\r' && buf[1] == '\n')
1116         buf++;
1117       buf++;
1118       // null terminate the left-over slop so we don't confuse msg filters.
1119       *saveBuf = 0;
1120       *last = 0;  /* short-circuit const, and null-terminate header. */
1121     }

Someone who knows nsParseMailbox.cpp better than I do can find out
whether |date->value| is a null-terminated C string.  Perhaps for the
TB "make mozmill" test, this code is not executed for some reason?

If |date->value| is not a null-terminated C string, we can use
nsAutoCString to convert it to a C string.

If |date->value| is a null-terminated C string, then please change this
bug back to an NSPR bug and I will track down the problem in
PR_ParseTimeStringToExplodedTime.

Thanks.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Mark, perhaps you might know how to proceed here?

Comment 14

[Wan-Teh Chang]

Comment on attachment 701829 [details] [diff] [review]
Final (current) patch to prtime.c (created against the re-indented version.)

Thank you for the patch. This patch seems to contain the indentation
changes, so it is extremely difficult to see the non-indentation changes.

Could you generate a new patch that contains only the non-indentation
changes? Thanks.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Chiaki, do a `hg diff -w` and post the output - that should remove the indentation line changes.

Comment 16

[ISHIKAWA, Chiaki]

Attachment: hg diff -w prtime.c

created hg diff -w prtime.c

Comment 17

[ISHIKAWA, Chiaki]

Something is fishy.
The problem disappeared. I could think of only one reason what may have caused
the disappearance. I ran TB to check the patched nsHelperAppDlg.js (to check for 
the operation of TB when it tries to save attachement to write-protected directory.
Bug 567585  "TB3 fails to raise an error when it tries to save an attachment to write-protected directory."
Can it change the user profile environment enough to cause changed behavior?
(But I thought mozmill test sets up test user environment for repeatability.)

Or, maybe I made a typo to either prtime.c or to nsParseMailbox.cpp while fixing the
debug output for better readability :-(

Or maybe the addition of debug statements (and data structure) changed the memory layout to mask the problem (?)

I will upload the final change to prtime.c and nsParseMailbox.cpp and the
latest log (though only in one place the PR_ParseTimeString fails, but now
the error occurs since PR_ParseTimeString() is passed an empty string (i.e.,
a character array with its first element being NUL character. 
PR_ParseTimeStringToExplodedTime() in this case doesn't look inside the array and
BIG while statement is skipped and then an error is returned very quickly.

It is true that the NUL termination is more or less attempted, but I have not
verified if all calls to PR_ParseTimeString() has a proper NUL-terminated string yet.

TIA

Comment 18

[ISHIKAWA, Chiaki]

Attachment: Excerpt of session log with symbolic stack trace [with additional dump.]

I am attaching excerpts from session log.  (and the modification to
prtime.c and nsParseMailbox.cpp)

I confirm again that the the problem is for real, and the symptom is
non-deterministic.

I say this because the strange behavior is observed again for the same
test target in "make mozmill", i.e., test_add_contact_from_context_menu.  
PR_ParseTimeString() is passed with again bogus buffer after I
reported a strange behavior of seeing an empty string passed to it
earlier (for the same test target).  There must be something to this
target that triggers a strange behavior in an non-deterministic
manner.

Details:
In a couple of early runs today, I saw that PR_ParseTimeString() is
passed an empty string. "<[e-mail]> ..." was the buffer passed in
nsParseMailMessageState::Finalize() and then subsequently
somehow PR_ParseTimeString() ended up with empty string.

Yesterday, and today's later session showed PR_ParseTimeString was
passed a large array filled with 0xDA.  I think 0xDA shows that the
area is a part of a formerly allocated then subsequently freed memory
block.  This I say because 0xDA seems to be filled into free'ed area.
Searching 0xDA in comm-central source tree using mxr.mozilla.org and
searching for define shows two interesting entries.
/mozilla/nsprpub/lib/ds/plarena.h 
line 112 -- #define PL_FREE_PATTERN 0xDA 
(There is also /mozilla/js/public/Utility.h 
line 45 -- #define JS_FREE_PATTERN 0xDA) 

Validity of limit 1000: I think this is questionable now.

Originally PR_ParseTimeString (actually PR_...) has the built-in limit
of 1000. See the initial check inside the big while statement.  And I
made sure this limit is observed by inserting more proper
checks. [NOTE that without these, memcheck warned that TB was going
outside the bound, i.e. invalid memory access. So I think these checks
are certainly needed.]

I wonder if this value of 1000 is valid only for e-mail message during
flight. It seems that it may not be quite correct (we may need to
increase it?) for messages in local mbox file (?) or the way
nsParseMailbox.cpp handles it (?)

I notice that there are flags that are added to each message (Berkeley
flag as nsParseMailbox.cpp calls. There may be other data tucked onto
mail message and this may necessitates longer limit.  

I say this is because the routine in nsParseMailbox.cpp seems to
attach the terminating NUL octet at 1007th octet position of the
buffer when the problem occurred.  Yikes, 1007.  This is way beyond
1000!

Since PR_ParseTimeStringToExplodedTime() has the built-in limit of
1000 octets to scan, it can not see this terminating NUL ever.

Clearly, something is very wrong here!  There seemed to be unfulfilled
preconditions or misunderstood APIs.  (Or maybe there are two different
types of buffers with different upper limits that need to be handled
by prtime.c?)

Non-determinism:

The symptom differed from the one yesterday (and even from one run to
the other today). This may be due to

 - the changed memory layout due to the added dumping, static array, etc.
   may change the buggy behavior, and/or

 - there is an issue of uninitialized variable that may affect this
   non-deterministic behavior, come to think of it.
   (Bug 806293 - Use of uninitialized value of size 4 in
   PR_ParseTimeStringToExplodedTime probably created by
   nsByteArray::GrowBuffer )
   [Can someone with resource rich PC investigate this issue?]

Also, in nsParseMailbox.cpp, there was one place where uint32_t was
used to hold the result of object.IndexOf() and then checked against
"-1" for error condition. This is bad.  

I fixed it to use int32_t instead.  (This problem was brought home by
the compiler's warning about signed and unsigned comparison which
would always fail. We should heed these warnings help the efforts in
Bug 187528 "[META] Fix compiler 'Build Warnings'" to eradicate compile
time warning messages.  Frivolous warnings mask these serious
warnings. Tough luck :-(

The problematic path in nsParseMailbox.cpp
saw only one error during "make mozmill" execution.
This is confirmed by the strange date->length value that is observed
only once.

grep "date->length" sessionlog | sort | uniq -c | sort -n -r &

    775 date->length=31
      1 date->length=-2  <--- this was the strange value
                        when the erratic behavior was observed.
(This strange value was observed also when the empty string
is passed to PR_ParseTimeString().)
			
BTW, adding the dump message

 "PR_ParseTimeString: PR_ParseTimeString failed." 

to TB revealed that running test targets in cookies directory caused
the TB test suite to print this message as soon as it starts with the
testing environment for cookies test.  So it seems that THERE IS
ANOTHER FAILURE/Problematic path. I will investigate this further.

TIA

Comment 19

[ISHIKAWA, Chiaki]

Attachment: Modification to nsParseMailbox.cpp

I am obsoleting the previous patch to nsParseMailbox.cpp

Comment 20

[ISHIKAWA, Chiaki]

Attachment: The patch to prtime.c to create the attached session log

I am obsoleting the previous diff of prtime.c

Comment 21

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, chiaki from comment #18)
> Created attachment 702387 [details]
> Excerpt of  session log with symbolic stack trace [with additional dump.]
> 
> I am attaching excerpts from session log.  (and the modification to
> prtime.c and nsParseMailbox.cpp)
> 

Let me highlight the buggy symptom using a summary of session log.

TEST-START | /TB-NEW/TB-3HG/new-src/mail/test/mozmill/message-header/test-message-header.js | test_add_contact_from_context_menu
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "Controller.keypress()"}
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "Controller.keypress()"}

ParseHeaders entered.
ParseHeaders buf.

   [CI's comment:  Note there is no NUL termination at this stage.
   Repetition of <0xda> is substitued with ... ]

Content-Type: text/plain; charset=ISO-8859-1; format=flowed<0x0d><0x0a>Subject: Blue Spreadsheet Highest Priority<0x0d><0x0a>From: "Will Bell" <[e-mail]><0x0d><0x0a>To: "Xavier Clarke" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a><0xda>....................................<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08>
 -----
before checking CR NL:
header->length=45
dumping header->value

	CI's comment: note that there is a NUL now. (Printing is
	terminated at this NUL. I wonder why the particular position
	is chosen. See full listing to learn the position.

text/plain; charset=ISO-8859-1; format=flowed<0x0d><0x0a>Subject: Blue Spreadsheet Highest Priority<0x0d><0x0a>From: "Will Bell" <[e-mail]><0x0d><0x0a>To: "Xavier Clarke" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a><0xda>....................................<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08><0x04><0x00>
 -----
*saveBuf = 0 was executed.
before checking CR NL:
header->length=33
dumping header->value

	CI's comment: note that there is a NUL.

Blue Spreadsheet Highest Priority<0x0d><0x0a>From: "Will Bell" <[e-mail]><0x0d><0x0a>To: "Xavier Clarke" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a><0xda>....................................<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08><0x04><0x00>
 -----
*saveBuf = 0 was executed.
before checking CR NL:
header->length=27
dumping header->value

	CI's comment: note that there is a NUL. (Printing is
	terminated at this NUL.)

"Will Bell" <[e-mail]><0x0d><0x0a>To: "Xavier Clarke" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a><0xda>....................................<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08><0x04><0x00>
 -----
*saveBuf = 0 was executed.
before checking CR NL:
header->length=35
dumping header->value

	CI's comment: note that there is a NUL. (Printing is
	terminated at this NUL.)

"Xavier Clarke" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a><0xda>....................................<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08><0x04><0x00>
 -----
*saveBuf = 0 was executed.
before checking CR NL:
header->length=11
dumping header->value

	CI's comment: note that there is a NUL. (Printing is
	terminated at this NUL.)

	** This is where something goes wrong **

<[e-mail]><0x0d><0x0a><0xda>....................................<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08><0x04><0x00>
 -----
*saveBuf = 0 was executed.
date->length=-2

	CI's comment: Note the strange value of date->lenght, and note
	subsequently PR_ParseTimeStringToExplodedTime() is passed a
	char array full of 0xDA and no terminating NUL (within 1001 position)!

prtime.c: dumping string:
<0xda>..........................................<0xda>...<0xda>...<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda>
 -----
WARNING: PR_ParseTimeStringToExplodedTime scanned more than 1000 chars at line 1637
PR_ParseTimeString: PR_ParseTimeString failed.
--- Dumping original t->value:

    CI's comment: But wait a second! The caller's side stores the
    unmodified original char array which has a terminating NUL at the
    end (!) See below.
    Also note <0x08><0x04> before <0x00>. 

    I count the position in the full log (attached) and 0x00 is at
    1007th octet from the beginning.  So it does not appear in the
    dump of 1001 characters in PR_ParseTimeStringToExplode. Hmm...

<0xda>..........................................<0xda>...<0xda>...<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08><0x04><0x00>
 -----
###!!! ASSERTION: PR_ParseTimeString failed.
: 'PR_SUCCESS == timeStatus', file /TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp, line 1658
nsParseMailMessageState::FinalizeHeaders() (/TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp:1675)
nsParseMailMessageState::ParseFolderLine(char const*, unsigned int) (/TB-NEW/TB-3HG/new-src/mailnews/local/src/nsParseMailbox.cpp:707)

	      [...]


Another bug:

Running test targets in cookies directory
caused the TB test suite to print 
"PR_ParseTimeString: PR_ParseTimeString failed.".
So it seems that there IS ANOTHER FAILURE path. I will investigate
this further.

INFO | (runtestlist.py) | content-tabs: 58 passed, 3 failed
INFO | (runtestlist.py) | Running directory: cookies
['/TB-NEW/TB-3HG/objdir-tb3/./mozilla/_tests/mozmill/../mozmill-virtualenv/bin/python', 'runtest.py', '-t', '/TB-NEW/TB-3HG/new-src/mail/test/mozmill/cookies', '--binary', '../../.././mozilla/dist/bin/thunderbird', '--symbols-path', '/TB-NEW/TB-3HG/objdir-tb3/./mozilla/dist/crashreporter-symbols', '--timeout', '800']
OpenDis.c conn_buf_size =16384
++DOCSHELL 0x812b1a8 == 1 [id = 1]
++DOMWINDOW == 1 (0x8a99584) [serial = 1] [outer = (nil)]
++DOMWINDOW == 2 (0x8a9cc3c) [serial = 2] [outer = 0x8a99528]
Warning: unrecognized command line flag -foreground
++DOCSHELL 0x8c47660 == 2 [id = 2]
++DOMWINDOW == 3 (0x8c49dbc) [serial = 3] [outer = (nil)]
++DOMWINDOW == 4 (0x8c4b434) [serial = 4] [outer = 0x8c49d60]
++DOMWINDOW == 5 (0x8c70ec4) [serial = 5] [outer = 0x8a99528]

	    [...]

--DOMWINDOW == 26 (0x9f86a74) [serial = 17] [outer = 0x94bd130] [url = about:blank]
--DOMWINDOW == 25 (0x8a9cc3c) [serial = 2] [outer = 0x8a99528] [url = about:blank]
WARNING: NS_ENSURE_TRUE(wrapper) failed: file /TB-NEW/TB-3HG/new-src/mozilla/dom/base/nsJSUtils.cpp, line 81
WARNING: NS_ENSURE_TRUE(wrapper) failed: file /TB-NEW/TB-3HG/new-src/mozilla/dom/base/nsJSUtils.cpp, line 81
WARNING: NS_ENSURE_TRUE(wrapper) failed: file /TB-NEW/TB-3HG/new-src/mozilla/dom/base/nsJSUtils.cpp, line 81
PR_ParseTimeString: PR_ParseTimeString failed.
pldhash: for the table at address 0x93881e4, the given entrySize of 88 definitely favors chaining over double hashing.
WARNING: NS_ENSURE_TRUE(currentURI) failed: file /TB-NEW/TB-3HG/new-src/mozilla/content/base/src/ThirdPartyUtil.cpp, line 96
	 [...]

TIA

Comment 22

[Wan-Teh Chang]

chiaki: thank you for debugging this.

You are right about the source of the 0xDA pattern.

The |date| variable can be either &m_date or &m_envelope_date:

1319   date       = (m_date.length       ? &m_date :
1320   m_envelope_date.length ? &m_envelope_date :
1321   0);

You said the message that's causing problem is:

Content-Type: text/plain; charset=ISO-8859-1; format=flowed<0x0d><0x0a>Subject: Blue Spreadsheet Highest Priority<0x0d><0x0a>From: "Will Bell" <[e-mail]><0x0d><0x0a>To: "Xavier Clarke" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a><0xda>....................................<0xda><0xda><0xda><0xda><0xda><0xda><0xda><0xda><0x08>

Note that there is no "Date" header. So m_date.length should still be 0.
This means |date| should be &m_envelope_date. Since date->length is -2,
we should find out why m_envelope_date.length becomes -2.

m_envelope_date.length is set here:

1184   m_envelope_date.value = s;
1185   m_envelope_date.length = (uint16_t) (line_size - (s - m_envelope.GetBuffer()));
1186   while (IS_SPACE (m_envelope_date.value [m_envelope_date.length - 1]))
1187     m_envelope_date.length--;

The bug is most likely in that while loop. I guess that line 1185 sets
m_envelope_date.length to 0, and the previous two characters happen to
be CR LF (<0x0d><0x0a>), so the while loop decrements m_envelope_date.length
to -2.

I guess we can change the while loop to:

  while (m_envelope_date.length > 0 && IS_SPACE (m_envelope_date.value [m_envelope_date.length - 1]))
    m_envelope_date.length--;

Comment 23

[ISHIKAWA, Chiaki]

(In reply to Wan-Teh Chang from comment #22)
> 
> I guess we can change the while loop to:
> 
>   while (m_envelope_date.length > 0 && IS_SPACE (m_envelope_date.value
> [m_envelope_date.length - 1]))
>     m_envelope_date.length--;

Thank you for the suggestion.
I fixed nsParseMailbox.cpp (as suggested and inserted a few more checks
where id->length is also decremented.)

Voila, there is no longer the buggy output from "make mozmill" target of
test_add_contact_from_context_menu.

I noticed that the (-2) was used to write 0 of data->value[-2]

 /* bug 814870 */
  while (m_envelope_date.length > 0 && IS_SPACE (m_envelope_date.value [m_envelope_date.length - 1]))
    m_envelope_date.length--;

  /* #### short-circuit const */
  ((char *) m_envelope_from.value) [m_envelope_from.length] = 0;
  ((char *) m_envelope_date.value) [m_envelope_date.length] = 0; <== HERE


I have no idea what data this statement was breaking when m_envelope_date.length was -2. [Presumably some allocated areas since memcheck didn't complain, but
who knows what was being destroyed :-( ]

I would run the make mozmill test until completion and report any new discovery
concerning this bug.

TIA

Comment 24

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch for nsParseMailbox.cpp with the suggested fix.

I am obsoleting the previous patch.

I included the suggested patch, and also noticed the handling of id->length in one place, and just for completeness's sake, added a check to it.

TIA

Comment 25

[ISHIKAWA, Chiaki]

I wonder if "make mozmill" has a test for checking the sanity of TB behavior when
it receives very long headers such as 
"Date: <many blanks, say, about 975 +/-5 chars> 01-01-2013 01:02 (UTC+00).

You get the idea.

I don't want to think about it, but I may be able to crash 
the current production TB with such e-mails.
Let me try to check this: may take a week since I need to set up a local mail server from which I receive such e-mails and to which I will inject such e-mails by running sendmail (or equivalent) directly. [I have a serious doubt that
my ISP's e-mail soft may have an issue with such e-mail message and
so want to bypass it.]

That TB or outlook will not produce such date header when we send out e-mails doesn't prevent some script kiddies to create such e-mails and cause havoc :-(

Comment 26

[Wan-Teh Chang]

bienvenu: do you know who wrote the nsParseMailMessageState::ParseEnvelope
code in nsParseMailbox.cpp (see attachment 702760 [details] [diff] [review])?

CVS blame shows it was last touched by you in

    1.219 <[e-mail]> 2003-11-14 07:24

but in that revision you merely reformatted the code. So the code was
already there before 2003-11-14. It would be nice if the original author
of that code or you could take a look at this bug and review the patch.

chiaki: good progress! Can this bug explain the other TB valgrind bug
involving PR_ParseTimeString that you filed (bug 806293)?

Could you open a new NSPR bug report for your PR_ParseTimeString patch?
Please omit the reformatting changes. (I understand why you wanted to
reformat the code.) Thank you.

Comment 27

[ISHIKAWA, Chiaki]

(In reply to Wan-Teh Chang from comment #26)
> 
> chiaki: good progress! Can this bug explain the other TB valgrind bug
> involving PR_ParseTimeString that you filed (bug 806293)?
> 

Thanks. I will take a look at valgrind/memcheck run. This takes a looong time
on my PC.

> Could you open a new NSPR bug report for your PR_ParseTimeString patch?
> Please omit the reformatting changes. (I understand why you wanted to
> reformat the code.) Thank you.

Will do. I will only leave the essentials but may take the weekend to finish it
due to other matters.

TIA

Comment 28

[ISHIKAWA, Chiaki]

This is a very brief quick look into memcheck session log of running
"make mozmill" of TB debug build with the debug dump discussed here.

Short version:

I think the routine in nsParseMailbox.cpp wrongly assumed
that the passed area is filled with 0 when there are no
valid data (or something). 

memcheck fills malloc()'ed area with a value from command line option --malloc-fill and thus invalidates this false assumption and cause TB
to behave erratically. (Hmm, maybe this is the cause of the
uninitialized value issue in (bug 806293).)

Anyway, prtime.c is hardened now and we need the checking of
operation involving routines in nsParseMailbox and friends.

Long version:

While dumping the buffer from the
debug statements I added, TB get an initialized value warning!?

It seems that there are areas (maybe a couple of octets or more?)
that are not initialized (i.e., touched by the code) after a series of
valid user data.

Initially, I thought it was OK for this to happen.

e.g.
buffer
[newly used data ... uninitialized data ... \0 ]
					   NUL to terminate?

But again, there is something fishy here.
I don't see NUL in the vicinity of newly used data.

Also, when I compared the log with the earlier run of "make mozmill"
WITHOUT valgrind/memcheck, I see the normal run has NUL
in the expected place (i.e., immediately following <0x0d><0x0a> that
ends a line for a header line.)

(the following line may be folded at unintended place.)

From Normal run.

 ... Content-Base: http://example.com/<0x0d><0x0a>Bcc: Richard Roe <[e-mail]><0x0d><0x0a><0x00>

From Memcheck run.
 ... Content-Base: http://example.com/<0x0d><0x0a>Bcc: Richard Roe <[e-mail]><0x0d><0x0a><0x0a><0xa5><0xa5> ...

<0xa5> is the uninitialized data that triggers the warning.

I run valgrind with the following option. (actually I replace the
thunderbird binary with a binary that invokes thunderbird-bin under valgrind.)

valgrind --trace-children=yes --smc-check=all-non-file --gen-suppressions=all --track-origins=yes --malloc-fill=0xA5 --free-fill=0xC3 --leak-check=full --num-callers=50 --suppressions=$HOME/TB-NEW/TB-3HG/new-src/mozilla/build/valgrind/cross-architecture.sup --suppressions=$HOME/TB-NEW/TB-3HG/new-src/mozilla/build/valgrind/i386-redhat-linux-gnu.sup --suppressions=$HOME/Dropbox/myown.sup --show-possibly-lost=no  ~/TB-NEW/TB-3HG/objdir-tb3/mozilla/dist/bin/thunderbird-bin -profile /TB-NEW/TB-3HG/objdir-tb3/mozilla/_tests/mozmill/mozmillprofile -jsbridge 24242 -foreground

Note the use of --malloc-fill=0xA5.
So memcheck fills malloc'ed area by 0xA5 before passing it to the user
program. (So unless memcheck has a bug to fill calloc()'ed area to be
filled with zero, with this 0xA5, TB ought to fill the
passed area explicitly with zero if it expects the unused area 
by filling the newly alloc'ed area explicitly.)

I am not useing jremalloc library by the way by following the tips
to compile TB so that it runs well under valgrind.

In memcheck manual, the use of --malloc-fill option is advised to
track down hard-to-find memory problems. I think we hit one here.

Anyway, my conclusion is that the if the buffer is filled with non-NUL
value, routines in nsParseMailbox seems to misbehave (there may be
a deeper cause.)

This is the first thing I noticed from still running "make mozmill"
memcheck run under valgrind.

I mark my comment with "[***]" below in the excerpt from memcheck run.

--- EXCERPT from memcheck run.

[***] A new header is parsed: it is a very long subject test(!), but
the problem seems to occur in parsing the addresses.

ParseHeaders entered.
ParseHeaders buf.

Content-Type: text/plain; charset=ISO-8859-1; format=flowed<0x0d><0x0a>Subject: This is a really, really, really, really, really, really, really, really, long subject.<0x0d><0x0a>From: "Ulf Ulvaeus" <[e-mail]><0x0d><0x0a>To: "Vince Vannucci" <[e-mail]><0x0d><0x0a>Cc: "Andy Anway" <[e-mail]>,<0x0d><0x0a><0x09>"Bob Bell" <[e-mail]>,<0x0d><0x0a><0x09>"Chris Clarke" <[e-mail]>,<0x0d><0x0a><0x09>"David Davol" <[e-mail]>,<0x0d><0x0a><0x09>"Emily Ekberg" <[e-mail]>,<0x0d><0x0a><0x09>"Felix Flowers" <[e-mail]>,<0x0d><0x0a><0x09>"Gillian Gilbert" <[e-mail]>,<0x0d><0x0a><0x09>"Helen Hook" <[e-mail]>,<0x0d><0x0a><0x09>"Idina Ivarsson" <[e-mail]>,<0x0d><0x0a><0x09>"Johnny Jones" <[e-mail]>,<0x0d><0x0a><0x09>"Kate Kurtz" <[e-mail]>,<0x0d><0x0a><0x09>"Lilia Lowe" <[e-mail]>,<0x0d><0x0a><0x09>"Martin Morris" <[e-mail]>,<0x0d><0x0a><0x09>"Neil Nagel" <[e-mail]>,<0x0d><0x0a><0x09>"Olof Orzabal" <[e-mail]>,<0x0d><0x0a><0x09>"Pete Price" <[e-mail]>,<0x0d><0x0a><0x09>"Quinn Quinn" <[e-mail]>,<0x0d><0x0a><0x09>"Rasmus Rolinski" <[e-mail]>,<0x0d><0x0a><0x09>"Sarah Stanley" <[e-mail]>,<0x0d><0x0a><0x09>"Troels Tennant" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a>Date: Tue, 01 Feb 2000 00:0
 -----
before checking CR NL:
header->length=45
dumping header->value

text/plain; charset=ISO-8859-1; format=flowed<0x0d><0x0a>Subject: This is a really, really, really, really, really, really, really, really, long subject.<0x0d><0x0a>From: "Ulf Ulvaeus" <[e-mail]><0x0d><0x0a>To: "Vince Vannucci" <[e-mail]><0x0d><0x0a>Cc: "Andy Anway" <[e-mail]>,<0x0d><0x0a><0x09>"Bob Bell" <[e-mail]>,<0x0d><0x0a><0x09>"Chris Clarke" <[e-mail]>,<0x0d><0x0a><0x09>"David Davol" <[e-mail]>,<0x0d><0x0a><0x09>"Emily Ekberg" <[e-mail]>,<0x0d><0x0a><0x09>"Felix Flowers" <[e-mail]>,<0x0d><0x0a><0x09>"Gillian Gilbert" <[e-mail]>,<0x0d><0x0a><0x09>"Helen Hook" <[e-mail]>,<0x0d><0x0a><0x09>"Idina Ivarsson" <[e-mail]>,<0x0d><0x0a><0x09>"Johnny Jones" <[e-mail]>,<0x0d><0x0a><0x09>"Kate Kurtz" <[e-mail]>,<0x0d><0x0a><0x09>"Lilia Lowe" <[e-mail]>,<0x0d><0x0a><0x09>"Martin Morris" <[e-mail]>,<0x0d><0x0a><0x09>"Neil Nagel" <[e-mail]>,<0x0d><0x0a><0x09>"Olof Orzabal" <[e-mail]>,<0x0d><0x0a><0x09>"Pete Price" <[e-mail]>,<0x0d><0x0a><0x09>"Quinn Quinn" <[e-mail]>,<0x0d><0x0a><0x09>"Rasmus Rolinski" <[e-mail]>,<0x0d><0x0a><0x09>"Sarah Stanley" <[e-mail]>,<0x0d><0x0a><0x09>"Troels Tennant" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a>Date: Tue, 01 Feb 2000 00:00:00 +0900<0x0d><0x0a>Ne
 -----
*saveBuf = 0 was executed.

[***] So obviously the header(s) as a whole is much longer than 1000
octes and so we don't see NUL. So far, so good.

before checking CR NL:
header->length=87
dumping header->value

This is a really, really, really, really, really, really, really, really, long subject.<0x0d><0x0a>From: "Ulf Ulvaeus" <[e-mail]><0x0d><0x0a>To: "Vince Vannucci" <[e-mail]><0x0d><0x0a>Cc: "Andy Anway" <[e-mail]>,<0x0d><0x0a><0x09>"Bob Bell" <[e-mail]>,<0x0d><0x0a><0x09>"Chris Clarke" <[e-mail]>,<0x0d><0x0a><0x09>"David Davol" <[e-mail]>,<0x0d><0x0a><0x09>"Emily Ekberg" <[e-mail]>,<0x0d><0x0a><0x09>"Felix Flowers" <[e-mail]>,<0x0d><0x0a><0x09>"Gillian Gilbert" <[e-mail]>,<0x0d><0x0a><0x09>"Helen Hook" <[e-mail]>,<0x0d><0x0a><0x09>"Idina Ivarsson" <[e-mail]>,<0x0d><0x0a><0x09>"Johnny Jones" <[e-mail]>,<0x0d><0x0a><0x09>"Kate Kurtz" <[e-mail]>,<0x0d><0x0a><0x09>"Lilia Lowe" <[e-mail]>,<0x0d><0x0a><0x09>"Martin Morris" <[e-mail]>,<0x0d><0x0a><0x09>"Neil Nagel" <[e-mail]>,<0x0d><0x0a><0x09>"Olof Orzabal" <[e-mail]>,<0x0d><0x0a><0x09>"Pete Price" <[e-mail]>,<0x0d><0x0a><0x09>"Quinn Quinn" <[e-mail]>,<0x0d><0x0a><0x09>"Rasmus Rolinski" <[e-mail]>,<0x0d><0x0a><0x09>"Sarah Stanley" <[e-mail]>,<0x0d><0x0a><0x09>"Troels Tennant" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a>Date: Tue, 01 Feb 2000 00:00:00 +0900<0x0d><0x0a>Newsgroups: alt.test<0x0d><0x0a>Reply-To: J. Doe <[e-mail]
 -----
*saveBuf = 0 was executed.

[***] Note that addresses in total result in a long series of lines.
<0x0d><0x0a> == CR LF

[***] in the following dumping of the header, we get the problem.
There seemed to be a couple (? a single or more?) uninitialized octets in the
header->value area (before we see NUL). I am dumping 1001 octets and
wonder if this is at 1000 or 1001. But obviously,
there are other octets that get dumped by dumpstring and so the
problematic uninitialized (?) octets are NOT at 1000th or 1001th postiion.
Then after a while, I realize that 0xA5 is the filler value used by
memcheck for malloc()'ed area and thus, this non-zero value is essentially
untouched (uninitialized)  since malloc(). 

So I suppose before copying the header into newly parsed buffer area, we need
to zero out the new area first?



before checking CR NL:
header->length=31
dumping header->value

"Ulf Ulvaeus" <[e-mail]><0x0d><0x0a>To: "Vince Vannucci" <[e-mail]><0x0d><0x0a>Cc: "Andy Anway" <[e-mail]>,<0x0d><0x0a><0x09>"Bob Bell" <[e-mail]>,<0x0d><0x0a><0x09>"Chris Clarke" <[e-mail]>,<0x0d><0x0a><0x09>"David Davol" <[e-mail]>,<0x0d><0x0a><0x09>"Emily Ekberg" <[e-mail]>,<0x0d><0x0a><0x09>"Felix Flowers" <[e-mail]>,<0x0d><0x0a><0x09>"Gillian Gilbert" <[e-mail]>,<0x0d><0x0a><0x09>"Helen Hook" <[e-mail]>,<0x0d><0x0a><0x09>"Idina Ivarsson" <[e-mail]>,<0x0d><0x0a><0x09>"Johnny Jones" <[e-mail]>,<0x0d><0x0a><0x09>"Kate Kurtz" <[e-mail]>,<0x0d><0x0a><0x09>"Lilia Lowe" <[e-mail]>,<0x0d><0x0a><0x09>"Martin Morris" <[e-mail]>,<0x0d><0x0a><0x09>"Neil Nagel" <[e-mail]>,<0x0d><0x0a><0x09>"Olof Orzabal" <[e-mail]>,<0x0d><0x0a><0x09>"Pete Price" <[e-mail]>,<0x0d><0x0a><0x09>"Quinn Quinn" <[e-mail]>,<0x0d><0x0a><0x09>"Rasmus Rolinski" <[e-mail]>,<0x0d><0x0a><0x09>"Sarah Stanley" <[e-mail]>,<0x0d><0x0a><0x09>"Troels Tennant" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a>Date: Tue, 01 Feb 2000 00:00:00 +0900<0x0d><0x0a>Newsgroups: alt.test<0x0d><0x0a>Reply-To: J. Doe <[e-mail]><0x0d><0x0a>Content-Base: http://example.com/<0x0d><0x0a>Bcc: Richard Roe <[e-mail]><0x0d><0x0a>==5638== Use of uninitialised value of size 4
==5638==    at 0x41BBF4F: isprint (ctype.h:30)
==5638==    by 0xA66A5A3: nsParseMailMessageState::ParseHeaders() (nsParseMailbox.cpp:1145)
==5638==    by 0xA6757AA: nsParseMailMessageState::ParseFolderLine(char const*, unsigned int) (nsParseMailbox.cpp:703)
==5638==    by 0xA669928: nsMsgMailboxParser::HandleLine(char*, unsigned int) (nsParseMailbox.cpp:511)
	    ...

[***] Note that after "<[e-mail]><0x0d><0x0a>"
we have uninitialized octet.
Later it turns out to be a value of <0xA5>.

	    ...
==5638==    by 0x9ED9D10: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2385)
==5638==    by 0x9EE11BB: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==5638==
{
   <insert_a_suppression_name_here>
   Memcheck:Value4
   fun:_itoa_word
   fun:vfprintf
   fun:buffered_vfprintf
   fun:vfprintf
   fun:fprintf
   fun:_ZL10dumpstringPKhPKc
   fun:_ZN23nsParseMailMessageState12ParseHeadersEv
   fun:_ZN23nsParseMailMessageState15ParseFolderLineEPKcj
   fun:_ZN18nsMsgMailboxParser10HandleLineEPcj
   fun:_ZN15nsMsgLineBuffer20ConvertAndSendBufferEv
   fun:_ZN15nsMsgLineBuffer11BufferInputEPKci
   fun:_ZN20nsMsgLocalMailFolder15AddMessageBatchEjPPKcPP8nsIArray
   fun:NS_InvokeByIndex_P
   fun:_ZN16CallMethodHelper4CallEv
   fun:_ZN16XPCWrappedNative10CallMethodER14XPCCallContextNS_8CallModeE
   fun:_Z17XPC_WN_CallMethodP9JSContextjPN2JS5ValueE
   fun:_ZN2js12CallJSNativeEP9JSContextPFiS1_jPN2JS5ValueEERKNS2_8CallArgsE
   fun:_ZN2js12InvokeKernelEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
   fun:_ZN2js9InterpretEP9JSContextPNS_10StackFrameENS_10InterpModeE
   fun:_ZN2js9RunScriptEP9JSContextN2JS6HandleIP8JSScriptEEPNS_10StackFrameE
   fun:_ZN2js12InvokeKernelEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
   fun:_Z12js_fun_applyP9JSContextjPN2JS5ValueE
   fun:_ZN2js12CallJSNativeEP9JSContextPFiS1_jPN2JS5ValueEERKNS2_8CallArgsE
   fun:_ZN2js12InvokeKernelEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
}
==5638== Conditional jump or move depends on uninitialised value(s)
==5638==    at 0x41D70E3: _itoa_word (_itoa.c:196)
==5638==    by 0x41DA98B: vfprintf (vfprintf.c:1622)
==5638==    by 0x41DCBB1: buffered_vfprintf (vfprintf.c:2289)
==5638==    by 0x41D7DC2: vfprintf (vfprintf.c:1309)
==5638==    by 0x41E1E4E: fprintf (fprintf.c:33)
==5638==    by 0xA668F6B: dumpstring(unsigned char const*, char const*) (nsParseMailbox.cpp:79)
==5638==    by 0xA66A5A3: nsParseMailMessageState::ParseHeaders() (nsParseMailbox.cpp:1145)
	    ...

==5638==    by 0x9ED9D10: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2385)
==5638==    by 0x9EE11BB: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==5638==
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:_itoa_word
   fun:vfprintf
   fun:buffered_vfprintf
   fun:vfprintf
   fun:fprintf
   fun:_ZL10dumpstringPKhPKc
   fun:_ZN23nsParseMailMessageState12ParseHeadersEv
   fun:_ZN23nsParseMailMessageState15ParseFolderLineEPKcj
   fun:_ZN18nsMsgMailboxParser10HandleLineEPcj
   fun:_ZN15nsMsgLineBuffer20ConvertAndSendBufferEv
   fun:_ZN15nsMsgLineBuffer11BufferInputEPKci
   fun:_ZN20nsMsgLocalMailFolder15AddMessageBatchEjPPKcPP8nsIArray
   fun:NS_InvokeByIndex_P
   fun:_ZN16CallMethodHelper4CallEv
   fun:_ZN16XPCWrappedNative10CallMethodER14XPCCallContextNS_8CallModeE
   fun:_Z17XPC_WN_CallMethodP9JSContextjPN2JS5ValueE
   fun:_ZN2js12CallJSNativeEP9JSContextPFiS1_jPN2JS5ValueEERKNS2_8CallArgsE
   fun:_ZN2js12InvokeKernelEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
   fun:_ZN2js9InterpretEP9JSContextPNS_10StackFrameENS_10InterpModeE
   fun:_ZN2js9RunScriptEP9JSContextN2JS6HandleIP8JSScriptEEPNS_10StackFrameE
   fun:_ZN2js12InvokeKernelEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
   fun:_Z12js_fun_applyP9JSContextjPN2JS5ValueE
   fun:_ZN2js12CallJSNativeEP9JSContextPFiS1_jPN2JS5ValueEERKNS2_8CallArgsE
   fun:_ZN2js12InvokeKernelEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
}
<0xa5>		       <------ [***] here is the uninialized value.
==5638== Conditional jump or move depends on uninitialised value(s)
==5638==    at 0xA668F6E: dumpstring(unsigned char const*, char const*) (nsParseMailbox.cpp:80)
==5638==    by 0xA66A5A3: nsParseMailMessageState::ParseHeaders() (nsParseMailbox.cpp:1145)
==5638==    by 0xA6757AA: nsParseMailMessageState::ParseFolderLine(char const*, unsigned int) (nsParseMailbox.cpp:703)
	    ...

<0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5>
 -----

[***] It is a little surprising to see uninitialized values
IMMEDIATELY following the valid string without seeing NUL.
I wonder if the logic of inserting NUL (somewhere) is correct in
deciding where to place NUL.

*saveBuf = 0 was executed.
before checking CR NL:
header->length=37
dumping header->value

"Vince Vannucci" <[e-mail]><0x0d><0x0a>Cc: "Andy Anway" <[e-mail]>,<0x0d><0x0a><0x09>"Bob Bell" <[e-mail]>,<0x0d><0x0a><0x09>"Chris Clarke" <[e-mail]>,<0x0d><0x0a><0x09>"David Davol" <[e-mail]>,<0x0d><0x0a><0x09>"Emily Ekberg" <[e-mail]>,<0x0d><0x0a><0x09>"Felix Flowers" <[e-mail]>,<0x0d><0x0a><0x09>"Gillian Gilbert" <[e-mail]>,<0x0d><0x0a><0x09>"Helen Hook" <[e-mail]>,<0x0d><0x0a><0x09>"Idina Ivarsson" <[e-mail]>,<0x0d><0x0a><0x09>"Johnny Jones" <[e-mail]>,<0x0d><0x0a><0x09>"Kate Kurtz" <[e-mail]>,<0x0d><0x0a><0x09>"Lilia Lowe" <[e-mail]>,<0x0d><0x0a><0x09>"Martin Morris" <[e-mail]>,<0x0d><0x0a><0x09>"Neil Nagel" <[e-mail]>,<0x0d><0x0a><0x09>"Olof Orzabal" <[e-mail]>,<0x0d><0x0a><0x09>"Pete Price" <[e-mail]>,<0x0d><0x0a><0x09>"Quinn Quinn" <[e-mail]>,<0x0d><0x0a><0x09>"Rasmus Rolinski" <[e-mail]>,<0x0d><0x0a><0x09>"Sarah Stanley" <[e-mail]>,<0x0d><0x0a><0x09>"Troels Tennant" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a>Date: Tue, 01 Feb 2000 00:00:00 +0900<0x0d><0x0a>Newsgroups: alt.test<0x0d><0x0a>Reply-To: J. Doe <[e-mail]><0x0d><0x0a>Content-Base: http://example.com/<0x0d><0x0a>Bcc: Richard Roe <[e-mail]><0x0d><0x0a><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5><0xa5>
 -----
*saveBuf = 0 was executed.

...


[***] Now compare this.

Excerpt from the corresponding log from the normal run of "make mozmill" (i.e.,
without memcheck). Note the proper termination immediately after CR
NL.

before checking CR NL:
header->length=31
dumping header->value

"Ulf Ulvaeus" <[e-mail]><0x0d><0x0a>To: "Vince Vannucci" <[e-mail]><0x0d><0x0a>Cc: "Andy Anway" <[e-mail]>,<0x0d><0x0a><0x09>"Bob Bell" <[e-mail]>,<0x0d><0x0a><0x09>"Chris Clarke" <[e-mail]>,<0x0d><0x0a><0x09>"David Davol" <[e-mail]>,<0x0d><0x0a><0x09>"Emily Ekberg" <[e-mail]>,<0x0d><0x0a><0x09>"Felix Flowers" <[e-mail]>,<0x0d><0x0a><0x09>"Gillian Gilbert" <[e-mail]>,<0x0d><0x0a><0x09>"Helen Hook" <[e-mail]>,<0x0d><0x0a><0x09>"Idina Ivarsson" <[e-mail]>,<0x0d><0x0a><0x09>"Johnny Jones" <[e-mail]>,<0x0d><0x0a><0x09>"Kate Kurtz" <[e-mail]>,<0x0d><0x0a><0x09>"Lilia Lowe" <[e-mail]>,<0x0d><0x0a><0x09>"Martin Morris" <[e-mail]>,<0x0d><0x0a><0x09>"Neil Nagel" <[e-mail]>,<0x0d><0x0a><0x09>"Olof Orzabal" <[e-mail]>,<0x0d><0x0a><0x09>"Pete Price" <[e-mail]>,<0x0d><0x0a><0x09>"Quinn Quinn" <[e-mail]>,<0x0d><0x0a><0x09>"Rasmus Rolinski" <[e-mail]>,<0x0d><0x0a><0x09>"Sarah Stanley" <[e-mail]>,<0x0d><0x0a><0x09>"Troels Tennant" <[e-mail]><0x0d><0x0a>Message-Id: <[e-mail]><0x0d><0x0a>Date: Tue, 01 Feb 2000 00:00:00 +0900<0x0d><0x0a>Newsgroups: alt.test<0x0d><0x0a>Reply-To: J. Doe <[e-mail]><0x0d><0x0a>Content-Base: http://example.com/<0x0d><0x0a>Bcc: Richard Roe <[e-mail]><0x0d><0x0a><0x00>
 -----

[***] Note the <0x0d><0x0a><0x00> sequence at the end.

TIA

Comment 29

[ISHIKAWA, Chiaki]

One additional point. This seems to happen in the initialization phase for message-header directory target of the
mozmill test suite (as opposed to a particular test target run). Meaning that this is while reading an article from the message folder (?). Obviously depending on the uninitialized value in the memory buffer, the error apppears/does not appear and so hard to track down. But I bet memcheck version will persistently (hopefully)

TIA

Comment 30

[Wan-Teh Chang]

chiaki: in comment 28 you said "prtime.c is hardened now".  I am very
interested in knowing if there are any memory errors when prtime.c is
NOT patched. Thanks.

I don't remember why PR_ParseTimeStringToExploded imposes a maximum
iteration count of 1000. There are two possible reasons:

1. A valid time string cannot possibly be longer than 1000 characters.
So this is a form of input validation.

2. It allows PR_ParseTimeStringToExploded to terminate if there is
a bug, such as skipping over the terminating null byte by mistake.
Note: PR_ParseTimeStringToExploded only supports null-terminated C
strings.

The if (iterations++ > 1000) test has been there since rev. 1.1 in
the public CVS repository, so I can't find out more about this from
CVS history.

Comment 31

[ISHIKAWA, Chiaki]

(In reply to Wan-Teh Chang from comment #30)
> chiaki: in comment 28 you said "prtime.c is hardened now".  I am very
> interested in knowing if there are any memory errors when prtime.c is
> NOT patched. Thanks.
> 

I answer the following first.

> The if (iterations++ > 1000) test has been there since rev. 1.1 in
> the public CVS repository, so I can't find out more about this from
> CVS history.

I think this comes from the line limit legnth discussed in Internet Message Format RFC.
From http://tools.ietf.org/html/rfc5322#section-2.1.1

2.1.1.  Line Length Limits

   There are two limits that this specification places on the number of
   characters in a line.  Each line of characters MUST be no more than
   998 characters, and SHOULD be no more than 78 characters, excluding
   the CRLF.

   The 998 character limit is due to limitations in many implementations
   that send, receive, or store IMF messages which simply cannot handle
   more than 998 characters on a line.  Receiving implementations would
   do well to handle an arbitrarily large number of characters in a line
   for robustness sake.  However, there are so many implementations that
   (in compliance with the transport requirements of [RFC5321]) do not
   accept messages containing more than 1000 characters including the CR
   and LF per line, it is important for implementations not to create
   such messages.

   The more conservative 78 character recommendation is to accommodate
   the many implementations of user interfaces that display these
   messages which may truncate, or disastrously wrap, the display of
   more than 78 characters per line, in spite of the fact that such
   implementations are non-conformant to the intent of this
   specification (and that of [RFC5321] if they actually cause
   information to be lost).  Again, even though this limitation is put
   on messages, it is incumbent upon implementations that display
   messages to handle an arbitrarily large number of characters in a
   line (certainly at least up to the 998 character limit) for the sake

In this sense, TB is an implementation that does not accept lines longer than 1000 (or so. I think 1000 includes the end of line CR LF and terminating NUL.)
Not entirely sure what happens when a line goes over 1000. 
(My tests this morning suggest that if header lines are very long, TB fails to receive and store the messages. It pulls such a message from a POP3 server and dumps some error messages, but the message does not stay in the folder. At least, the patched version doesn't crash.)

> if there are any memory errors when prtime.c is
> NOT patched.

The checking in prtime.c was rather loose in prtime.c.
If a bug in the caller side calls this routine with non-NUL terminated string
and uninitialized area filled with non-zero values, it will go out to lunch until
it hits on NUL, meaning that it will access outside the valid memory range (as memcheck
spotted in the past.)

Although at the beginning of big while statement, the
variable iterations are checked, but in the while body, you see small loops such as
the following:  This is the biggest offender.

1446           /* Skip to the end of this token, whether we parsed it or not.
1447                  Tokens are delimited by whitespace, or ,;-/
1448                  But explicitly not :+-.
1449            */
1450           while (*rest &&
1451                          *rest != ' ' && *rest != '\t' &&
1452                          *rest != ',' && *rest != ';' &&
1453                          *rest != '-' && *rest != '+' &&
1454                          *rest != '/' &&
1455                          *rest != '(' && *rest != ')' && *rest != '[' && *rest != ']')
1456                 rest++;
1457           /* skip over uninteresting chars. */
 
Well, the value of |res|t is not checked in this sub-loop. Suppose the |rest| points at an
area filled with 0xDA or 0xA5. Then this will loop r until it will go outside the previously
allocated area to the following memory area (uncharted territory) and until it sees one of the characters, it won't stop. If the malloc()'ed area filled with 0xDA or 0xA5 is at the end of user memory space obtained by sbrk() or brk(), a segfault occurs when you go beyond it.

There are smaller loops such as

1203                         const char *end = rest + 1;
1204                         while (*end >= '0' && *end <= '9')
1205                           end++;
1206 
1207                         /* end is now the first character after a range of digits. */
1208 
1209                         if (*end == ':')
1210

where the value of |end| is not checked if it falls within 1000 octets from the value of |string|,
I felt compelled to insert checks mechanically.

After inserting these checks, PR_ParseTimeStringToExplodedTime() notice the error conditions and begin returning error value properly when the scanning goes over 1000. 
                         
So "harden" may be too strong a word. Properly enforcing the intended check may be a better description.

As I write this,  now I see that the check for |iterations| at the beginning of 
loop ought to be changed into the check of (rest - string) instead of |iterations|.

e.g. Original:
971           if (iterations++ > 1000)
972                 {
973                   return PR_FAILURE;
974                 }

My patch:
1056 	BOUNDCHECK(iterations);　 <==== should be replaced with the check of (rest - string)
1057 	iterations ++;

This is because |iterations| is only bumped once here while |rest| is incremented in many places. (or was it the intention of the original coder to check for 1000 tokens? Given RFC's mention of 998 chars, I think 1000 refers to octets. Then the check is clearly wrong.)

I will fix my patch to prtime.c accordingly and see if it changes any thing in the normal run of "make mozmll" test of TB.

TIA

Comment 32

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch for prtime.c (hg diff -w)

Changed according to the previous patch (BOUNDCHECK(rest - string)) in one place.

Comment 33

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch for nsParseMailbox.cpp with the suggested fix. (hg diff -w )

A patch changed to print buffer only up to a given length for debugging :
my previous dumpstring() tried to print up to 1000 no matter what, and it turned out THIS dumpsring() was accessing uninitialized area :-(
So now that length is good for all cases (no more (-2)), I use that length value
to print only up to the specified length.

I am running make "mozmill test" under memcheck, it will take a day almost, and so will report the result after the weekend...

Comment 34

[ISHIKAWA, Chiaki]

(In reply to Wan-Teh Chang from comment #26)
> chiaki: good progress! Can this bug explain the other TB valgrind bug
> involving PR_ParseTimeString that you filed (bug 806293)?

Yes.
Looks like this can explain the issue since I don't particularly see the
issue in the recent memcheck session log any more.
Good!

(I might have triggered a bug of libc on Debian GNU/Linux 32-bit versin, though. 
Now I get a strange invalid memory access bug as followse: 
length=31
dump upperlimit=31
Tue, 01 Feb 2000 02:00:00 +0900
 -----
*saveBuf = 0 was executed.
date->length=31
==24420== Invalid read of size 8
==24420==    at 0x42B719F: __bcopy_ssse3 (memcpy-ssse3.S:720)
==24420==    by 0xA67585C: nsParseMailMessageState::ParseFolderLine(char const*, unsigned int) (nsParseMailbox.cpp:714)
==24420==    by 0xA6699A0: nsMsgMailboxParser::HandleLine(char*, unsigned int) (nsParseMailbox.cpp:518)
  
I filed a bug report for valgrind:
https://bugs.kde.org/show_bug.cgi?id=311407
It seems that the library substition of bcopy(memcpy) is so eager to use the
wide memory access of ssse3 instruction that it oversteps the boundary somehow.
It was reported it does not happen on Ubuntu 64bit, and so I suspect this
may be due to 32 bit Debian GNU/Linux system. But I digress.)

> Could you open a new NSPR bug report for your PR_ParseTimeString patch?
> Please omit the reformatting changes. (I understand why you wanted to
> reformat the code.) Thank you.

I filed a bug entry:

Bug 832932 - NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

I only inserted the bound-check (with the removal of trailing blanks.)
I have omitted the use of new macros and the support of CET (Central European Time.)
Would you take the review?

Comment 35

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch for nsParseMailbox.cpp with the suggested fix.

This is the final current version which I think solves the problem reported earlier.
I have tested this with the combination of the latest prtime.c by running
"make mozill" test (with and without valgrind), and
they work without the strange issue any more.

TIA

Comment 36

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch for nsParseMailbox.cpp with the suggested fix. A correction

A work-in-progress patch for nsParseMailbox.cpp with the suggested fix. 
A correction.
In one place (for debugging), I was copying too many bytes for later
dumping,  and thus limit the copying with proper length.
1024 => date->length

+          bcopy(&date->value[0], tbuf, date->length);

No more memcheck warning related to the routine(s).

TIA

Comment 37

[Wan-Teh Chang]

Comment on attachment 705259 [details] [diff] [review]
A work-in-progress patch for nsParseMailbox.cpp with the suggested fix.  A correction

Review of attachment 705259 [details] [diff] [review]:
-----------------------------------------------------------------

Gary (:gkw): could you find the right TB developer to review the patches for this bug?

chiaki: thanks a lot for the patch. I suggest some changes below. In addition, please
remove all the debugging printf and dumpstring changes from the patch. Most of that
code was commented out with #if 0, so it is likely to "bit rot" in the future. Also
it probably only makes sense to you.

::: mailnews/local/src/nsParseMailbox.cpp
@@ +1198,5 @@
>                m_receivedTime = resultTime;
> +            else
> +            {
> +              NS_ASSERTION(false, "PR_ParseTimeString failed.\n");
> +            }

If you use curly braces on the "else" branch, you should also
use curly braces on the "if" branch.

I suggest omitting curly braces. That seems to be the style
used in this file.

@@ +1424,5 @@
>        if (id->value[0] == '<')
> +      {
> +        id->value++;
> +        if(id-> length > 0)
> +          id->length--;

This is wrong. id->value should only be incremented when id->length
is decremented, otherwise they will become inconsistent.

@@ +1710,5 @@
>              PRTime2Seconds(resultTime, &rcvTimeSecs);
> +          else
> +          {
> +            NS_ASSERTION(PR_SUCCESS == timeStatus, "PR_ParseTimeString failed.\n");
> +          }

Omit curly braces.

Comment 38

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 705259 [details] [diff] [review]
A work-in-progress patch for nsParseMailbox.cpp with the suggested fix.  A correction

Moving review on to standard8, whom I think is the right person to review. Please change as necessary.

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 705259 [details] [diff] [review]
A work-in-progress patch for nsParseMailbox.cpp with the suggested fix.  A correction

I'd be more comfortable with David reviewing this, as I suspect he's got a better history around this code than I have.

Comment 40

[David :Bienvenu]

Comment on attachment 705259 [details] [diff] [review]
A work-in-progress patch for nsParseMailbox.cpp with the suggested fix.  A correction

thx for the patch.

-        id->value++, id->length--;
+      {
+        id->value++;
+        if(id-> length > 0)
+          id->length--;
+        else
+        {
+          NS_WARNING("Tried to decrement 0 length value!.");
+        }
+      }

This would be better as 

if (id->length > 0 && id->value[0] == '<') 

this looks OK:

+  /* bug 814870 */
+  while (m_envelope_date.length > 0 && IS_SPACE (m_envelope_date.value [m_envelope_date.length - 1]))
     m_envelope_date.length--;

(except that the line is a bit too long, I think).

I can't r+ it because of all the debugging cruft. I'm also not sure if the added assertions are going to be painful and perhaps should be warnings.

Comment 41

[ISHIKAWA, Chiaki]

(In reply to David :Bienvenu from comment #40)
> Comment on attachment 705259 [details] [diff] [review]
> A work-in-progress patch for nsParseMailbox.cpp with the suggested fix.  A
> correction
> 
> thx for the patch.
> 
> -        id->value++, id->length--;
> +      {
> +        id->value++;
> +        if(id-> length > 0)
> +          id->length--;
> +        else
> +        {
> +          NS_WARNING("Tried to decrement 0 length value!.");
> +        }
> +      }
> 
> This would be better as 
> 
> if (id->length > 0 && id->value[0] == '<') 
> 
> this looks OK:
> 
> +  /* bug 814870 */
> +  while (m_envelope_date.length > 0 && IS_SPACE (m_envelope_date.value
> [m_envelope_date.length - 1]))
>      m_envelope_date.length--;
> 
> (except that the line is a bit too long, I think).
> 
> I can't r+ it because of all the debugging cruft. I'm also not sure if the
> added assertions are going to be painful and perhaps should be warnings.

Thank you for the comment.
I am attaching a cleaned up patch that removes the debugging cruft and
incorporated your suggestions.

Comment 42

[ISHIKAWA, Chiaki]

Attachment: Improved patch, take 2.

Comment 43

[Wan-Teh Chang]

Comment on attachment 705910 [details] [diff] [review]
Improved patch, take 2.

Review of attachment 705910 [details] [diff] [review]:
-----------------------------------------------------------------

Please consider my review as advisory only.

I suggest removing all the #if DEBUG code.

::: mailnews/local/src/nsParseMailbox.cpp
@@ +1176,5 @@
>              PRTime resultTime;
>              if (PR_ParseTimeString (receivedDate.get(), false, &resultTime) == PR_SUCCESS)
>                m_receivedTime = resultTime;
> +            else
> +              NS_WARNING("PR_ParseTimeString failed near L1180.\n");

The line number "near L1180" should be removed from the warning message because
it is likely to change when this file is modified.

@@ +1182,5 @@
>          }
>          // Someone might want the received header saved.
>          if (m_customDBHeaders.Length())
>          {
> +          /* was uint32_t: incorrect!? */

Remove this comment and revert this change.

I looked into this. IndexOf() actually does return uint32_t.

I think we should replace -1 with static_cast<uint32_t>(-1).

Instead of -1, you can use the nsTArray<nsCString>::NoIndex enum defined in
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsTArray.h

You can also use nsTArray<nsCString>::index_type instead of uint32_t.

@@ +1218,5 @@
>      s++;
>    m_envelope_date.value = s;
>    m_envelope_date.length = (uint16_t) (line_size - (s - m_envelope.GetBuffer()));
> +
> +  /* bug 814870 */

Remove this comment.

@@ +1404,5 @@
> +      {
> +        id->length--, id->value++;
> +      }
> +
> +      if(id->length <= 0)

Nit: add a space after "if".

@@ +1405,5 @@
> +        id->length--, id->value++;
> +      }
> +
> +      if(id->length <= 0)
> +        NS_WARNING("id->length failure.: Not positive!");

Is it OK to continue to the next statement and deference id->value[id->length - 1]
if id->length <= 0? That seems bad.

@@ +1655,5 @@
> +#endif
> +            NS_WARNING("PR_ParseTimeString failed near L1656.\n");
> +          }
> +
> +

Remove these two blank lines,

Comment 44

[ISHIKAWA, Chiaki]

Attachment: Improved patch, take 3.

I incorporated suggestions from the following.


>Please consider my review as advisory only.
>
>I suggest removing all the #if DEBUG code.

I did this. Basically, the code to dump the buffer in #if DEBUG code is now held in prtime.c. 

But the nit is that there is no easy way to locate where problematic call to
PR_ParseTimeString() is made. [We can't use the NS_ASSERTION() and its backtrace feature in NSPR library code, it seems :-( ]

::: mailnews/local/src/nsParseMailbox.cpp
@@ +1176,5 @@
>>              PRTime resultTime;
>>              if (PR_ParseTimeString (receivedDate.get(), false, &resultTime) == PR_SUCCESS)
>>                m_receivedTime = resultTime;
>> +            else
>> +              NS_WARNING("PR_ParseTimeString failed near L1180.\n");
>
>The line number "near L1180" should be removed from the warning message because
>it is likely to change when this file is modified.

The label was necessary to figure out where the call to PR_ParseTimeString() is done. Now I inserted the function name within the string instead. That is good
enough, and I don't think the function name changes frequently now that this code has been around for more than 10 years.

@@ +1182,5 @@
>          }
>          // Someone might want the received header saved.
>          if (m_customDBHeaders.Length())
>          {
> +          /* was uint32_t: incorrect!? */

>Remove this comment and revert this change.
>
>I looked into this. IndexOf() actually does return uint32_t.
>
>I think we should replace -1 with static_cast<uint32_t>(-1).
>
>Instead of -1, you can use the nsTArray<nsCString>::NoIndex enum defined in
>http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/glue/nsTArray.h
>
>You can also use nsTArray<nsCString>::index_type instead of uint32_t.

OK, I used  nsTArray<nsCString>::NoIndex for "-1".
I looked around and saw one palce where similar change seems to be necessary.

But there is one place where IndexOf() is immediately compared to "-1".
If I changed this (-1) to NoIndex, I got sign-unsigned comparison warning.
I am not sure what is going on. (I put a comment for someone to figure out in the patch.)

@@ +1218,5 @@
>>      s++;
>>    m_envelope_date.value = s;
>>    m_envelope_date.length = (uint16_t) (line_size - (s - >m_envelope.GetBuffer()));
>> +
>> +  /* bug 814870 */
>
>Remove this comment.

I thought it was a good idea to leave this, but come to think of it, it is not
so useful in this particular case, and so I took it out.

@@ +1404,5 @@
>> +      {
>> +        id->length--, id->value++;
>> +      }
>> +
>> +      if(id->length <= 0)>

>Nit: add a space after "if".

I added a space. I looked around and there were a couple of places where such
extra space was warranted and so inserted a space there.

@@ +1405,5 @@
>> +        id->length--, id->value++;
>> +      }
>> +
>> +      if(id->length <= 0)
>> +        NS_WARNING("id->length failure.: Not positive!");

>Is it OK to continue to the next statement and deference id->value[id->length - 1]
>if id->length <= 0? That seems bad.

Right. I had already fixed this by putting id->length > 0 check in the following
if statement in the local copy. The check is in the patch.

@@ +1655,5 @@
>> +#endif
>> +            NS_WARNING("PR_ParseTimeString failed near L1656.\n");
>> +          }
>> +
>> +

>Remove these two blank lines,

I removed extra blank lines from my patch.

TIA

Comment 45

[ISHIKAWA, Chiaki]

I forgot to add.
(In reply to ISHIKAWA, chiaki from comment #34)
> (In reply to Wan-Teh Chang from comment #26)
> > chiaki: good progress! Can this bug explain the other TB valgrind bug
> > involving PR_ParseTimeString that you filed (bug 806293)?
> 
> Yes.
> Looks like this can explain the issue since I don't particularly see the
> issue in the recent memcheck session log any more.
> Good!
> 

Well, that could have been a premature statement.
In the latest test run of "make mozmill" with these patches, I got
the symptom of a strange buffer filled with marker value passed to
PR_ParseTimeString(). 

The problem is that this DID NOT occur in a previous run.
The symptom is random!

I think there is indeed an issue of uninitialized variable
problem somewhere (but may not be directly related to
bug  806293, and presumably the following error occurs
in a call to PR_ParseTimeSTring() done somewhere else. )

To wit:

TEST-START | /TB-NEW/TB-3HG/new-src/mail/test/mozmill/message-header/test-message-header.js | test_add_contact_from_context_menu
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "Controller.keypress()"}
Step Pass: {"function": "controller.click()"}
Step Pass: {"function": "Controller.keypress()"}
prtime.c: dumping string:
<0xda><0xda><0xda> ... up to 1000 ....<0xda>
 -----
WARNING: PR_ParseTimeStringToExplodedTime scanned more than 1000 chars at line 1634
PR_ParseTimeString: PR_ParseTimeString failed.
prtime.c: dumping string:
<0xda><0xda><0xda> ... up to 1000 ....<0xda>
 -----
WARNING: /build/buildd-glib2.0_2.33.12+really2.32.4-2-i386-TMMpf5/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3397: signal name `selection_changed' is invalid for instance `0xbd0f5b8': 'glib warning', file /TB-NEW/TB-3HG/new-src/mozilla/toolkit/xre/nsSigHandlers.cpp, line 127

	 ...

The problem here is that we have no way of knowing where this
problematic call to PR_ParseTimeString() was made during the batch run "make mozmill". 
(That is why I said I miss the incorporation of backtracing feature
in PR_ParseTimeString() in my previous post. )

I think we have to insert the debug print statement next to the each
and every call of PR_ParseTimeString() and dump the file/line info
when the return value is not zero ( for automatic batch run of "make mozmill".)
At this moment, I don't have the patience of running TB under gdb and
wait for this error condition during my normal usage.

TIA

Comment 46

[ISHIKAWA, Chiaki]

(In reply to Wan-Teh Chang from comment #26)
> chiaki: good progress! Can this bug explain the other TB valgrind bug
> involving PR_ParseTimeString that you filed (bug 806293)?
> 
> Could you open a new NSPR bug report for your PR_ParseTimeString patch?
> Please omit the reformatting changes. (I understand why you wanted to
> reformat the code.) Thank you.

My current observation.
The patch here (Bug 814870) , and the patch in 
Bug 832932 - NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends
have solved the issue in Bug 806293.

I have not seen the strange bug of PR_ParseTimeString any more.
(The only incidence might have been a out-of-sync miscompilation issue
that occurred after my refreshing the source tree to reflect the latest change.)
There is a place where PR_ParseTimeString() fails because it is passed
an empty string ("") during "make mozmill" test run: PR_ParseTimeString() is
called  from a cookie handling routine when it is passed an empty string, but
that situation is foreseen and taken care of in the caller routine, and so
it is OK.

I will mark 806293 as resolved if is OK.

TIA

Comment 47

[David :Bienvenu]

Comment on attachment 706359 [details] [diff] [review]
Improved patch, take 3.

thx for the new patch

don't need braces here:
+      if (id->length > 0 && id->value[0] == '<')
+      {
+        id->length--, id->value++;
+      }
+
can you use NS_WARN_IF_FALSE here?
+      if (id->length <= 0)
+        NS_WARNING("nsParseMailbox.cpp: id->length failure in FinalizeHeaders(). Not positive!");
+
+

we don't do fprintf(stderr) - the warning is sufficient.
+              fprintf(stderr,"Unexpected flag char:<%c>\n", *s);
+              NS_WARNING("Problem in nsParseMailbox.cpp: should not happen.\n");

Comment 48

[ISHIKAWA, Chiaki]

Attachment: Improved patch, take 4.

Modified the patch according to the suggestion.

- omitted the unnecessary pair of braces,
- used NS_WARN_IF_FALSE, and
- removed fprintf(). Yes, I think just getting the warning and
  then dumping the file later on will get us the
  mysterious character. [There are cases where the data that caused
  the rare error conditions may be best captured for later analysis, but
  since the unexpected data is in the file in this case, 
  warning alone would suffice.]

TIA

Comment 49

[David :Bienvenu]

Comment on attachment 711318 [details] [diff] [review]
Improved patch, take 4.

don't think you need \n in warning string:

+            NS_WARNING("PR_ParseTimeString of date failed in FinalizeHeader().\n");


+              NS_ASSERTION(0, "Corrupt file. Should not happen.");
+              break;
should be NS_ERROR("Corrupt file...");

other than that, looks OK. I'm having problems with my build machine, so would it be possible for you to do try server run with your final patch and verify the results before having it checked in?

Comment 50

[ISHIKAWA, Chiaki]

(In reply to David :Bienvenu from comment #49)
> Comment on attachment 711318 [details] [diff] [review]
> Improved patch, take 4.
> 
> don't think you need \n in warning string:
> 
> +            NS_WARNING("PR_ParseTimeString of date failed in
> FinalizeHeader().\n");
> 
> 
> +              NS_ASSERTION(0, "Corrupt file. Should not happen.");
> +              break;
> should be NS_ERROR("Corrupt file...");
> 
> other than that, looks OK. I'm having problems with my build machine, so
> would it be possible for you to do try server run with your final patch and
> verify the results before having it checked in?

I will modify the patch according to the suggestion.

Now this part:

> would it be possible for you to do try server run with your final patch and
> verify the results

How do I do this???

Comment 51

[ISHIKAWA, Chiaki]

(In reply to David :Bienvenu from comment #49)
> Comment on attachment 711318 [details] [diff] [review]
> Improved patch, take 4.
> 
> don't think you need \n in warning string:
> 
> +            NS_WARNING("PR_ParseTimeString of date failed in
> FinalizeHeader().\n");
> 
> 
> +              NS_ASSERTION(0, "Corrupt file. Should not happen.");
> +              break;
> should be NS_ERROR("Corrupt file...");
> 
> other than that, looks OK. I'm having problems with my build machine, so
> would it be possible for you to do try server run with your final patch and
> verify the results before having it checked in?

I will modify the patch according to the suggestion.

Now this part:

> would it be possible for you to do try server run with your final patch and
> verify the results

How do I do this???

I have been checking my local build using "make mozmill" for sometime on my PC, but never did this try server run myself.
Any pointers will be appreciated. I vaguely recall reading about it before.

TIA

Comment 52

[:aceman]

You can read about it here:
https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer

Or visit us at irc.mozilla.org, channel #maildev and we can run it for you.

Comment 53

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #52)
> You can read about it here:
> https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer
> 

I am studying this right now. Too bad, it is not a self-contained doc, but we need to refer to the firefox try server document. (For a newbie, it is very confusing. ).

Anyway, I am attaching the fixed patch and if I don't find the procedure 
easy to understand, I may decide to join the chat channel on Monday (JST), which
is a holiday in Japan.
 
> Or visit us at irc.mozilla.org, channel #maildev and we can run it for you.

Thank you.

Comment 54

[ISHIKAWA, Chiaki]

Attachment: Improved patch, take 5.

Modified the patch to the latest suggestion.

TIA

Comment 55

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, chiaki from comment #53)
> (In reply to :aceman from comment #52)
> > You can read about it here:
> > https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer
> > 
  ...
> > Or visit us at irc.mozilla.org, channel #maildev and we can run it for you.
> 
> Thank you.

Took me a while to figure out the tryserver qualification setup. I have not registered myself at mozilla foundatio yet.
Then I got a bad bout of cold (I live in the northern hemisphere.)
Will try to complete the process this week.
TIA

Comment 56

[ISHIKAWA, Chiaki]

I wonder if someone can vouch for my level-1 access to use tryserver.
http://www.mozilla.org/hacking/commit-access-policy/ says :
Try/User access - 'try' trees and user repositories in Hg, plus any other tree placed at this level. Requirements: one voucher - any other user with level 2 or above access.
Sorry, I have never used IRC before, and am still trying to figure out how to use it. I felt dumb.

Comment 57

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to ISHIKAWA, chiaki from comment #56)
> I wonder if someone can vouch for my level-1 access to use tryserver.
> http://www.mozilla.org/hacking/commit-access-policy/ says :
> Try/User access - 'try' trees and user repositories in Hg, plus any other
> tree placed at this level. Requirements: one voucher - any other user with
> level 2 or above access.
> Sorry, I have never used IRC before, and am still trying to figure out how
> to use it. I felt dumb.

I can. Please cc me on the bug that you file for level 1 access, and I will vouch for you.

Comment 58

[David :Bienvenu]

I'm also happy to vouch for you.

Comment 59

[:aceman]

For the IRC access, you should be able to use TB itself (create a chat account) and the only access info you need is the server (port 6667) and any username you choose. Once you are connected, you need to specify the channel. I think said which one it is.

Comment 60

[ISHIKAWA, Chiaki]

Dear Gary, David and aceman,

Thank you for your help.
I have created Bug Bug 845675 - Commit Access (Level 1) for Chiaki Ishikawa

and so if Gary and David can vouch for me, I will appreciate it.
(Don't know how you do it. Presumably you will post a message there.)

As for IRC, you may not believe that for a really newbie, obtaining the userid 
and understanding the open distributed nature of such registration takes time to sink in. I was looking at the single entry in the left pane of TB when I chose chat and it took me a good part of a night to figure out how to 
register a username (and made a misspell there). Oh well.

Thank you again.

Comment 61

[ISHIKAWA, Chiaki]

(In reply to David :Bienvenu from comment #49)
> Comment on attachment 711318 [details] [diff] [review]
> Improved patch, take 4.
> 
> don't think you need \n in warning string:
> 
> +            NS_WARNING("PR_ParseTimeString of date failed in
> FinalizeHeader().\n");
> 
> 
> +              NS_ASSERTION(0, "Corrupt file. Should not happen.");
> +              break;
> should be NS_ERROR("Corrupt file...");
> 
> other than that, looks OK. I'm having problems with my build machine, so
> would it be possible for you to do try server run with your final patch and
> verify the results before having it checked in?

Hello David,

It took me such a long time to figure out how to submit my patch to TB TryServer, but I finally managed to run the submission with this patch here.
Please see

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=68b4c116df37

(I got hit with known random errors, but otherwise, there were no new errors.
[Which I thought would be the case based on my local run, anyway.]
Obviously I did something wrong with win build. I have not done much with win build on my local machine, and I suspect end of line mismatch or something.) 

The patch posted here is the patch used as
https://hg.mozilla.org/try-comm-central/rev/5eb93e4f864b
after adding git-like header and a message line.

TIA

PS: I am re-posting the git-like patch noted above as the new patch attachment.

Comment 62

[ISHIKAWA, Chiaki]

Attachment: "Improved patch, take 5." with git-like header, etc.

Replacing the previous "take 5" patch by adding only cosmetic change (git-like header block, etc.)

TIA

Comment 63

[David :Bienvenu]

Comment on attachment 725747 [details] [diff] [review]
"Improved patch, take 5." with git-like header, etc.

thx for the patch, and for doing the try server build!

Comment 64

[ISHIKAWA, Chiaki]

Thank you. I took the liberty of entering "checkin-needed" keyword.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/fd0615bc9ee5

Comment 66

[:aceman]

Chiaki, could you place mark the not-checked in patches as obsolete?

Also, while I like this:
- int32_t customHeaderIndex = m_customDBHeaders.IndexOf(headerStr);
- if (customHeaderIndex != -1)
+ uint32_t customHeaderIndex = m_customDBHeaders.IndexOf(headerStr);
+ if (customHeaderIndex != nsTArray<nsCString>::NoIndex)

shouldn't it be m_customDBHeaders.NoIndex? I.e. retrieved directly from the array. What if m_customDBHeaders changes from nsTArray<nsCString> to something else? This will still compile and the changer may not notice to update this line.

Comment 67

[Mark Banner (:standard8)]

I just backed this out due to unit test failures:

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

https://tbpl.mozilla.org/php/getParsedLog.php?id=20852033&tree=Thunderbird-Trunk&full=1#error0

SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_that_msg_without_date_clears_previous_headers
  EXCEPTION: Expected <row> elemnent for Newsgroups header to be collapsed, but it wasn't
!
    at: test-message-header.js line 424
       test_that_msg_without_date_clears_previous_headers test-message-header.js 424
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_more_widget
  EXCEPTION: more node was collapsed when it should have been visible
    at: test-message-header.js line 554
       subtest_more_widget_display test-message-header.js 554
       test_more_widget test-message-header.js 450
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_show_all_header_mode
  EXCEPTION: more node was collapsed when it should have been visible
    at: test-message-header.js line 554
       subtest_more_widget_display test-message-header.js 554
       test_show_all_header_mode test-message-header.js 497
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_more_widget_with_maxlines_of_3
  EXCEPTION: more node was collapsed when it should have been visible
    at: test-message-header.js line 554
       subtest_more_widget_display test-message-header.js 554
       test_more_widget test-message-header.js 450
       test_more_widget_with_maxlines_of_3 test-message-header.js 637
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183

Comment 68

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #66)
> Chiaki, could you place mark the not-checked in patches as obsolete?
> 
> Also, while I like this:
> - int32_t customHeaderIndex = m_customDBHeaders.IndexOf(headerStr);
> - if (customHeaderIndex != -1)
> + uint32_t customHeaderIndex = m_customDBHeaders.IndexOf(headerStr);
> + if (customHeaderIndex != nsTArray<nsCString>::NoIndex)
> 
> shouldn't it be m_customDBHeaders.NoIndex? I.e. retrieved directly from the
> array. What if m_customDBHeaders changes from nsTArray<nsCString> to
> something else? This will still compile and the changer may not notice to
> update this line.

That may make sense. I will investigate the regression and then 
during that time, I will incorporate the suggested change.


In reply to Mark Banner (:standard8) from comment #67)
> I just backed this out due to unit test failures:
> 
> https://hg.mozilla.org/comm-central/rev/5d27a4a4a01e
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=20852033&tree=Thunderbird-
> Trunk&full=1#error0

Sorry, I overlooked this particular error in the log.
(I thought the errors were grouped into chunks with a leading line or something, but
no actually the leading lines were errors without any bugzilla entries are associated
with them. And this error was such a subject-like-looking error.) 
> 
> SUMMARY-UNEXPECTED-FAIL | test-message-header.js |
> test-message-header.js::test_that_msg_without_date_clears_previous_headers
>   EXCEPTION: Expected <row> elemnent for Newsgroups header to be collapsed,
> but it wasn't
> !
> I just backed this out due to unit test failures:

[] SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_that_msg_without_date_clears_previous_headers
  EXCEPTION: Expected <row> elemnent for Newsgroups header to be collapsed, but it wasn't
!

I can understand that patch may affect this test.
The name of the test "test_that_msg_without_date_clears_previous_headers" suggest
a test is being performed against an article without Date. header line.

This is because the patch now correctly handles an article without Date: header
without generating bogus index (-2), and thus not corrupting some unrelated memory area, and then passing bogus pointer to prtime.c 
But in doing so, the patch may change the manner such an article is handled.

ONE QUESTION to people in the know: Does the mail/news article that gets fed into TB
during testing being fed by a real POP3/NNTP server that uses the real calendar time, or
are the articles fed into the Inbox folder (and other folders) off-line for testing with known Date: and/or Received header lines?
[I need to find out exactly which method is used for this particular test since the
date information when the "Date:" header is missing is supposed to be obtained from the "Received:" line attached by the mail server. If I test MANUALLY the behavior of thunderbird using a real mail server by sending "Date:" less e-mail, that timestamp obviously would be different if "make mozmill" uses fixed "Received:" header in injected articles for testing purposes. ]

[] To be honest, I am not sure how the other three tests may be related to the patch at all. 

Searching 
test-message-header.js  test_more_widget 
turned up the following:

  Bug 647211 - Random orange: TEST-UNEXPECTED-FAIL | test-message-header.js | test_show_all_header_mode
(seems to have been solved, WORKSFORME)

Searching 
test-message-header.js test_show_all_header_mode 
turned up the following as well as the previous bug entry.

Bug 581922 - Random orange: TEST-UNEXPECTED-FAIL | test-message-header.js | test_show_all_header_mode 
(seems to have bee FIXED?)

Searching
test-message-header.js test_more_widget_with_maxlines_of_3
turned up the following as well as the preceding two bug entries.

Bug 565209 - Preference to show n lines before "more" is shown doesn't display multiple lines if needed
(seems to have been FIXED?)

I will investigate these three if I find relevant correlation factors.

TIA

Comment 69

[Mark Banner (:standard8)]

(In reply to ISHIKAWA, chiaki from comment #68)
> > SUMMARY-UNEXPECTED-FAIL | test-message-header.js |
> > test-message-header.js::test_that_msg_without_date_clears_previous_headers
> >   EXCEPTION: Expected <row> elemnent for Newsgroups header to be collapsed,
> > but it wasn't
> > !
> > I just backed this out due to unit test failures:
> 
> [] SUMMARY-UNEXPECTED-FAIL | test-message-header.js |
> test-message-header.js::test_that_msg_without_date_clears_previous_headers
>   EXCEPTION: Expected <row> elemnent for Newsgroups header to be collapsed,
> but it wasn't
> !
> 
> I can understand that patch may affect this test.
> The name of the test "test_that_msg_without_date_clears_previous_headers"
> suggest
> a test is being performed against an article without Date. header line.
> 
> This is because the patch now correctly handles an article without Date:
> header
> without generating bogus index (-2), and thus not corrupting some unrelated
> memory area, and then passing bogus pointer to prtime.c 
> But in doing so, the patch may change the manner such an article is handled.
> 
> ONE QUESTION to people in the know: Does the mail/news article that gets fed
> into TB
> during testing being fed by a real POP3/NNTP server that uses the real
> calendar time, or
> are the articles fed into the Inbox folder (and other folders) off-line for
> testing with known Date: and/or Received header lines?

You can see this in the test itself:

http://hg.mozilla.org/comm-central/annotate/8a50d60dd7a0/mail/test/mozmill/message-header/test-message-header.js#l399

the create_message() is manually creating a message and inserting it into a folder within Local Folders.

To expand on the failure mode: a "Newsgroup:" line is displayed in the header for a standard email message when the date header isn't present - obviously this shouldn't happen.

> [] To be honest, I am not sure how the other three tests may be related to
> the patch at all. 

The other three tests are all in the same file. There's two possible causes a) because the first test failed, it lead onto the other tests failing; b) there were other failures on the tree at the time.

I'd say lets concentrate on the first failure, and then run this through try server and we can revisit the other three failures if necessary.

Comment 70

[ISHIKAWA, Chiaki]

(In reply to Mark Banner (:standard8) from comment #69)

> I'd say lets concentrate on the first failure, and then run this through try
> server and we can revisit the other three failures if necessary.

Thank you for the pointers. I will investigate the javascript and see what I can do.

> To expand on the failure mode: a "Newsgroup:" line is displayed in the 
> header for a standard email message when the date header isn't 
> present - obviously this shouldn't happen.

This sound very problematic. I will see if I can create this on my local test.
(I seem to be able to inject any headers [even broken ones) using a program called "exim" mail program (similar to sendmail, but Debian GNU/Linux seems to prefer "exim" over "sendmail" in default configuration.)

TIA

Comment 71

[ISHIKAWA, Chiaki]

> (In reply to Mark Banner (:standard8) from comment #69)

> 
> > To expand on the failure mode: a "Newsgroup:" line is displayed in the 
> > header for a standard email message when the date header isn't 
> > present - obviously this shouldn't happen.
> 

Manual checking did not produce errors.:
I tried to re-create the condition manually by inserting an article without Date: header and with/without Newsgroups: header line.

In my manual tests of display such article, the problem did not manifest itself.
The header pane for the article shrunk as expected.

But test in "make mozill" failed. It could be that, 

possibility 1: as noted in the bugzilla entries, that the repaint may take a 
  while, and we need to insert a more longer sleep or something before checking
  the final status of display (it seems it is already done?), or

possibility 2: it seems that the test in http://hg.mozilla.org/comm-central/annotate/8a50d60dd7a0/mail/test/mozmill/message-header/test-message-header.js#l399
assumes that the newly inserted article without Date: header and Newsgroup: header
will be in the first position (in a folder sorted in ascending order of Date:).
[It assumes that the date put in the Received: line for the newly created and inserted
article puts the article to the beginning in the Date: sorted order.]

It then selects the article in the first position, and assumes that this is the
newly injected article, and looks at the display for the collapsing of Newsgroups row.

But this assumption may no longer be true after the fix for invalid index/invalid memory reference issue for Date-less article in the patch presented here.
(I suspect a hither-to unexpected behavior in extracting time info from Received: header line.)
I will check this by trying to see if the newly injected article is indeed the one selected by the test routine. (If someone can suggest the easist way to do this, I will welcome it. I suspect if I can obtain the message ID for two articles, the one that was created and injected, and that one at the beginning that is selected, I will be done.)

TIA

Comment 72

[ISHIKAWA, Chiaki]

Attachment: Flawed test result: the article is not located at where the test assumed.

Just as I suspected, the possibility-2 is the case.
The test is now flawed.

The newly injected article without "Date" header is no longer shown at
the beginning of date-sorted folder because of the fix.  Instead, it
is shown at the end of the list (with today's timestamp).

I am attaching the capture of the screen when the problem appears. I
inserted the subject "This is without date" for better recognition."
to the article.
See the newly injected article is not at the top, and a different article at the top is selected. So the test failed.

So the test itself is flawed.

Why did this happen?

Firstly, what was/is the perceived timestamp for the
injected article without Date: header line.

In "mailnews/local/src/nsParseMailbox.cpp" which
the proposed patch fixes, we have the following code.


nsresult nsParseMailMessageState::FinalizeHeaders()
{
  nsresult rv;
  struct message_header *sender;

  ...
  sender     = (m_from.length          ? &m_from :
  ...
  date       = (m_date.length       ? &m_date :
  m_envelope_date.length ? &m_envelope_date :
  0);
  deliveryDate = (m_delivery_date.length ? &m_delivery_date : 0);

Note, previously before the patch, when there was no Date, the code was buggy and
|m_envelope_date.length| was set to (-2).
So as a result, |m_evenlope_date| which contained garbage is set to |date|.

Then the timestamp was obtained using PR_ParseTimeString() from this
garbage in the heap.

Today with the proposed patch, |date| is properly set to 0 if there is no
Date: header, and |m_envelope_date.length| is 0.
(And presumably |m_delivery_date.length| is also 0. The
create_message() function is skipping these headers since, I suppose,
NNTP, IMAP, and POP3 may need to attach different headers. Less
complication by skipping the details.)

The following code obtains the timestamp from the memory if |date| is non-null.

        uint32_t rcvTimeSecs = 0;
        if (date)
        {  // Date:
          PRTime resultTime;
          PRStatus timeStatus = PR_ParseTimeString (date->value, false, &resultTime);
	  [...]
        }
	else
	{ // PR_NOW()
	 [...]
          PRTime resultTime = PR_Now();
          m_newMsgHdr->SetDate(resultTime);
         }

But if date is 0, the timestamp for the article is internally set to
PR_NOW(), i.e., today's date. This is what happens today, I think.

Now, previously before the proper fix, what PR_ParseTimeString()
picked up from the random junk in the heap is anyone's guess.

But I think since PR_ParseTimeString() fails unless 
date, month, and year are properly set at the end of scanning, and
for this to happen, PR_ParseTimeString() must see one of the following

 dd/mm/yy
 dd-mm-yy
 dd/mm/yyy
 dd/mm/yyyy

or some such similarly properly formatted date string.

If PR_ParseTimeString() is passed random heap data (during "make
mozmill") it is likely to fail and thus rcvTimeSecs remains 0.

(The failure of PR_ParseTimeString() was not reported in the original
code.  Now my patch for the fix at least inserts the following
for detecting this fishy condition.)

          else
            NS_WARNING("PR_ParseTimeString of date failed in FinalizeHeader().");

rcvTimeSecs being 0, I think, means that the article is deemed at the
calendar epoch used by mozilla's internal code.  (Presumably it is
UNIX-epoch or MS-WINDOWS epoch?)  

In any case, both are older than the year 2000, the year with which
other articles are timestamped by create_message() during "make
mozmill".  

Thus with the buggy original code, the newly injected "Date"-less
article was shown at the beginning of the date-sorted folder since it
was deemed older than any other article.

QED :-)

So now it is time to re-write the test.
What will that be?

TIA

Comment 73

[ISHIKAWA, Chiaki]

Attachment: The screen capture that shows the positions of the tests that failed.

The subsequent three tests used articles that were injected, but
since the "Date:"-less article remains at the end of the display list always,
trying to use the last article as the test target is not correct.
So they failed. 
Since the position is known in advance as 5, 6 and 7 (0-th origin) as the
articles are sequentially injected, I decided to 
let each failing test explicitly specify the correct article position.

Then all the tests from test-message-header.js passed.

The patch to the test follows.

TIA

Comment 74

[ISHIKAWA, Chiaki]

Attachment: Fixing the failed tests by taking care of the "Last" position of "Date:"-less article.

Here is the modified test to consider now the "LAST" position of the "Date:"-less article in the displayed list of articles.

This passed local "make mozmill" tests a couple of times now.

Comment 75

[ISHIKAWA, Chiaki]

I think I need to explain how I reached the patch in comment 75.

By choosing the last article using select_click_row(-1) instead of
select_click_row(0) as follows,

  // select and open the LAST message
  let curMessage = select_click_row(-1); // not the first anymore.

I could get the particular test,
test_that_msg_without_date_clears_previous_headers,
pass.

So this indeed added to the confidence that my analysis is correct.

However, then the following tests still failed.


SUMMARY-PASS | test-message-header.js::test_that_msg_without_date_clears_previous_headers
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_more_widget
  EXCEPTION: more node was collapsed when it should have been visible
    at: test-message-header.js line 561
       subtest_more_widget_display test-message-header.js 561
       test_more_widget test-message-header.js 457
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_show_all_header_mode
  EXCEPTION: more node was collapsed when it should have been visible
    at: test-message-header.js line 561
       subtest_more_widget_display test-message-header.js 561
       test_show_all_header_mode test-message-header.js 504
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_more_widget_with_maxlines_of_3
  EXCEPTION: more node was collapsed when it should have been visible
    at: test-message-header.js line 561
       subtest_more_widget_display test-message-header.js 561
       test_more_widget test-message-header.js 457
       test_more_widget_with_maxlines_of_3 test-message-header.js 644
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
       Bridge.prototype.execFunction server.js 183
SUMMARY-PASS | test-message-header.js::test_more_widget_with_disabled_more
SUMMARY-PASS | test-message-header.js::test_toolbar_collapse_and_expand


Eventually, I figured that
this is because the following failing tests assumed that their
new injected articles would be displayed at the END of the displayed list
of articles (by the order of injection, which had automatically increased
the timestamp with the  year 2000 ), but the "Date:"-less article sits
at the END because of its brand-new timestamp (This year is 2013).

test_more_widget chooses its own injected article with
  // select and open the last message
  let curMessage = select_click_row(-1);

test_show_all_header_mode also chooses its own injected article with
  // select and open the last message
  let curMessage = select_click_row(-1);

test_more_widget_with_maxlines_of_3
calls test_more_widget internally (which failed as noted above).

I think I could either delete the "Date:"-less article or correctly
specify the article which would be shown at the second last position.

But it was not obvious how to remove the article, nor specify "the second to last" position easily.

So eventually I monitored the position of the injected article for
testing purposes caefully, and select the article by specifying the
correct row (the absolute value against 0-th origin) to the
iselect_click_row().

That is how the patch was produced.

Comment 76

[ISHIKAWA, Chiaki]

TB TryServer run with the proposed patches (it also has some other patches for
folder size going over 4GB issues)

The observed test failures were no longer there :
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=2cf5d49ebf2b

Comment 77

[Mark Banner (:standard8)]

Comment on attachment 728623 [details] [diff] [review]
Fixing the failed tests by taking care of the "Last" position of "Date:"-less article.

Review of attachment 728623 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this generally seems good and fixes it on my machine as well, but there's a few review comments to sort out.

::: mail/test/mozmill/message-header/test-message-header.js
@@ +409,5 @@
>  
>    // this will add the message to the end of the folder
>    add_message_to_folder(folder, msg);
>  
> +  // see bug 814870: not the first anymore. The timestamp is that of "NOW".

Please drop the reference to the bug, we don't need historical comments in here (that's what blame & history is for).

@@ +411,5 @@
>    add_message_to_folder(folder, msg);
>  
> +  // see bug 814870: not the first anymore. The timestamp is that of "NOW".
> +  // select and open the LAST message
> +  let curMessage = select_click_row(-1); 

nit: unnecessary blank space at end of line (several places throughout the file).

@@ +442,5 @@
>    add_message_to_folder(folder, msg);
>  
> +  //  See bug 814870: 
> +  // let us see if this is called from outside.
> +  // the first call ought to take a look at 5th article (0-th origin).

Again, please drop the historical reference. A simple explanation of why it is not at the end would be fine. I think we can also use -2 rather than 5. As this will be the second from the bottom of the list.

I think also, I'd prefer not to have the called_once global variable, and default to -2 if the argument isn't supplied.

Please also change the argument name to something like "aSelectRow".

@@ +456,5 @@
>    // make sure it loads
>    wait_for_message_display_completion(mc);
>    assert_selected_and_displayed(mc, curMessage);
>  
> +  mc.sleep(2000);

I assume this is debug-only, hence please remove it.

@@ +499,5 @@
>    // add the message to the end of the folder
>    add_message_to_folder(folder, msg);
>  
> +  // select and open the added message.
> +  // See bug 814870: Not the end any more, but 6th. 

Lets go for -2 again if that's right, and drop the historical reference.

@@ +502,5 @@
> +  // select and open the added message.
> +  // See bug 814870: Not the end any more, but 6th. 
> +  let curMessage = select_click_row(6);
> +
> +  mc.sleep(2000 * 5);

Again, I guess this is debug that we don't want.

@@ +652,5 @@
>    let maxLines = prefBranch.setIntPref(
>      "mailnews.headers.show_n_lines_before_more", 3);
>  
>    // call test_more_widget again
> +  // See bug 814870: we need to look at the 7th article. (0-th origin)

Ditto with dropping the historical reference and possibly using a minus index.

Comment 78

[ISHIKAWA, Chiaki]

(In reply to Mark Banner (:standard8) from comment #77)
> Comment on attachment 728623 [details] [diff] [review]
> Fixing the failed tests by taking care of the "Last" position of
> "Date:"-less article.
> 
> Review of attachment 728623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, this generally seems good and fixes it on my machine as well, but
> there's a few review comments to sort out.

Thank you for the initial review.
I am uploading the modified patch.

> 
> ::: mail/test/mozmill/message-header/test-message-header.js
> @@ +409,5 @@
> >  
> >    // this will add the message to the end of the folder
> >    add_message_to_folder(folder, msg);
> >  
> > +  // see bug 814870: not the first anymore. The timestamp is that of "NOW".
> 
> Please drop the reference to the bug, we don't need historical comments in
> here (that's what blame & history is for).

I dropped the historical reference (bug 814870).

> 
> @@ +411,5 @@
> >    add_message_to_folder(folder, msg);
> >  
> > +  // see bug 814870: not the first anymore. The timestamp is that of "NOW".
> > +  // select and open the LAST message
> > +  let curMessage = select_click_row(-1); 
> 
> nit: unnecessary blank space at end of line (several places throughout the
> file).

I removed the trailing blanks using emacs's delete-trailing-whitespace.

> 
> @@ +442,5 @@
> >    add_message_to_folder(folder, msg);
> >  
> > +  //  See bug 814870: 
> > +  // let us see if this is called from outside.
> > +  // the first call ought to take a look at 5th article (0-th origin).
> 
> Again, please drop the historical reference. A simple explanation of why it
> is not at the end would be fine. I think we can also use -2 rather than 5.
> As this will be the second from the bottom of the list.

The use of (-1) is it. I used it exclusively instead of
relying on counting the row position.

> I think also, I'd prefer not to have the called_once global variable, and
> default to -2 if the argument isn't supplied.
> 
> Please also change the argument name to something like "aSelectRow".

Using (-2) unnecessitated the use of callonece, and the added argument.

> 
> @@ +456,5 @@
> >    // make sure it loads
> >    wait_for_message_display_completion(mc);
> >    assert_selected_and_displayed(mc, curMessage);
> >  
> > +  mc.sleep(2000);
> 
> I assume this is debug-only, hence please remove it.

Extra sleeps() removed.

 [...]
The rests are similarly taken care of.

TIA

Comment 79

[ISHIKAWA, Chiaki]

Attachment: Fixing the failed tests by taking care of the "Last" position of "Date:"-less article. (Take 2)

The improved patch.

Comment 80

[Mark Banner (:standard8)]

Comment on attachment 729156 [details] [diff] [review]
Fixing the failed tests by taking care of the "Last" position of "Date:"-less article. (Take 2)

Review of attachment 729156 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update, looks great.

I'll re-land the main patch, and land this with it in a hour or two.

Comment 81

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/b4549ca76992
https://hg.mozilla.org/comm-central/rev/669171e27d6a

Comment 82

[ISHIKAWA, Chiaki]

(In reply to Mark Banner (:standard8) from comment #81)
Thank you!

Comment 83

[Mark Banner (:standard8)]

After consideration (sec-low, potential of changes to order of some emails in UI), not going to take this forward onto the mainstream/esr branch, but we'll let it roll out over the next few weeks and let it go through the cycles.

Comment 84

[ISHIKAWA, Chiaki]

(In reply to Mark Banner (:standard8) from comment #83)
> After consideration (sec-low, potential of changes to order of some emails
> in UI), not going to take this forward onto the mainstream/esr branch, but
> we'll let it roll out over the next few weeks and let it go through the
> cycles.

That is fine. Please take the time necessary for dusts to settle down.

Actually, the serious security bug is the friend of this bugzilla entry,
i.e., 
https://bugzilla.mozilla.org/show_bug.cgi?id=832932
Bug 832932 - NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

Without the patch there, the code happily runs beyond the allocated memory block
until it sees '\0'. That was when valgrind/memcheck spotted the issue.

I have not been able to figure out exactly how to run win32 bit compilation on TB
TryServer, but linux32, and linux64 compilation and testing have worked OK.
So  I think it is about time,
that patch in bug 832932 ought to land. I will try one more time for win32 build
later today or tomorrow.