Hello andreip, nigeltao,
I'd like you to do a code review. Please execute
g4 diff -c 8972115
or point your web browser to
http://mondrian/8972115
(this changelist has been uploaded to Mondrian)
to review the following code:
Change 8972115 by [EMAIL PROTECTED] on 2008/11/12 16:59:08 *pending*
Fix comparison of Dispatcher::ImplCallback with NULL.
Dispatcher::ImplCallback is a templated pointer to member
function. On some platforms (e.g Android, which is ARM EABI),
these pointers are actually two words (64 bits), where the
second word indicates whether the first is a plain pointer
(static or non-virtual member), or an offset in the vtable
(virtual member).
The problem is that the first virtual function in a vtable, at
offset 0, is encoded as: 0x00000000, 0x00000001. Implicit cast
to bool results in false, and it compares as equal to NULL
(casting NULL to a pointer-to-vtable-offset results in the
same encoding). This means that RegisterProperty() fails if
the getter is a virtual function at offset 0.
It also means that SetProperty() will fail if the setter
function is at virtual offset 0, as NULL is used as a special
value to indicate read-only. (This does not trigger at the
moment, by luck).
This fix involves using a union to check the raw binary value
of the pointer. This does still assume that NULL will map to
all-zero, and vtable offset 0 will not, but this seems safe on
current platforms.
PRESUBMIT=passed
R=andreip,nigeltao
[EMAIL PROTECTED]
DELTA=33 (31 added, 0 deleted, 2 changed)
OCL=8972115
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/dispatcher.h#8 edit
33 delta lines: 31 added, 0 deleted, 2 changed
Also consider running:
g4 lint -c 8972115
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 8972115 by [EMAIL PROTECTED] on 2008/11/12 16:59:08 *pending*
Fix comparison of Dispatcher::ImplCallback with NULL.
Dispatcher::ImplCallback is a templated pointer to member
function. On some platforms (e.g Android, which is ARM EABI),
these pointers are actually two words (64 bits), where the
second word indicates whether the first is a plain pointer
(static or non-virtual member), or an offset in the vtable
(virtual member).
The problem is that the first virtual function in a vtable, at
offset 0, is encoded as: 0x00000000, 0x00000001. Implicit cast
to bool results in false, and it compares as equal to NULL
(casting NULL to a pointer-to-vtable-offset results in the
same encoding). This means that RegisterProperty() fails if
the getter is a virtual function at offset 0.
It also means that SetProperty() will fail if the setter
function is at virtual offset 0, as NULL is used as a special
value to indicate read-only. (This does not trigger at the
moment, by luck).
This fix involves using a union to check the raw binary value
of the pointer. This does still assume that NULL will map to
all-zero, and vtable offset 0 will not, but this seems safe on
current platforms.
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/dispatcher.h#8 edit
==== //depot/googleclient/gears/opensource/gears/base/common/dispatcher.h#8 -
/usr/local/google/home/jripley/gears-trunk3/googleclient/gears/opensource/gears/base/common/dispatcher.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/dispatcher.h
2008-11-03 11:32:08.000000000 +0000
+++ googleclient/gears/opensource/gears/base/common/dispatcher.h
2008-11-12 16:47:02.000000000 +0000
@@ -69,6 +69,35 @@
// Callback function used for property and method invocations.
typedef void (ImplClass::*ImplCallback)(JsCallContext *);
+ // This strange union comparison is required due to the way
+ // pointer-to-member is implemented on some
+ // architectures. Specifically, if the platform uses an extra word
+ // to indicate whether the pointer is a plain function or a vtable
+ // offset, then casting vtable offset 0 to bool results in
+ // false. This workaround assumes that a vtable offset uses a
+ // non-zero value in the second word. This method also works if the
+ // platform uses thunks and a single word for the pointer.
+ static bool IsImplCallbackValid(ImplCallback callback) {
+ const int words = (sizeof(ImplCallback) + sizeof(int) - 1) / sizeof(int);
+ union {
+ ImplCallback value;
+ int data[words];
+ } combined;
+ // Initialize the union to zero.
+ for (int i = 0; i < words; ++i) {
+ combined.data[i] = 0;
+ }
+ // Set the ImplCallback.
+ combined.value = callback;
+ // Check if the union is now non-zero.
+ for (int i = 0; i < words; ++i) {
+ if (combined.data[i] != 0) {
+ return true;
+ }
+ }
+ return false;
+ }
+
Dispatcher(ImplClass *impl);
virtual ~Dispatcher() {}
@@ -202,6 +231,7 @@
return false;
ImplCallback callback = method->second;
+ assert(IsImplCallbackValid(callback));
(impl_->*callback)(context);
return true;
}
@@ -215,6 +245,7 @@
return false;
ImplCallback callback = property->second;
+ assert(IsImplCallbackValid(callback));
(impl_->*callback)(context);
return true;
}
@@ -228,7 +259,7 @@
return false;
}
ImplCallback callback = property->second;
- if (callback == NULL) { // Read only property.
+ if (!IsImplCallbackValid(callback)) { // Read only property.
context->SetException(
STRING16(L"Cannot assign value to a read only property."));
return true;
@@ -258,7 +289,7 @@
template<class T>
void Dispatcher<T>::RegisterProperty(const char *name,
ImplCallback getter, ImplCallback setter)
{
- assert(getter);
+ assert(IsImplCallbackValid(getter));
DispatchId id = GetStringIdentifier(name);
GetPropertyGetterList()[id] = getter;
GetPropertySetterList()[id] = setter;
