Closed Bug 1178890 Opened 9 years ago Closed 9 years ago

TimerThread::DoAfterSleep() seems to not be threadsafe

Categories

(Core :: XPCOM, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 + verified
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- wontfix
firefox-esr38 40+ verified
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: bwc, Assigned: jesup)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main40+][adv-esr38.2+])

Attachments

(1 file, 1 obsolete file)

TimerThread::DoAfterSleep() iterates over mTimers without any locking, updating timeouts. Any insertions of new timers (or canceling/rescheduling) is likely to go awry if DoAfterSleep is running.

I need to verify that this function is ever called, since dxr doesn't seem to know about any callers.
Right, but dxr doesn't know about callers of |Observe|.
Verified that |DoAfterSleep| is indeed called when coming out of sleep. This is an actual problem. This could be the root cause for bug 1151046 (since this could cause timers to be inserted in the wrong order).
See Also: → 1151046
Calling this sec-high, since this could easily lead to a UAF of the buffer that backs mTimers, if a re-allocation happens due to an insertion while DoAfterSleep is running. It could also trip up code that relies on timers firing in a deterministic order.
Keywords: sec-high
This just gets better and better. The call to timer->SetDelay() in DoAfterSleep eventually leads here:

https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#387

There are a few things worth noting here:
1. We lock in here, which is going to make fixing this bug a little more complex.
2. We remove the timer from mTimers, and re-insert it, while iterating over mTimers (yikes)
3. This code assumes the caller is holding onto a strong ref to the timer (it isn't; also yikes)
It seems like it might be prudent to lock, copy mTimers into a temporary, clear mTimers, unlock, and then reschedule everything.
This is sounding more like a sec-crit given the UAF potential and the possibility it could cause revectoring of calls.  Thanks for digging in, Byron.  I agree this may explain bug 1151046, of which analysis has not yielded any way the timers could be out-of-order.

NI dveditz on the sec level (or froyd may want to opine)

Likely affects all versions and ESRs
Severity: normal → critical
Flags: needinfo?(dveditz)
Ok, the more I think about this the weirder it becomes. On wake from sleep, we are trying to re-schedule all existing timers why? For example, if I schedule a timer to fire in one minute, and just before this timer pops the machine goes to sleep, why should it be re-scheduled for one minute after the machine comes out of sleep? Shouldn't it fire right away? Put another way, if I schedule a timer (A) for one minute, and 50 seconds later I schedule a timer (B) for 30 seconds, the expected order of firing is A then B. But if the machine goes to sleep, after waking the order will be B then A!
(This is assuming the algorithm works; we're mutating mTimers while iterating over it so it probably doesn't, but this does seem to be the intent)
Do you have any idea why it might be a good idea to reschedule timers like described in comment 8?
Flags: needinfo?(khuey)
(In reply to Byron Campen [:bwc] from comment #8)
> Ok, the more I think about this the weirder it becomes. On wake from sleep,
> we are trying to re-schedule all existing timers why? For example, if I
> schedule a timer to fire in one minute, and just before this timer pops the
> machine goes to sleep, why should it be re-scheduled for one minute after
> the machine comes out of sleep? Shouldn't it fire right away? Put another
> way, if I schedule a timer (A) for one minute, and 50 seconds later I
> schedule a timer (B) for 30 seconds, the expected order of firing is A then
> B. But if the machine goes to sleep, after waking the order will be B then A!

We're not rescheduling them for the unelapsed time, but rather the original time interval?  That does seem crazy.
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> (In reply to Byron Campen [:bwc] from comment #8)
> > Ok, the more I think about this the weirder it becomes. On wake from sleep,
> > we are trying to re-schedule all existing timers why? For example, if I
> > schedule a timer to fire in one minute, and just before this timer pops the
> > machine goes to sleep, why should it be re-scheduled for one minute after
> > the machine comes out of sleep? Shouldn't it fire right away? Put another
> > way, if I schedule a timer (A) for one minute, and 50 seconds later I
> > schedule a timer (B) for 30 seconds, the expected order of firing is A then
> > B. But if the machine goes to sleep, after waking the order will be B then A!
> 
> We're not rescheduling them for the unelapsed time, but rather the original
> time interval?  That does seem crazy.

   Yeah. At first I thought that this might be doing something reasonable like making the time during sleep "not count", but it is just straight up rescheduling everything for now + delay (except for repeating precise, in which case it is scheduled for <old-timeout> + delay, which is also weird).
I'm going to rip out this rescheduling business and do a try push, and see what happens.
I would expect some form of either unexpired-time, or reschedule with sleep-time removed (though that may cause bad amounts of firing all at once coming out of sleep).  Reschedule with Original time is just plain wacky.

If it was decrementing 'delay' as it goes along then it would be unexpired-time - though there still could be vulnerabilities.

BTW,the 1 minute and then 50-seconds later add a 30-second timer case would be racy unless you have some other way to *know* >30 seconds had elapsed before scheduling it (which one could, but it opens the question of timer accuracy - generally timers are a form of "at or after".  Cross-timer invariants generally involve 0-timeout timers or "B will happen after A if B has an equal-or-longer delay than A and is added after A".  If B has shorter than A, then I'm not sure when of if the invariant applies (without more thought or doc/code-reading).
(In reply to Randell Jesup [:jesup] from comment #7)
> This is sounding more like a sec-crit given the UAF potential

Do we have a reasonably reliable way to trigger these crashes from external content? If not sec-high is fine for racy pitfalls. Then again if user content can directly cause timer creation (setTimeout?) then maybe that is enough to play with that we should just assume sec-critical. I'm fine either way, and either way we'll definitely backport this to ESR.
The problem is that try runs don't sleep....  This may require manual testing.
Absolutely. Question is, what should I test? This effects a lot more than just webrtc.
In particular, I worry that there is code out there that depends on us not firing a bunch of old timers all at once when something comes out of sleep.
(In reply to Byron Campen [:bwc] from comment #20)
> In particular, I worry that there is code out there that depends on us not
> firing a bunch of old timers all at once when something comes out of sleep.

Exactly.  And even if it doesn't depend on it, it could cause real annoyance to users/sites.  I think we need to account for remaining time if at all possible, and *specify* the invariants.

I think you'll need to craft a test that abuses the timer API and checks the invariants constantly, and sleep/wake-up many times.  We should get QA to pound on this after vetting the test program.  And test normal operation.
(In reply to Randell Jesup [:jesup] from comment #21)
> (In reply to Byron Campen [:bwc] from comment #20)
> > In particular, I worry that there is code out there that depends on us not
> > firing a bunch of old timers all at once when something comes out of sleep.
> 
> Exactly.  And even if it doesn't depend on it, it could cause real annoyance
> to users/sites.  I think we need to account for remaining time if at all
> possible, and *specify* the invariants.
> 
> I think you'll need to craft a test that abuses the timer API and checks the
> invariants constantly, and sleep/wake-up many times.  We should get QA to
> pound on this after vetting the test program.  And test normal operation.

   Accounting for remaining time seems reasonable, and is definitely way more reasonable than what we're doing now. I'll have to think about what I would put in such a test-case.
First cut idea: schedule a bunch of random short timers (and when the fire, schedule new short timers).  link each one to the list of timers that the invariants say should not fire before it.  Include a non-0 chance that we select 0 for a timeout, and that we schedule multiple 0 timeouts in the same JS sequence (to check the invariant that scheduling two 0-timeout timers will expire in order always).  Start this, then sleep/wakeup/repeat ad nauseum until the tester gets tired.  For extra points (but not as the only test, since it single-cores things!) do it under rr. And then repeat on different OS's.

The actual testing (once you verify the test works) can be done by Juan or perhaps better softvision.
We'll need multiple threads scheduling timers. To do this with JS will require workers I guess, and a human causing sleep. It would be vastly better to have a test-case in c++ land.
Let's get a fix now, and then deal with the test/verification.  I think we know there's a problem now that can lead to a UAF (not to mention the functionality issues that may have caused other problems).

We could write a simple testcase that invokes one of the problematic issues waking from sleep.
First-cut fix proposal: (to avoid this drifting)

a) add locking (yes, this means some extra overhead in timers)
b) if we know the sleep-time delta (got pre-sleep notification), then adjust all timers forward by delta (and ensure that the shortest timer ends up >= 0; if not adjust all forward such that it has a 0 timeout).  Retain ordering.  This effectively attempts to reschedule based on "remaining time" as best we can tell.
c) if we don't have a sleep-time delta (no pre-sleep time), adjust all timers by a delta based on the last timer that fired (or was added) before the sleep used as the pre-sleep time, and process as per 'b' above.  This may be over-aggressive about increasing timeouts as it'll be an over-estimate of slept time (though likely they'll be close to correct).

This should be safe/low-risk.  (Note that some timeout-based things may still fail after wakeup (like timed re-registration); this doesn't attempt to solve such an issue, but it does reduce the impact of such an issue somewhat.  To 'solve' such an issue would require exposing "wakeup" events to content so it knows to re-register/etc.)

After landing this we can look at options with lower overhead (minimize/avoid locks, etc).  But right now it's unsafe, and can cause incorrect firing orders and break things.

Nathan: how do you want to proceed?  Comments from anyone else/Byron?
Flags: needinfo?(nfroyd)
I'm working on a timer fuzz test in TestTimers. Seeing some unexpected behavior without any sleep business, trying to get to the bottom of it. Might spawn additional sec bugs.
(In reply to Randell Jesup [:jesup] from comment #26)
> First-cut fix proposal: (to avoid this drifting)
> 
> a) add locking (yes, this means some extra overhead in timers)

The plans for adjusting the firing time of timers seems straightforward enough, though I'm a bit curious about whether there are races between timers going off once we come out of sleep and those same timers being adjusted forward...maybe we just have to accept that...

It's not clear to me what "add locking" means--what are you locking, when are you locking it, etc.  My immediate assumption is locking the manipulation of timers we do once we come out of sleep, but then I don't see how that adds the extra overhead in timers that you mention.  Can you elaborate on this part of the plan, or point out where I am missing things?
Flags: needinfo?(nfroyd)
We would need to lock TimerThread::mMonitor for the duration of timer adjustment. Given that this is simply a matter of a single integer addition for each scheduled timer, and these timers are stored in an array, this should not introduce much contention with other threads trying to schedule/cancel/etc timers.
I'm working on a patch for this
Note that we *never* fire sleep/wake/suspend/resume except on Windows....  Doing a windows build to try to 5Dtest there
Attachment #8631220 - Flags: review?(nfroyd)
Attachment #8631220 - Flags: review?(docfaraday)
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
(In reply to Randell Jesup [:jesup] from comment #31)
> Created attachment 8631220 [details] [diff] [review]
> Update timer arrays after sleep to account for time sleeping
> 
> Note that we *never* fire sleep/wake/suspend/resume except on Windows.... 
> Doing a windows build to try to 5Dtest there

   I checked on OS X, and DoAfterSleep() is called there too.
Aha, hiding in widget/cocoa/nsToolkit.mm (search macros bit me)
Comment on attachment 8631220 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping

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

There's some simplification and threadsafety work that I want to see here.

::: xpcom/threads/TimerThread.cpp
@@ +30,5 @@
>    mShutdown(false),
>    mWaiting(false),
>    mNotified(false),
> +  mSleeping(false),
> +  mLastTimerFiredAt(TimeStamp::Now())

I would make this mLastTimeEventLoopRan (or similar), and update it here: https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp?from=TimerThread.cpp#244

@@ +484,5 @@
>  
>  void
>  TimerThread::DoBeforeSleep()
>  {
> +  mStartedSleepAt = TimeStamp::Now();

I don't think it is necessary to have a second variable if we're remembering the last time the event loop ran; that should be more than good enough. Nobody expects high precision when coming out of sleep. Also, I'm not sure what thread this is called from, but I would be surprised if it were the TimerThread. Lastly, removing this extra variable simplifies the logic in DoAfterSleep considerably.

@@ +502,5 @@
> +    // a small over-estimate
> +    slept = now - mStartedSleepAt;
> +  } else {
> +    // an over-estimate of time slept, usually small
> +    slept = now - mLastTimerFiredAt;

I'm not totally sure what thread this ends up being called from; I suspect it is not the TimerThread. Since we need to lock mMonitor anyway to update the timeouts, we can just protect ourselves with that.

@@ +516,4 @@
>    for (uint32_t i = 0; i < mTimers.Length(); i ++) {
>      nsTimerImpl* timer = mTimers[i];
> +    if (timer->mTimeout + slept < now) {
> +      slept = now - timer->mTimeout;

Expired timers are not all that unusual, so I don't really see the harm. Also, if we just use the last time we ran the event loop, we should never have a timer that satisfies timer->mTimeout + slept < now.

@@ +526,5 @@
> +    nsTimerImpl* timer = mTimers[i];
> +    timer->mTimeout += slept;
> +  }
> +  mSleeping = false;
> +  mLastTimerFiredAt = now;

Probably not necessary? I guess there's no harm.

@@ +531,4 @@
>  
> +  // Wake up the timer thread to process the updated array
> +  mNotified = true;
> +  mMonitor.Notify();

Good idea.
Attachment #8631220 - Flags: review?(docfaraday) → review-
I verified the original patch on Windows and the 'slept' adjustment looks reasonable.  Added a couple of assertions about the monitor being held; those aren't necessary, but if we think the perf impact of them is small (likely, though maybe this is expensive on windows?) in debug builds we should keep them.
Attachment #8631596 - Flags: review?(nfroyd)
Attachment #8631596 - Flags: review?(docfaraday)
Attachment #8631220 - Attachment is obsolete: true
Attachment #8631220 - Flags: review?(nfroyd)
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping

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

Looks good, only would like to see one small change.

::: xpcom/threads/TimerThread.cpp
@@ +479,5 @@
>  TimerThread::ReleaseTimerInternal(nsTimerImpl* aTimer)
>  {
> +  if (!mShutdown) {
> +    // copied to a local array before releasing in shutdown
> +    mMonitor.AssertCurrentThreadOwns();

Yikes. Sometimes we call this with a lock, sometimes we don't... I'll see if this can be cleaned up.

@@ +507,1 @@
>    mSleeping = true; // wake may be notified without preceding sleep notification

Since we're locking now, I don't think anything will be able to observe this transition, so it can probably go away.
Attachment #8631596 - Flags: review?(docfaraday) → review+
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping

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

WFM.  I think removing mSleeping is safe, but I'd have to think about it a little bit, and I'd prefer it happen in a separate patch, probably in a different bug.

::: xpcom/threads/TimerThread.cpp
@@ +500,5 @@
>  TimerThread::DoAfterSleep()
>  {
> +  // Mainthread
> +  TimeStamp now = TimeStamp::Now();
> +  TimeDuration slept;

Code nit: I think this declaration should be moved down and combined with the initialization of |slept|, below.
Attachment #8631596 - Flags: review?(nfroyd) → review+
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Hard or very hard, quite possibly extremely hard/impossible - and requires sleep and wakeup to even have a chance

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Clearly adds locking and monitor checks.  The rest of the patch has a clear purpose (avoiding timer reordering, which has limited to no direct security implications, so that makes it less obvious.

Which older supported branches are affected by this flaw?
All - code predates VCS->mercurial

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply trivially, or be trivial to merge.


How likely is this patch to cause regressions; how much testing does it need?
Overall low risk, though in a critical piece.  Changes worth noting though are only in the Wakeup path.    A test rig (c++ unit test perhaps) to exercise this and the wake/sleep code might help; otherwise minor manual testing on mac and Windows (no sleep/wake notification on linux it appears).  It's unlikely to be worse than the current code.

Given the long history of this flaw, we could ride the trains, or bake on nightly for a while before uplift.
Attachment #8631596 - Flags: sec-approval?
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping

sec-approval+. We'll want branch patches, including ESR.
Attachment #8631596 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/1ab4e8c69ee1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Possible UAF (unproven but believable) in timer code

User impact if declined: 
Low if no one has managed to exploit it yet (or after it shows up in the tree).  Also can only happen during sleep/wakeup.

Fix Landed on Version:
42

Risk to taking this patch (and alternatives if risky): 
Pretty low risk for a core xpcom change, and limited to Wakeup.  Grabs the lock (duh) and actually simplified the code a bunch.

String or UUID changes made by this patch: 
none
Attachment #8631596 - Flags: approval-mozilla-esr38?
Attachment #8631596 - Flags: approval-mozilla-esr31?
Attachment #8631596 - Flags: approval-mozilla-beta?
Attachment #8631596 - Flags: approval-mozilla-aurora?
Comment on attachment 8631596 [details] [diff] [review]
Update timer arrays after sleep to account for time sleeping

sec-high, taking it.
However, not on esr31 as we won't be doing a new esr (except in case of chemspill).
Attachment #8631596 - Flags: approval-mozilla-esr38?
Attachment #8631596 - Flags: approval-mozilla-esr38+
Attachment #8631596 - Flags: approval-mozilla-esr31?
Attachment #8631596 - Flags: approval-mozilla-esr31-
Attachment #8631596 - Flags: approval-mozilla-beta?
Attachment #8631596 - Flags: approval-mozilla-beta+
Attachment #8631596 - Flags: approval-mozilla-aurora?
Attachment #8631596 - Flags: approval-mozilla-aurora+
(In reply to Randell Jesup [:jesup] from comment #23)
> The actual testing (once you verify the test works) can be done by Juan or
> perhaps better softvision.

Softvision can help here if you still need it and have a test at hand. If you do, then please give us some details on: test steps, expected bad behavior and expected fixed behavior.
Flags: needinfo?(rjesup)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #47)
> (In reply to Randell Jesup [:jesup] from comment #23)
> > The actual testing (once you verify the test works) can be done by Juan or
> > perhaps better softvision.
> 
> Softvision can help here if you still need it and have a test at hand. If
> you do, then please give us some details on: test steps, expected bad
> behavior and expected fixed behavior.

We don't have a test in hand, the bug was found by inspection.  Byron is working on a way to test this and other aspects of the timer API, but that's a complex task. 

A basic functionality test would be to set up something that will timeout in a minute (and display some text on the page, or whatever), then put the machine to sleep for a few minutes, and wake it up and see if the timer fires more or less when it should (with a 2 minute timer, and you put it to sleep after 30 seconds, it should fire around 1:30 after waking up - note that time will start once you wake the machine up, not when you log in.  This merely verifies we didn't break anything major.  (I did smoketests like this before landing.)

This will only be seen on Windows and Mac; we don't have any direct support for sleep notification on linux
Flags: needinfo?(rjesup) → needinfo?(florin.mezei)
Thanks Randell! Setting as qe-verify+.

I'm assuming a simple example to use for the basic functionality test would be something like this: http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_settimeout. Or more similar examples like this: http://www.w3schools.com/jsref/met_win_settimeout.asp.
Flags: needinfo?(florin.mezei) → qe-verify+
> I'm assuming a simple example to use for the basic functionality test would
> be something like this:
> http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_settimeout.
> Or more similar examples like this:
> http://www.w3schools.com/jsref/met_win_settimeout.asp.

yes
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #49)
> Thanks Randell! Setting as qe-verify+.
> 
> I'm assuming a simple example to use for the basic functionality test would
> be something like this:
> http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_settimeout.
> Or more similar examples like this:
> http://www.w3schools.com/jsref/met_win_settimeout.asp.

I used this examples and the steps provided by Randell from comment 48 with Firefox 40 beta 6 and latest ESR 38.1.0 tinderbox build. The alert appears exactly after the time I set, after waking up the PC from sleep. I used Windows 7 64-bit and Mac OS X 10.10.4 OS`s.
There have been reports on Yammer of beachballing coming out of sleep on Macs recently.  Since this has recently changed and affects sleep, I want to take a close look at these reports.  It may be coincidence.
FWIW we get bouts of people reporting hangs after sleep occasionally, stretching back for years.
(In reply to Andrew McCreight [:mccr8] from comment #55)
> FWIW we get bouts of people reporting hangs after sleep occasionally,
> stretching back for years.

Hmmm.  Who knows, maybe this will help that!  Before this patch, timers could end up out-of-order after sleep.  If it's not a recent regression (and IIRC sheppy thought it was), then it's not me.  Fingers crossed.  (though this seems like a pretty clean patch).  If your process vm size is especially large, that might cause fun on wakeup
Flags: qe-verify+
Whiteboard: [adv-main40+][adv-esr38.2+]
byron - awesome to hear this is fixed. thanks!

I have sort of a drive by thought - over in 

https://bugzilla.mozilla.org/show_bug.cgi?id=1152048 we found that NS_WIDGET_WAKE_OBSERVER_TOPIC would call observers re-entrantly (emerging from sleep). That was causing a deadlock in socket transport (fixed)

https://bugzilla.mozilla.org/show_bug.cgi?id=1186074 was filed to notify the observers asynchronously from the wake.

According to my really quick drive by -it looks like the timer code could also have this problem, and the new locks added here might actually deadlock on it (though there was always a correctness problem).
Flags: needinfo?(docfaraday)
I think the key problem here was the call to nsTimerImpl::SetDelay, which then caused the reentrancy. We no longer call SetDelay in DoAfterSleep; we simply do some arithmetic on the timeouts, kick the monitor, and return. I bet this fix would have fixed your deadlock too.
Flags: needinfo?(docfaraday)
@mcmanus Hmm, looking at the stack in https://crash-stats.mozilla.org/report/index/5dbc5816-6b99-409f-9286-089bb2150704, it doesn't make any sense. The stack has nsTimerImpl::SetDelay calling nsIOService::Observe, which is definitely not something that the code has ever done. That stack is trashed, I think. I don't think this bug could have caused stack corruption, though.
The stack is a little weird, but I think the bigger point about it is how PostSleepWakeNotification calls all of its observers (including the timer code) re-rentrantly from whatever happened to be interrupted when you went to sleep. In the case of the stack trace in that crash it is network code (send()), but it seems like it could just as easily be timer code where send() sits in that trace.
if this turns out to be more theoretical than practical then just fixing https://bugzilla.mozilla.org/show_bug.cgi?id=1186074 on nightly only makes sense.
Depends on: 1197152
Group: core-security → core-security-release
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.