1347657 - nsUrlClassifierStreamUpdater::FetchNextRequest has potential use-after-free issue

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement)

Description

[Henry Chang [:hchang]]

http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#

nsresult
nsUrlClassifierStreamUpdater::FetchNextRequest()
{
  //
  PendingRequest &request = mPendingRequests[0];
  DownloadUpdates(...) // (1)
  request.mSuccessCallback = nullptr;
}

At (1), mPendingRequests may be realloc'ed internally because of AppendElement [1][2]. If the realloc happens, |request| becomes a dangling reference.

[1] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#236
[2] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#276

Comment 1

[Henry Chang [:hchang]]

https://public-artifacts.taskcluster.net/PNtHucRCRoOcK38JRpKplg/0/public/logs/live_backing.log

Try run for Bug 1339050 hit this long-standing issue :(

Comment 2

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry][:hchang] from comment #0)
> http://searchfox.org/mozilla-central/rev/
> ca7015fa45b30b29176fbaa70ba0a36fe9263c38/toolkit/components/url-classifier/
> nsUrlClassifierStreamUpdater.cpp#
> 
> nsresult
> nsUrlClassifierStreamUpdater::FetchNextRequest()
> {
>   //
>   PendingRequest &request = mPendingRequests[0];
>   DownloadUpdates(...) // (1)
>   request.mSuccessCallback = nullptr;
> }
> 
> At (1), mPendingRequests may be realloc'ed internally because of
> AppendElement [1][2]. If the realloc happens, |request| becomes a dangling
> reference.
> 

What's actually caught by the address sanitizer is not 

"request.mSuccessCallback = nullptr"

but 

http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#237

because |aRequestTables| is the reference (const nsACString&) from a dangling reference.

Comment 3

[Henry Chang [:hchang]]

Attachment: Bug 1347657 - Use array entry as value instead of reference to avoid being invalidated by realloc.

nsTArray::AppendElement may cause memory reallocation if out of capacity.
In nsUrlClassifierStreamUpdater::FetchNextRequest(), we take the reference of
the first element of mPendingRequests and pass its member as reference to
DownloadUpdate(), where mPendingRequests.AppendElement will be called.
If the AppendElement in DownloadUpdate() causes realloc, the reference
becomes dangling.

The most efficient fix is to "move" the reference's (i.e. request)
member variables to DownloadUpdate() but I think in this case we can just
take the value from the array and pass it around with no given that the
array element contains simply a couple of strings and pointers.

Review commit: https://reviewboard.mozilla.org/r/120926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120926/

Comment 4

[François Marier [:francois]]

Comment on attachment 8847971 [details]
Bug 1347657 - Use array entry as value instead of reference to avoid being invalidated by realloc.

https://reviewboard.mozilla.org/r/120926/#review123216

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d4a86ef46e9
Use array entry as value instead of reference to avoid being invalidated by realloc. r=francois

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d4a86ef46e9

Comment 7

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75e2a3ffc1ee

Comment 8

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5350e77082b4

Comment 9

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f27895a4552

Comment 10

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf782c7da04

Comment 11

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd3762b92f3b