809064 - Uninitialized value usage in ./mailnews/base/src/nsMsgDBView.cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[ISHIKAWA, Chiaki]

Uninitialized value usage in ./mailnews/base/src/nsMsgDBView.cpp

Found by valgrind run. See bug 803816 about mozmill run of TB.

Source version:
comm-central thunderbird.
$ hg identify
1016cef82fd8+ tip
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src$ cd mozilla
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src/mozilla$ hg identify
a517f7ea5bef+ tip

Valgrind log

==30518== Conditional jump or move depends on uninitialised value(s)
==30518==    at 0x5B8F829: nsMsgDBView::GetInsertIndexHelper(nsIMsgDBHdr*, nsTArray<unsigned int, nsTArrayDefaultAllocator>&, nsCOMArray<nsIMsgFolder>*, int, int) (nsMsgDBView.cpp:5227)
==30518==    by 0x5B908A1: nsMsgDBView::GetInsertIndex(nsIMsgDBHdr*) (nsMsgDBView.cpp:5301)
==30518==    by 0x5B9D103: nsMsgSearchDBView::InsertHdrFromFolder(nsIMsgDBHdr*, nsIMsgFolder*) (nsMsgSearchDBView.cpp:655)
==30518==    by 0x5B9DDCD: nsMsgSearchDBView::OnSearchHit(nsIMsgDBHdr*, nsIMsgFolder*) (nsMsgSearchDBView.cpp:691)
==30518==    by 0x5BCC450: nsMsgSearchSession::AddSearchHit(nsIMsgDBHdr*, nsIMsgFolder*) (nsMsgSearchSession.cpp:549)
==30518==    by 0x5BC5EBC: nsMsgSearchOfflineMail::AddResultElement(nsIMsgDBHdr*) (nsMsgLocalSearch.cpp:788)
==30518==    by 0x5BC7D24: nsMsgSearchOfflineMail::Search(bool*) (nsMsgLocalSearch.cpp:741)
==30518==    by 0x5BC49A1: nsMsgSearchScopeTerm::TimeSlice(bool*) (nsMsgSearchTerm.cpp:1881)
==30518==    by 0x5766379: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3132)
==30518==    by 0x576DC97: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==30518==    by 0x66D2880: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==30518==    by 0x66C4414: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2369)
==30518==    by 0x66D18AC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==30518==    by 0x66D296F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==30518==    by 0x66D3037: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==30518==    by 0x6712C1A: js::IndirectProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:456)
==30518==    by 0x67820EB: js::DirectWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:390)
==30518==    by 0x6784837: js::CrossCompartmentWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:722)
==30518==    by 0x67184C3: js::Proxy::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:2478)
==30518==    by 0x671851A: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3038)
==30518==    by 0x66D2AB8: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==30518==    by 0x66C4414: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2369)
==30518==    by 0x66D18AC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==30518==    by 0x66D296F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==30518==    by 0x66D3037: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==30518==    by 0x6712C1A: js::IndirectProxyHandler::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:456)
==30518==    by 0x67820EB: js::DirectWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:390)
==30518==    by 0x6784837: js::CrossCompartmentWrapper::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jswrapper.cpp:722)
==30518==    by 0x67184C3: js::Proxy::call(JSContext*, JSObject*, unsigned int, JS::Value*) (jsproxy.cpp:2478)
==30518==    by 0x671851A: proxy_Call(JSContext*, unsigned int, JS::Value*) (jsproxy.cpp:3038)
==30518==    by 0x66D2AB8: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==30518==    by 0x66C4414: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2369)
==30518==    by 0x66D18AC: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:324)
==30518==    by 0x66D296F: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:379)
==30518==    by 0x66D3037: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==30518==    by 0x6649E05: JS_CallFunctionValue (jsapi.cpp:5901)
==30518==    by 0x538E6B2: nsJSContext::CallEventHandler(nsISupports*, JSObject*, JSObject*, nsIArray*, nsIVariant**) (nsJSEnvironment.cpp:1914)
==30518==    by 0x542F123: nsJSEventListener::HandleEvent(nsIDOMEvent*) (nsJSEventListener.cpp:212)
==30518==    by 0x51DC9B6: nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) (nsEventListenerManager.cpp:868)
==30518==    by 0x51DD09A: nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) (nsEventListenerManager.cpp:941)
==30518==    by 0x51FF9A0: nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) (nsEventListenerManager.h:144)
==30518==    by 0x137161F7: ???
==30518==  Uninitialised value was created by a stack allocation
==30518==    at 0x5B8F520: nsMsgDBView::GetInsertIndexHelper(nsIMsgDBHdr*, nsTArray<unsigned int, nsTArrayDefaultAllocator>&, nsCOMArray<nsIMsgFolder>*, int, int) (nsMsgDBView.cpp:5198)
==30518==



Observation:

Local variable fieldType is not initialized, but is supposed to be set
by GetFieldTypeAndLenForSort();

However, checking GetFieldTypeAndLenForSort() in the same file reveals
that there is an error path in which the third argument is not set and
the function returns NS_ERROR_UNEXPECTED.

In the code below quoted from nsMsgDBView.cpp, the return value is not
checked at all, and thus the third argument fieldType is undefined,
but unnoticed. So the uninitialized fieldType is referenced in the
switch() statement that follows.

Clearly something is wrong here.
  

 5195	nsMsgViewIndex nsMsgDBView::GetInsertIndexHelper(nsIMsgDBHdr *msgHdr, nsTArray<nsMsgKey> &keys,
 5196	                                                 nsCOMArray<nsIMsgFolder> *folders,
 5197	                                                 nsMsgViewSortOrderValue sortOrder, nsMsgViewSortTypeValue sortType)
 5198	{
 5199	  nsMsgViewIndex highIndex = keys.Length();
 5200	  nsMsgViewIndex lowIndex = 0;
 5201	  IdKeyPtr EntryInfo1, EntryInfo2;
 5202	  EntryInfo1.key = nullptr;
 5203	  EntryInfo2.key = nullptr;
 5204	
 5205	  nsresult rv;
 5206	  uint16_t  maxLen;
 5207	  eFieldType fieldType;
 5208	  rv = GetFieldTypeAndLenForSort(sortType, &maxLen, &fieldType);
 5209	  const void *pValue1 = &EntryInfo1, *pValue2 = &EntryInfo2;
 5210	
 5211	  int (* comparisonFun) (const void *pItem1, const void *pItem2, void *privateData) = nullptr;
 5212	  int retStatus = 0;
 5213	  msgHdr->GetMessageKey(&EntryInfo1.id);
 5214	  msgHdr->GetFolder(&EntryInfo1.folder);
 5215	  EntryInfo1.folder->Release();
 5216	  //check if a custom column handler exists. If it does then grab it and pass it in
 5217	  //to either GetCollationKey or GetLongField
 5218	  nsIMsgCustomColumnHandler* colHandler = GetCurColumnHandlerFromDBInfo();
 5219	
 5220	  viewSortInfo comparisonContext;
 5221	  comparisonContext.view = this;
 5222	  comparisonContext.isSecondarySort = false;
 5223	  comparisonContext.ascendingSort = (sortOrder == nsMsgViewSortOrder::ascending);
 5224	  rv = EntryInfo1.folder->GetMsgDatabase(&comparisonContext.db);
 5225	  NS_ENSURE_SUCCESS(rv, highIndex);
 5226	  comparisonContext.db->Release();
 5227	  switch (fieldType)
 5228	  {


The full log is in bug 803816.

(There could be a security implication since use of uninitialized variable led to many exploits in other software packages before. So I am setting Security status just in case. Please reset it is fixed or is not quite security-related.)

Comment 1

[:aceman]

Strange, I've done a sweep on unchecked rv, I wonder why gcc didn't report this one (line 5208).

I think Ischikawa can fix this sort of stuff, this just needs a decision if it is OK to bailout on failure (NS_ENSURE_SUCCESS(rv)) in this function.

Comment 2

[ISHIKAWA, Chiaki]

Thank you for the comment.
I have no idea if it is OK or not (since NS_ERROR_UNEXPECTED sounds a little
difficult to deal with easily), but
several lines below, I see the following bailout for a different 
error:
line 5258:
NS_ASSERTION(NS_SUCCEEDED(rv),"failed to create collation key");

I will adopt this as 
NS_ASSERTION(NS_SUCCEEDED(rv), "failed to obtain field type");
just below the line where rv is set and see how it goes.

TIA

Comment 3

[:aceman]

Actually I think line 5225 (NS_ENSURE_SUCCESS(rv, highIndex);) would be the pattern to copy here. But you'd have to see what returning "highIndex" means for the callers.
I think you can't report any nsresult values like (NS_ERROR_UNEXPECTED) as the function is defined to return nsMsgViewIndex .

Comment 4

[Andrew McCreight [:mccr8]]

It sounds like you are working on this, Ishikawa, so I'll assign it to you.

That sounds kind of sketchy, so I'm going to set sec-high. Feel free to adjust it if you think it should be changed.
https://wiki.mozilla.org/Security_Severity_Ratings

Comment 5

[ISHIKAWA, Chiaki]

Attachment: A proposed patch to take care of the undefined value usage issue.

I created a patch to take care of the particular problematic place, but then
I realized that similar usage without checking is found in a few places in the same file.

Unfortunately, the processing that follows the call to the function, GetFieldTypeAndLenForSort()  is not quite uniform.

To begin with switch() is used to select the processing in some places where as series of if() is used to select the processing (and forgets about the undefined case!).

I tried to return the reasonable value immediately if that is possible.

But in one place, it is not entirely clear what we should do.
So I bomb out.

But, I can certainly say that the change does not break the ordinary cases, and
this patch eliminated the undefined value usage warning from valgrind log.

The case where GetFieldTypeAndLenForSort() returns error value must be
really an error situation that requires an early bail out.

I think someone familiar with the code should check for the bomb-out case,.

All I can say that the patched code is much safer than the current code is, and I am more comfortable to run a safer code than some code that runs with uninitialized variable (which can mean it is non-deterministic from the ordinary 
user's point of view.)

TIA

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 679645 [details] [diff] [review]
A proposed patch to take care of the undefined value usage issue.

Sorry, I'm not actually familiar with Thunderbird code. :)

Comment 7

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 679645 [details] [diff] [review]
A proposed patch to take care of the undefined value usage issue.

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

Thanks for taking this on.

I don't think we need to add the kUnknown alternative to the enum; by my reading, it works to just take the early exit any time a call to GetFieldTypeAndLenForSort() fails.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3833,5 @@
> +//
> +// TODO/FIXME:
> +// There is a case when pFieldType is not set, and the function
> +// returns NS_ERROR_UNEXPECTED;
> +//

Keep the body of the comment, but remove the TODO/FIXME.

@@ +4291,1 @@
>    rv = GetFieldTypeAndLenForSort(sortType, &maxLen, &fieldType);

NS_ENSURE_SUCCESS(rv, 0); and update the comment

@@ +4469,5 @@
>      // could be a problem here if the ones that appear here are different than the ones already in the array
>      uint32_t actualFieldLen = 0;
> +
> +    // TODO/FIXME 
> +    // fieldType 

We exited early (by NS_ENSURE_SUCCESS) if fieldType was not set, so I don't think we need this comment

@@ +5794,3 @@
>    rv = GetFieldTypeAndLenForSort(m_sortType, &maxLen, &fieldType);
> +  // if we fail to obtain fieldType here, the default in switch
> +  // below should catch it. (It will result in error.)

This is debug code, so just fail by assertion like you do for ValidateSort(), and remove the TODO/FIXME line from the comment

@@ +5835,5 @@
> +  // TODO/FIXME
> +  // It is not entirely clear what we should do since
> +  // if fieldType is not available, there is no way to know
> +  // how to compare the field to check for sorting.
> +  // So we bomb out for now. 

Trailing white space. Since this is debug code, excluded by #ifdef, failing by ASSERTION is fine.

@@ +5836,5 @@
> +  // It is not entirely clear what we should do since
> +  // if fieldType is not available, there is no way to know
> +  // how to compare the field to check for sorting.
> +  // So we bomb out for now. 
> +  rv = GetFieldTypeAndLenForSort(m_sortType, &maxLen, &fieldType);

Need to declare nsresult rv?

@@ +5837,5 @@
> +  // if fieldType is not available, there is no way to know
> +  // how to compare the field to check for sorting.
> +  // So we bomb out for now. 
> +  rv = GetFieldTypeAndLenForSort(m_sortType, &maxLen, &fieldType);
> +  NS_ASSERTION(NS_SUCCEEDED(rv),"failed to obtain fieldType");      

Trailing white space

::: mailnews/base/src/nsMsgDBView.h
@@ +39,5 @@
>  
>  enum eFieldType {
>      kCollationKey,
> +    kU32,
> +    kUnknown                    // error case

Don't need to add this

Comment 8

[:Irving Reid (No longer working on Firefox)]

One more thing - it would be nice to have an NS_WARNING that tells us what case caused us to return NS_ERROR_UNEXPECTED, so we have a chance of catching latent bugs where people add values to the enum without adding cases to the switch.

Apparently bugzilla let me stomp on mccr8's re-assignment of the reviewer; sorry about that. Feel free to assign review to either me or :standard8 when you respond to my feedback.

Comment 9

[ISHIKAWA, Chiaki]

(In reply to Irving Reid (:irving) from comment #8)
> One more thing - it would be nice to have an NS_WARNING that tells us what
> case caused us to return NS_ERROR_UNEXPECTED, so we have a chance of
> catching latent bugs where people add values to the enum without adding
> cases to the switch.

OK, I will upload a modified patch tomorrow.
I am afraid that my diff got a little mixed up about the undeclared rv. I thought I recreated a new diff after a successful compilation, 
but seems to obtain a copy from a stale file and used it :-(

Actualy, I am not that familiar with NS_WARNING, but I take that
I can put this statement next to where we bomb out (or
where we return by failure in GetFieldTypeAndLenForSort()?)

> Apparently bugzilla let me stomp on mccr8's re-assignment of the reviewer;
> sorry about that. Feel free to assign review to either me or :standard8 when
> you respond to my feedback.
Will do.

Comment 10

[:Irving Reid (No longer working on Firefox)]

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1728 has an example of building up a message and passing it to NS_ERROR(); NS_WARNING() works the same but doesn't trigger a breakpoint for people debugging the code. I'd put it right before the "return NS_ERROR_UNEXPECTED" in GetFieldTypeAndLenForSort().

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Irving Reid (:irving) from comment #8)
> Apparently bugzilla let me stomp on mccr8's re-assignment of the reviewer;
> sorry about that.
It is no problem, I just wanted somebody who knew this code to look at the patch, and it looks like that has happened. :)

Comment 12

[ISHIKAWA, Chiaki]

Attachment: rev 02 of proposed patch

Tried to incoporate the comment on the first patch.

A note: when I tried to insert NS_WARNING before return NS_ERROR_UNEXPECTED, I realized a grave error/problem or oversight in the original code. (Maybe it is a never-happens case.)
When colHandler is null and *pFieldType is NOT set, the function
GetFieldTypeAndLenForSort() returns NS_OK (!)
So I put another NS_WARNING there.

Header file was restored to the original.

Now, regarding trailing whitespace at the end of a line. I use emacs, and it has "delete-trailing-space" function to remove such extra whitespace characters.

I ran delete-trailing-space. Voila, it removed several other unnoticed trailing whitespaces, it seems. So you see seemingly unnecessary changes. They are because the removal of trailing whitespaces.

One other thing. I let the test run of make mozmill run overnight.
Unfortunately, this happens sometimes, but depending on the load on my PC,
the operation under valgrind takes a looong time, and
the timeout function in the testing harness kicks in.
This happened last time, and I am afraid that the particular test which
caused the printing of the original error got aborted early and
so the printing from NS_WARNING is not found in the latest log.

I left another test run running, and hopefully (depending on the phase of the Moon, etc.), the particular test would run to completion and the warning is printed there.

Comment 13

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 679934 [details] [diff] [review]
rev 02 of proposed patch

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

r=irving after you add the error return I suggest. Thank you for doing this valgrind testing, it is difficult but very helpful.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3895,5 @@
>                *pMaxLen = 0;
>              }
>            }
> +          else
> +            NS_WARNING("colHandler is null. *pFieldType is not set.");

We should return NS_ERROR_NULL_POINTER here

Comment 14

[ISHIKAWA, Chiaki]

(In reply to Irving Reid (:irving) from comment #13)
> r=irving after you add the error return I suggest. Thank you for doing this
> valgrind testing, it is difficult but very helpful.
 
You are welcome.

> ::: mailnews/base/src/nsMsgDBView.cpp
> @@ +3895,5 @@
> >                *pMaxLen = 0;
> >              }
> >            }
> > +          else
> > +            NS_WARNING("colHandler is null. *pFieldType is not set.");
> 
> We should return NS_ERROR_NULL_POINTER here

I re-created the patch and am uploading it in the next post.
(If the patch doesn't apply cleanly, please let me know. By my manual operation mistake, I may have currupted my source tree, but I don't have the time to clean everything and download the whole source tree and start over...(

Comment 15

[ISHIKAWA, Chiaki]

Attachment: rev 03 of proposed patch

>>            }
>> +          else
>> +            NS_WARNING("colHandler is null. *pFieldType is not set.");
>
>We should return NS_ERROR_NULL_POINTER here

Addressed the above issue.

TIA

Comment 16

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 683973 [details] [diff] [review]
rev 03 of proposed patch

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

Looks good, compiles clean. You can carry forward r=irving from this comment when you fix the white space, no need to request another review.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3833,5 @@
> +//
> +// There are cases when pFieldType is not set:
> +// one case returns NS_ERROR_UNEXPECTED;
> +// the other case now return NS_ERROR_NULL_POINTER (this is only when
> +// colhandler below is null, but is very unlikely). 

Trailing white space

Comment 17

[ISHIKAWA, Chiaki]

Attachment: rev 04 of proposed patch

The final patch taking care of the trailing blank issue.
(I am not sure what to do now. Mozilla web page suggests that I add commit-needed keyword somewhere, but uploading page does not allow that operation?)

Comment 18

[Andrew McCreight [:mccr8]]

The keyword is checkin-needed. Remove it if you aren't yet ready for checkin. :)

Comment 19

[ISHIKAWA, Chiaki]

(In reply to Andrew McCreight [:mccr8] from comment #18)
> The keyword is checkin-needed. Remove it if you aren't yet ready for
> checkin. :)

Thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This is rated sec-high (and AFAICT, it affects more than just tb20), which means that the patch requires security approval to land, correct?

Comment 21

[Ludovic Hirlimann [:Usul]]

(In reply to Ryan VanderMeulen from comment #20)
> This is rated sec-high (and AFAICT, it affects more than just tb20), which
> means that the patch requires security approval to land, correct?

Irving can you do that , too much question I can't answer.

Comment 22

[:Irving Reid (No longer working on Firefox)]

The security level was set because of uninitialized memory use, and nobody has done the analysis to determine whether the problem is exploitable or not. I can't speak to what review is necessary; I'll leave that decision to Standard8.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 684029 [details] [diff] [review]
rev 04 of proposed patch

Note: I'm not altogether convinced this is a sec-high issue. There are some cases where we could go down random branches in the code due to this, but I suspect that in general all the default cases are handled, and realistically we'd only hit this in an add-on situation (though I've not checked all these routes myself, so there may be something internally to TB that could affect this still).

[Security approval request comment]
How easily can the security issue be deduced from the patch? Quite easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The comments would help.

Which older supported branches are affected by this flaw? Probably all including 10.0.x.

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, small differences probably just to account for type name changes & a few other things.

How likely is this patch to cause regressions; how much testing does it need? Its possible, could do with a bit of time on beta before pushing to release.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 684029 [details] [diff] [review]
rev 04 of proposed patch

sec-approval+.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 684029 [details] [diff] [review]
rev 04 of proposed patch

[Triage Comment]
Approving for aurora/beta now so that we can get this out for testing in the next beta. Flagging for 17 as well. Looks like we'll need a back-ported patch for ESR 10.

Comment 27

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/bef6aa5c28f7
https://hg.mozilla.org/releases/comm-beta/rev/05854645c419

Comment 28

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr17/rev/3cc88679f0b7

Comment 29

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #28)
> https://hg.mozilla.org/releases/comm-esr17/rev/3cc88679f0b7

And a minor bustage fix due to the recent nsCAutoString -> nsAutoCString transition:

https://hg.mozilla.org/releases/comm-esr17/rev/c96629272e7f

Comment 30

[Mark Banner (:standard8)]

Attachment: ESR 10 patch (unbitrotted)

This is the fixed up version for ESR 10. There were no functional changes in the patch between 10 & 17, just bitrot issues.

Comment 31

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr10/rev/95b1e9e74157

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Did this make it into the current ESR builds for next week?

Comment 33

[Mark Banner (:standard8)]

Yes it has.

Comment 35

[ISHIKAWA, Chiaki]

(In reply to Al Billings [:abillings] from comment #32)
> Did this make it into the current ESR builds for next week?

Hi,

I got a notice from Chris Hoffmann that I have won the security bounty award(!) for reporting and fixing the problem.

Without Gary's designating this bugzilla entry as a security issue, I have never thought of being part of the security bounty efforts.

I would like to thank everybody  who pitched in to fix this bug as well as Gary.

Now, I have a request. Can I take a look at the draft of the announcement that you will put regarding the security issues: I am curious how my name will appear
in it and might want to ask a few modifications if possible.

TIA and what a surprise to receive a bounty in new year.
In Japan, we have a practice of giving a monetary allowance during new year holidays to children (like a Christmas gift in North America, but usually given as some amount of money so that children can go out and spend it as they see fit. Buy a toy, buy some sweet, use it to go to amusement park, save it for the future like buying an expensive book later, etc. I used to buy plastic models like Tamiya's when I was young.).
Receiving the bounty at this time of the year is like receiving this new year's monetary allowance, called "Otoshidama" in
Japan. [Except that I have been on the giver's side for a quite some time now :-) ]

Thank you again.

Comment 36

[Al Billings [:abillings - ex-MoCo]]

 This is for bug 809064, which was fixed in the release that went live yesterday. The bug was mentioned in the roll up advisory at http://www.mozilla.org/security/announce/2013/mfsa2013-01.html. It did not get a separate advisory, but was included in the "Miscellaneous memory safety hazards" advisory.