Bug 1660223 Comment 15 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Original comment by
on 
Presumably this is a case where (In reply to Jonathan Watt [:jwatt] from comment #1)
> So the `gtk_enumerate_printers` call made via the runable here:
> https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/widget/gtk/nsDeviceContextSpecG.cpp#266

Here's a more current searchfox link; this particular snippet is unchanged:
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#357-360
```cpp
  NS_DispatchToCurrentThread(
      NewRunnableMethod("nsDeviceContextSpecGTK::EnumeratePrinters", this,
                        &nsDeviceContextSpecGTK::EnumeratePrinters));
}
```

I'm noticing one red flag here... We're passing a raw `this` pointer in to a runnable there which gets executed asynchronously, and I'm not sure we have any guarantee that `this` will still be alive when the runnable gets executed.  (`this` here is a `nsDeviceContextSpecGTK` which is a refcounted object, owned by the print data for the particular print job, I think).

It seems conceivable that if the print operation fails or is canceled and the `nsDeviceContextSpecGTK` is torn down before the runnable can be executed, then that would trivially trigger a UAF there.

We've got the same possible-problem for our other `NewRunnableMethod` invocation, here:
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#293-296
Revision 1 by
on 
Presumably this is a case where (In reply to Jonathan Watt [:jwatt] from comment #1)
> So the `gtk_enumerate_printers` call made via the runable here:
> https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/widget/gtk/nsDeviceContextSpecG.cpp#266

Here's a more current searchfox link; though this particular snippet is unchanged since comment 1 was posted.
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#357-360
```cpp
  NS_DispatchToCurrentThread(
      NewRunnableMethod("nsDeviceContextSpecGTK::EnumeratePrinters", this,
                        &nsDeviceContextSpecGTK::EnumeratePrinters));
}
```

I'm noticing one red flag here... We're passing a raw `this` pointer in to a runnable there which gets executed asynchronously, and I'm not sure we have any guarantee that `this` will still be alive when the runnable gets executed.  (`this` here is a `nsDeviceContextSpecGTK` which is a refcounted object, owned by the print data for the particular print job, I think).

It seems conceivable that if the print operation fails or is canceled and the `nsDeviceContextSpecGTK` is torn down before the runnable can be executed, then that would trivially trigger a UAF there.

We've got the same possible-problem for our other `NewRunnableMethod` invocation, here:
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#293-296
Revision 2 by
on 
In reply to Jonathan Watt [:jwatt] from comment #1)
> So the `gtk_enumerate_printers` call made via the runable here:
> https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/widget/gtk/nsDeviceContextSpecG.cpp#266

Here's a more current searchfox link; though this particular snippet is unchanged since comment 1 was posted.
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#357-360
```cpp
  NS_DispatchToCurrentThread(
      NewRunnableMethod("nsDeviceContextSpecGTK::EnumeratePrinters", this,
                        &nsDeviceContextSpecGTK::EnumeratePrinters));
}
```

I'm noticing one red flag here... We're passing a raw `this` pointer in to a runnable there which gets executed asynchronously, and I'm not sure we have any guarantee that `this` will still be alive when the runnable gets executed.  (`this` here is a `nsDeviceContextSpecGTK` which is a refcounted object, owned by the print data for the particular print job, I think).

It seems conceivable that if the print operation fails or is canceled and the `nsDeviceContextSpecGTK` is torn down before the runnable can be executed, then that would trivially trigger a UAF there.

We've got the same possible-problem for our other `NewRunnableMethod` invocation, here:
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#293-296
Revision 3 by
on 
In reply to Jonathan Watt [:jwatt] from comment #1)
> So the `gtk_enumerate_printers` call made via the runable here:
> https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/widget/gtk/nsDeviceContextSpecG.cpp#266

Here's a more current searchfox link; though this particular snippet is unchanged since comment 1 was posted.
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#357-360
```cpp
  NS_DispatchToCurrentThread(
      NewRunnableMethod("nsDeviceContextSpecGTK::EnumeratePrinters", this,
                        &nsDeviceContextSpecGTK::EnumeratePrinters));
}
```

I'm noticing one red flag here... We're passing a raw `this` pointer in to a runnable there which gets executed asynchronously, and I'm not sure we have any guarantee that `this` will still be alive when the runnable gets executed.  (`this` here is a `nsDeviceContextSpecGTK` which is a refcounted object, owned by the print data for the particular print job, I think).

It seems conceivable that if the print operation fails or is canceled and the `nsDeviceContextSpecGTK` is torn down before the runnable can be executed, then that would trivially trigger a UAF here, when the runnable is executed.

We've got the same possible-problem for our other `NewRunnableMethod` invocation, here:
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#293-296
Revision 4 by
on 
In reply to Jonathan Watt [:jwatt] from comment #1)
> So the `gtk_enumerate_printers` call made via the runable here:
> https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/widget/gtk/nsDeviceContextSpecG.cpp#266

Here's a more current searchfox link; though this particular snippet is unchanged since comment 1 was posted.
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#357-360
```cpp
  NS_DispatchToCurrentThread(
      NewRunnableMethod("nsDeviceContextSpecGTK::EnumeratePrinters", this,
                        &nsDeviceContextSpecGTK::EnumeratePrinters));
}
```

I'm noticing one red flag here... We're passing a raw `this` pointer in to a runnable there which gets executed asynchronously, and I'm not sure we have any guarantee that `this` will still be alive when the runnable gets executed.  (`this` here is a `nsDeviceContextSpecGTK` which is a refcounted object, owned by the print data for the particular print job, I think).

It seems conceivable that if the print operation fails or is canceled and the `nsDeviceContextSpecGTK` is torn down before the runnable can be executed, then that would trivially trigger a UAF here, when the runnable is executed.

[EDIT: This actually shouldn't be a problem; see comment 20. `NewRunnableMethod` takes this into consideration and generates a runnable that maintains a strong reference to the passed-in `this` pointer.]
Revision 5 by
on 
In reply to Jonathan Watt [:jwatt] from comment #1)
> So the `gtk_enumerate_printers` call made via the runable here:
> https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/widget/gtk/nsDeviceContextSpecG.cpp#266

Here's a more current searchfox link; though this particular snippet is unchanged since comment 1 was posted.
https://searchfox.org/mozilla-central/rev/16526d690cccc9c60cacf09cc08e2c3041ef884e/widget/gtk/nsDeviceContextSpecG.cpp#357-360
```cpp
  NS_DispatchToCurrentThread(
      NewRunnableMethod("nsDeviceContextSpecGTK::EnumeratePrinters", this,
                        &nsDeviceContextSpecGTK::EnumeratePrinters));
}
```

~I'm noticing one red flag here... We're passing a raw `this` pointer in to a runnable there which gets executed asynchronously, and I'm not sure we have any guarantee that `this` will still be alive when the runnable gets executed.  (`this` here is a `nsDeviceContextSpecGTK` which is a refcounted object, owned by the print data for the particular print job, I think).~

~It seems conceivable that if the print operation fails or is canceled and the `nsDeviceContextSpecGTK` is torn down before the runnable can be executed, then that would trivially trigger a UAF here, when the runnable is executed.~

[EDIT: This actually shouldn't be a problem; see comment 20. `NewRunnableMethod` takes this into consideration and generates a runnable that maintains a strong reference to the passed-in `this` pointer.]

Back to Bug 1660223 Comment 15