Closed Bug 802029 Opened 7 years ago Closed 7 years ago

The SENSOR_UNKNOWN passed to GetSensorObservers() Will Crash System

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: mchen, Assigned: alan.yenlin.huang)

Details

Attachments

(1 file, 5 obsolete files)

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.
Assignee: nobody → mchen
Assignee: mchen → ahuang
Attached patch Proposed patch to fix this bug (obsolete) — Splinter Review
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.
Attachment #672662 - Flags: review?(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.
Attachment #672662 - Flags: review?(mwu)
When PollSensors() found one of its event type is SENSOR_UNKNOWN, don't create a SensorRunnable.
Attachment #672662 - Attachment is obsolete: true
Attachment #673756 - Flags: review?(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.
Attachment #673756 - Flags: review?(mwu)
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.
Attachment #673756 - Attachment is obsolete: true
Attachment #674573 - Flags: review?(mwu)
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.
Attachment #674573 - Attachment is obsolete: true
Attachment #674573 - Flags: review?(mwu)
Attachment #675491 - Flags: review?(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.
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.
Attachment #675491 - Attachment is obsolete: true
Attachment #675491 - Flags: review?(mwu)
Attachment #676053 - Flags: review?(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;
Attachment #676053 - Flags: review?(mwu) → review+
Status: NEW → RESOLVED
blocking-basecamp: --- → ?
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
According to reviewer's advice, invert checking condition to previous review+ patch.
Attachment #676053 - Attachment is obsolete: true
blocking-basecamp: ? → +
Keywords: checkin-needed
Whiteboard: needs-checkin-aurora
https://hg.mozilla.org/mozilla-central/rev/e8f94aee02aa
Status: NEW → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: needs-checkin-aurora
You need to log in before you can comment on or make changes to this bug.