802029 - The SENSOR_UNKNOWN passed to GetSensorObservers() Will Crash System

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

If a new device supports a new sensor type then SENSOR_UNKNOWN (-1) will be passed to NotifySensorChange(). And this function call GetSensorObservers() to get SensorObserverList. Finally the wrong use of gSensorObservers[-1] will crash system.

Comment 1

[Alan Huang]

Attachment: Proposed patch to fix this bug

When sensor type is SENSOR_UNKNOWN, proposed patch just return and won't notify sensor change.

There's a mapping between Android and B2G's sensor type. Gonk set sensor type to SENSOR_UNKNOWN if it's not in the table. But system will crash (array index out of bound) when passing SENSOR_UNKNOWN, which is -1, to NotifySensorChange.

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 672662 [details] [diff] [review]
Proposed patch to fix this bug

Review of attachment 672662 [details] [diff] [review]:
-----------------------------------------------------------------

So I think a better approach for this would be to:

1. Have PollSensors determine what sensor_t * is associated with the event before creating a sensor runnable and pass that to SensorRunnable instead of an array of sensor_t.
2. Have PollSensors call HardwareSensorToHalSensor on the android sensor type (obtained from sensor_t *) and not dispatch any sensor events which are SENSOR_UNKNOWN.

This way we never have to create a SensorRunnable for events we don't know how to handle. It'll also generalize the check we have in PollSensors for ignoring SENSOR_TYPE_MAGNETIC_FIELD.

Comment 3

[Alan Huang]

Attachment: Don't create a SensorRunnable for SENSOR_UNKNOWN type.

When PollSensors() found one of its event type is SENSOR_UNKNOWN, don't create a SensorRunnable.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 673756 [details] [diff] [review]
Don't create a SensorRunnable for SENSOR_UNKNOWN type.

Review of attachment 673756 [details] [diff] [review]:
-----------------------------------------------------------------

This is the right approach, but we should move the fallback used in the SensorRunnable constructor to this code. The Android emulator's sensor hal doesn't set buffer[i].type so every sensor on the emulator would be ignored if we used this. Alternately, you can just use the sensors array to look up the type instead of using the type in the buffer array.

Comment 5

[Alan Huang]

Attachment: Don't create a SensorRunnable for SENSOR_UNKNOWN type.

Thanks to reviewer Michael.

Considering Android emulator's sensor hal doesn't set buffer[i].type, we use sensors array to look up the type as in the SensorRunnable constructor.

Comment 6

[Alan Huang]

Attachment: Don't create a SensorRunnable for SENSOR_UNKNOWN type.

After discuss with Marco, this patch keep Bug 802004's fix. Also, we move fixing broken emulator code in SensorRunnable constructor to PollSensors and add assign correct type to buffer array.

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 675491 [details] [diff] [review]
Don't create a SensorRunnable for SENSOR_UNKNOWN type.

Review of attachment 675491 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/gonk/GonkSensor.cpp
@@ +180,5 @@
>        if (buffer[i].type == SENSOR_TYPE_MAGNETIC_FIELD)
>          continue;
> +      if (HardwareSensorToHalSensor(buffer[i].type) == SENSOR_UNKNOWN) {
> +        // Emulator is broken and gives us events without types set
> +        if (buffer[i].sensor < size) {

We should just skip this event if buffer[i].sensor isn't in a valid range. A LOGW warning would also be appropriate.

Comment 8

[Alan Huang]

Attachment: Don't create a SensorRunnable for SENSOR_UNKNOWN type.

1. Considering Android emulator's sensor hal doesn't set buffer[i].type, we use sensors array to look up the type as in the SensorRunnable constructor.

2. This patch keep Bug 802004's fix. Also, we move fixing broken emulator code in SensorRunnable constructor to PollSensors and add assign correct type to buffer array.

3. If buffer[i].sensor isn't in a valid range, just skip it. A LOGW warning also added.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 676053 [details] [diff] [review]
Don't create a SensorRunnable for SENSOR_UNKNOWN type.

Review of attachment 676053 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the code simplified.

::: hal/gonk/GonkSensor.cpp
@@ +181,5 @@
>        if (buffer[i].type == SENSOR_TYPE_MAGNETIC_FIELD)
>          continue;
> +      if (HardwareSensorToHalSensor(buffer[i].type) == SENSOR_UNKNOWN) {
> +        // Emulator is broken and gives us events without types set
> +        if (buffer[i].sensor < size) {

Invert this check so we can eliminate an indentation level.

if (buffer[i].sensor >= size) {
  ...
  continue;
}

if (Hardware... == SENSOR_UNKNOWN) {
  continue;
}

buffer[i].type = sensors[buffer[i].sensor].type;

Comment 10

[Alan Huang]

Attachment: Don't create a SensorRunnable for SENSOR_UNKNOWN type.

According to reviewer's advice, invert checking condition to previous review+ patch.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8f94aee02aa

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e8f94aee02aa

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a62e0864b5d