Re: Audit your code if you use mozilla::WeakPtr
On 10/8/13 10:32 PM, Justin Dolske wrote: Would that be a kungFuDeathGrep? (Sorry. Just making weak pun.) You mean mozilla::WeakpPunT? -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Audit your code if you use mozilla::WeakPtr
I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
usage unsafe by default. The issue is that WeakPtr provides convenience
methods such as operator-, which mean that the consumer can directly
dereference it without the required null checking first. This means that
you can have code like the below:
WeakPtrClass foo = realObject-asWeakPtr();
// ...
foo-Method();
That will happily compile and will crash at runtime if the object behind
the weak pointer is dead. The correct way of writing such code is:
Class* object = foo.get();
if (object) {
object-Method();
}
I don't know enough about all of the places which use WeakPtr myself to fix
them all, but if you have code using this in your module, please spend some
time auditing the code, and fix it. Please file individual bugs for your
components blocking bug 924658.
Thanks!
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Audit your code if you use mozilla::WeakPtr
Isn't it ultimately up to the developer to get it right? Someone could just as
well forget to use |if (object)| from your example.
Here's a sample usage from the header file:
* // Test a weak pointer for validity before using it.
* if (weak) {
* weak-num = 17;
* weak-act();
* }
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 3:54:17 PM
Subject: Audit your code if you use mozilla::WeakPtr
I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
usage unsafe by default. The issue is that WeakPtr provides convenience
methods such as operator-, which mean that the consumer can directly
dereference it without the required null checking first. This means that
you can have code like the below:
WeakPtrClass foo = realObject-asWeakPtr();
// ...
foo-Method();
That will happily compile and will crash at runtime if the object behind
the weak pointer is dead. The correct way of writing such code is:
Class* object = foo.get();
if (object) {
object-Method();
}
I don't know enough about all of the places which use WeakPtr myself to fix
them all, but if you have code using this in your module, please spend some
time auditing the code, and fix it. Please file individual bugs for your
components blocking bug 924658.
Thanks!
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:10 PM, David Major wrote:
Isn't it ultimately up to the developer to get it right? Someone could just as
well forget to use |if (object)| from your example.
Here's a sample usage from the header file:
* // Test a weak pointer for validity before using it.
* if (weak) {
* weak-num = 17;
* weak-act();
* }
Sure, but that convenience operator makes it natural to write
incorrect code by default. It's better to be explicit and correct, than
implicit and wrong. :-)
Cheers,
Ehsan
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 3:54:17 PM
Subject: Audit your code if you use mozilla::WeakPtr
I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
usage unsafe by default. The issue is that WeakPtr provides convenience
methods such as operator-, which mean that the consumer can directly
dereference it without the required null checking first. This means that
you can have code like the below:
WeakPtrClass foo = realObject-asWeakPtr();
// ...
foo-Method();
That will happily compile and will crash at runtime if the object behind
the weak pointer is dead. The correct way of writing such code is:
Class* object = foo.get();
if (object) {
object-Method();
}
I don't know enough about all of the places which use WeakPtr myself to fix
them all, but if you have code using this in your module, please spend some
time auditing the code, and fix it. Please file individual bugs for your
components blocking bug 924658.
Thanks!
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Audit your code if you use mozilla::WeakPtr
Does it? I don't think I'm any more or less likely to omit a validity check
using operator-() vs get(). Maybe it's just me.
It seems like get() might actually be *more* prone to failure. Imagine:
Class* object = foo.get();
if (object) {
object-Method();
}
// ... A lot of stuff happens and the ref blows up ...
if (object) {
object-Method(); // oops
}
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: David Major dma...@mozilla.com
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:27:03 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:10 PM, David Major wrote:
Isn't it ultimately up to the developer to get it right? Someone could just
as well forget to use |if (object)| from your example.
Here's a sample usage from the header file:
* // Test a weak pointer for validity before using it.
* if (weak) {
* weak-num = 17;
* weak-act();
* }
Sure, but that convenience operator makes it natural to write
incorrect code by default. It's better to be explicit and correct, than
implicit and wrong. :-)
Cheers,
Ehsan
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 3:54:17 PM
Subject: Audit your code if you use mozilla::WeakPtr
I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
usage unsafe by default. The issue is that WeakPtr provides convenience
methods such as operator-, which mean that the consumer can directly
dereference it without the required null checking first. This means that
you can have code like the below:
WeakPtrClass foo = realObject-asWeakPtr();
// ...
foo-Method();
That will happily compile and will crash at runtime if the object behind
the weak pointer is dead. The correct way of writing such code is:
Class* object = foo.get();
if (object) {
object-Method();
}
I don't know enough about all of the places which use WeakPtr myself to fix
them all, but if you have code using this in your module, please spend some
time auditing the code, and fix it. Please file individual bugs for your
components blocking bug 924658.
Thanks!
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Audit your code if you use mozilla::WeakPtr
It would appear to me that you really need to promote this to a strong
reference while you're using it.
Even `if (foo) foo-bar();` could theoretically have the value of `foo` change
between the branch and the deref.
Also, do we at least `MOZ_ASSERT(get())` in `operator-`? Yes, it will crash
immediately anyways, but it's always nicer to have something say 'Hey, you
tried to deref a null weak-ptr'.
-Jeff
- Original Message -
From: David Major dma...@mozilla.com
To: Ehsan Akhgari ehsan.akhg...@gmail.com
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:38:24 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
Does it? I don't think I'm any more or less likely to omit a validity check
using operator-() vs get(). Maybe it's just me.
It seems like get() might actually be *more* prone to failure. Imagine:
Class* object = foo.get();
if (object) {
object-Method();
}
// ... A lot of stuff happens and the ref blows up ...
if (object) {
object-Method(); // oops
}
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: David Major dma...@mozilla.com
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:27:03 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:10 PM, David Major wrote:
Isn't it ultimately up to the developer to get it right? Someone could just
as well forget to use |if (object)| from your example.
Here's a sample usage from the header file:
* // Test a weak pointer for validity before using it.
* if (weak) {
* weak-num = 17;
* weak-act();
* }
Sure, but that convenience operator makes it natural to write
incorrect code by default. It's better to be explicit and correct, than
implicit and wrong. :-)
Cheers,
Ehsan
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 3:54:17 PM
Subject: Audit your code if you use mozilla::WeakPtr
I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
usage unsafe by default. The issue is that WeakPtr provides convenience
methods such as operator-, which mean that the consumer can directly
dereference it without the required null checking first. This means that
you can have code like the below:
WeakPtrClass foo = realObject-asWeakPtr();
// ...
foo-Method();
That will happily compile and will crash at runtime if the object behind
the weak pointer is dead. The correct way of writing such code is:
Class* object = foo.get();
if (object) {
object-Method();
}
I don't know enough about all of the places which use WeakPtr myself to fix
them all, but if you have code using this in your module, please spend some
time auditing the code, and fix it. Please file individual bugs for your
components blocking bug 924658.
Thanks!
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:38 PM, David Major wrote:
Does it? I don't think I'm any more or less likely to omit a validity check using
operator-() vs get(). Maybe it's just me.
It seems like get() might actually be *more* prone to failure. Imagine:
Class* object = foo.get();
if (object) {
object-Method();
}
// ... A lot of stuff happens and the ref blows up ...
if (object) {
object-Method(); // oops
}
This code pattern is another problem -- you need to keep object alive if
you want to do this some other way. We have an idiom for this kind of
thing in fact (grep for kungFuDeathGrip!)
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: David Major dma...@mozilla.com
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:27:03 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:10 PM, David Major wrote:
Isn't it ultimately up to the developer to get it right? Someone could just as
well forget to use |if (object)| from your example.
Here's a sample usage from the header file:
* // Test a weak pointer for validity before using it.
* if (weak) {
* weak-num = 17;
* weak-act();
* }
Sure, but that convenience operator makes it natural to write
incorrect code by default. It's better to be explicit and correct, than
implicit and wrong. :-)
Cheers,
Ehsan
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 3:54:17 PM
Subject: Audit your code if you use mozilla::WeakPtr
I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
usage unsafe by default. The issue is that WeakPtr provides convenience
methods such as operator-, which mean that the consumer can directly
dereference it without the required null checking first. This means that
you can have code like the below:
WeakPtrClass foo = realObject-asWeakPtr();
// ...
foo-Method();
That will happily compile and will crash at runtime if the object behind
the weak pointer is dead. The correct way of writing such code is:
Class* object = foo.get();
if (object) {
object-Method();
}
I don't know enough about all of the places which use WeakPtr myself to fix
them all, but if you have code using this in your module, please spend some
time auditing the code, and fix it. Please file individual bugs for your
components blocking bug 924658.
Thanks!
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:48 PM, Jeff Gilbert wrote:
It would appear to me that you really need to promote this to a strong
reference while you're using it.
Yes, sure, if your object is refcounted somehow. Not all objects are
though, so strong pointer is a semantic in those cases.
Note that the idea here is to stop hiding the fact that you're accessing
a raw pointer behind the current WeakPtr syntactic sugar.
Even `if (foo) foo-bar();` could theoretically have the value of `foo` change
between the branch and the deref.
How? Remember that foo is a local variable.
(Note that WeakPtr is unsuitable for multi-threaded use cases, that is
bug 923554.)
Also, do we at least `MOZ_ASSERT(get())` in `operator-`? Yes, it will crash
immediately anyways, but it's always nicer to have something say 'Hey, you tried
to deref a null weak-ptr'.
My proposal is to remove those operators altogether, so this will be moot.
Cheers,
Ehsan
-Jeff
- Original Message -
From: David Major dma...@mozilla.com
To: Ehsan Akhgari ehsan.akhg...@gmail.com
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:38:24 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
Does it? I don't think I'm any more or less likely to omit a validity check using
operator-() vs get(). Maybe it's just me.
It seems like get() might actually be *more* prone to failure. Imagine:
Class* object = foo.get();
if (object) {
object-Method();
}
// ... A lot of stuff happens and the ref blows up ...
if (object) {
object-Method(); // oops
}
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: David Major dma...@mozilla.com
Cc: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 4:27:03 PM
Subject: Re: Audit your code if you use mozilla::WeakPtr
On 2013-10-08 7:10 PM, David Major wrote:
Isn't it ultimately up to the developer to get it right? Someone could just as
well forget to use |if (object)| from your example.
Here's a sample usage from the header file:
* // Test a weak pointer for validity before using it.
* if (weak) {
* weak-num = 17;
* weak-act();
* }
Sure, but that convenience operator makes it natural to write
incorrect code by default. It's better to be explicit and correct, than
implicit and wrong. :-)
Cheers,
Ehsan
- Original Message -
From: Ehsan Akhgari ehsan.akhg...@gmail.com
To: dev-platform@lists.mozilla.org
Sent: Tuesday, October 8, 2013 3:54:17 PM
Subject: Audit your code if you use mozilla::WeakPtr
I and Benoit Jacob discovered a bug in WeakPtr (bug 924658) which makes its
usage unsafe by default. The issue is that WeakPtr provides convenience
methods such as operator-, which mean that the consumer can directly
dereference it without the required null checking first. This means that
you can have code like the below:
WeakPtrClass foo = realObject-asWeakPtr();
// ...
foo-Method();
That will happily compile and will crash at runtime if the object behind
the weak pointer is dead. The correct way of writing such code is:
Class* object = foo.get();
if (object) {
object-Method();
}
I don't know enough about all of the places which use WeakPtr myself to fix
them all, but if you have code using this in your module, please spend some
time auditing the code, and fix it. Please file individual bugs for your
components blocking bug 924658.
Thanks!
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Audit your code if you use mozilla::WeakPtr
On 10/8/13 6:04 PM, Ehsan Akhgari wrote: This code pattern is another problem -- you need to keep object alive if you want to do this some other way. We have an idiom for this kind of thing in fact (grep for kungFuDeathGrip!) Would that be a kungFuDeathGrep? (Sorry. Just making weak pun.) Justin ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
