Fix potential UAF in DriveAppConverter.

Fix potential UAF in DriveAppConverter.

BUG=480941
Committed: https://crrev.com/462f20fa0030ef407fa258c2c83058234e1c64e6
Cr-Commit-Position: refs/heads/master@{#327428}

[xiyuan, 2015-04-27 17:18:07]

xiyuan@chromium.org changed reviewers:
+ benwells@chromium.org

[xiyuan, 2015-04-27 17:18:08]

Fix as suggested.

[pneubeck (no reviews), 2015-04-27 17:29:58]

pneubeck@chromium.org changed reviewers:
+ pneubeck@chromium.org

[pneubeck (no reviews), 2015-04-27 17:29:58]

lgtm

[benwells, 2015-04-27 22:22:10]

https://codereview.chromium.org/1105193002/diff/1/chrome/browser/apps/drive/d...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/1105193002/diff/1/chrome/browser/apps/drive/d...
chrome/browser/apps/drive/drive_app_converter.cc:114:
finished_callback_.Run(this, false);
Its also called here. Should this one be updated too?

TBH I don't really get the point of this change. Is the fear that |this| will be
referenced in the callback (DriveAppProvider::OnLocalAppConverted) after it is
freed? It doesn't appear like that is the case currently. Further, if
DriveAppProvider::OnLocalAppConverted was changed to use |converter| after the
pending coverter is erased (and thus destroyed), this change won't help.

Maybe someone can explain to me what I'm missing?

[xiyuan, 2015-04-27 22:54:14]

https://codereview.chromium.org/1105193002/diff/1/chrome/browser/apps/drive/d...
File chrome/browser/apps/drive/drive_app_converter.cc (right):

https://codereview.chromium.org/1105193002/diff/1/chrome/browser/apps/drive/d...
chrome/browser/apps/drive/drive_app_converter.cc:114:
finished_callback_.Run(this, false);
On 2015/04/27 22:22:10, benwells wrote:
> Its also called here. Should this one be updated too?
> 

Good catch. This should be updated too.

> TBH I don't really get the point of this change. Is the fear that |this| will
be
> referenced in the callback (DriveAppProvider::OnLocalAppConverted) after it is
> freed? It doesn't appear like that is the case currently. Further, if
> DriveAppProvider::OnLocalAppConverted was changed to use |converter| after the
> pending coverter is erased (and thus destroyed), this change won't help.
> 
> Maybe someone can explain to me what I'm missing?

My understanding is that the worry is callback might have stored bound arguments
which might be referenced after Callback::Run invokes the real function and
causes UAF. I don't know excatly how this could happen either. Maybe pneubeck@
can elaborate?

[pneubeck (no reviews), 2015-04-28 08:09:04]

On 2015/04/27 22:54:14, xiyuan wrote:
>
https://codereview.chromium.org/1105193002/diff/1/chrome/browser/apps/drive/d...
> File chrome/browser/apps/drive/drive_app_converter.cc (right):
> 
>
https://codereview.chromium.org/1105193002/diff/1/chrome/browser/apps/drive/d...
> chrome/browser/apps/drive/drive_app_converter.cc:114:
> finished_callback_.Run(this, false);
> On 2015/04/27 22:22:10, benwells wrote:
> > Its also called here. Should this one be updated too?
> > 
> 
> Good catch. This should be updated too.
> 
> > TBH I don't really get the point of this change. Is the fear that |this|
will
> be
> > referenced in the callback (DriveAppProvider::OnLocalAppConverted) after it
is
> > freed? It doesn't appear like that is the case currently. Further, if
> > DriveAppProvider::OnLocalAppConverted was changed to use |converter| after
the
> > pending coverter is erased (and thus destroyed), this change won't help.
> > 
> > Maybe someone can explain to me what I'm missing?
> 
> My understanding is that the worry is callback might have stored bound
arguments
> which might be referenced after Callback::Run invokes the real function and
> causes UAF. I don't know excatly how this could happen either. Maybe pneubeck@
> can elaborate?

Approximately.
Assume the callback has bound arguments (e.g. change it to
base::Bind(&DriveAppProvider::OnLocalAppConverted, base::Unretained(this),
std::string("abc")) ).
If the callback is destroyed, the bound std::string("abc") will be destroyed as
well, even if that happens within the Callback::Run().
The callback will be destroyed together with the converter (in the statement
pending_converters_.erase(pending_converters_.begin()) ),
which _will_ happen during Callback::Run().
The std::string argument to OnLocalAppConverted will silently become invalid
after the pending_converters_.erase .

I know that this currently is not a problem, because there are no bound
arguments.
However, someone who adds these arguments will likely not know how the callback
is called in the converter.

[benwells, 2015-04-29 00:28:43]

lgtm

[xiyuan, 2015-04-29 00:30:50]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2015-04-29 00:30:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1105193002/#ps20001
(title: "update the other call site")

[commit-bot: I haz the power, 2015-04-29 00:31:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1105193002/20001

[commit-bot: I haz the power, 2015-04-29 03:22:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-29 03:23:20]

Patchset 2 (id:??) landed as
https://crrev.com/462f20fa0030ef407fa258c2c83058234e1c64e6
Cr-Commit-Position: refs/heads/master@{#327428}