858034 - navigator.getDeviceStorage('') crashes on platforms device storage isn't supported on

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Opening the web console and writing the following crashes the browser:

navigator.getDeviceStorage('')

It doesn't matter what parameter is passed to the method as long as there is one.

Comment 1

[Benjamin Smedberg]

What is your crash report ID (from about:crashes)?

Comment 2

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> What is your crash report ID (from about:crashes)?


4B708448-1451-47A2-87B3-3A0272A12E9F

Comment 3

[Benjamin Smedberg]

Can you please submit your crash report (by clicking on it) and send me the report ID/URL of the crash on crash-stats?

Comment 4

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Sure, that's the same crash with a different ID:

https://crash-stats.mozilla.com/report/index/bp-698e1a88-3f10-4a07-910a-652c22130404

Comment 5

[Benjamin Smedberg]

Thanks. This is a null-pointer crash, so not exploitable.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Looks to me like device.storage.enabled being false leads to an automatic null-deref crash here, right?

Comment 7

[Aaron Train [:aaronmt]]

bp-d70df2f5-0fa3-4fbc-bbe9-4e02b2130404

Easy to reproduce in Fennec on m-c

Visit: http://mozqa.com/webapi-permissions-tests/, and install the 'Packaged App Test Case 1'

Comment 8

[Jason Smith [:jsmith]]

Here's a simple STR:

1. Go to http://jds2501.github.com/webapi-permissions-tests/webapi_permissions_test.html in the browser
2. Select any the device storage tests

Result - Firefox crashes.

Comment 9

[Jason Smith [:jsmith]]

Doug says apparently we can delegate this to Dave Hylands.

Comment 10

[Dave Hylands [:dhylands]]

I think that the problem is caused in Navigator.cpp

It currently has:

  if (!Preferences::GetBool("device.storage.enabled", false)) {
    return NS_OK;
  }

and should have:

  if (!Preferences::GetBool("device.storage.enabled", false)) {
    *_retval = nullptr;
    return NS_OK;
  }

Comment 11

[Boris Zbarsky [:bzbarsky]]

No.  The problem is that the caller checks the rv and bails if failure, then dereferences the pointer, which is null because the callee didn't set it to anything and it was already null.

As in, we either need to return a failure when disabled or we need to null-check in the caller.

Comment 12

[Dave Hylands [:dhylands]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> No.  The problem is that the caller checks the rv and bails if failure, then
> dereferences the pointer, which is null because the callee didn't set it to
> anything and it was already null.
> 
> As in, we either need to return a failure when disabled or we need to
> null-check in the caller.

Ahhh - right - I get it.

Yeah - it should return an error if its not going to return a proper object.

Comment 13

[Dave Hylands [:dhylands]]

Attachment: Make getDeviceStorage check if its enabled or not.

Add back the check in getDeviceStorage to see if its enabled or not.

Also make sure that the return value is always initialized.

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 733459 [details] [diff] [review]
Make getDeviceStorage check if its enabled or not.

I was able to reproduce on my unagi/FirefoxOS by disabling device storage.

Comment 15

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4170679cb85a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4170679cb85a

Comment 18

[Jason Smith [:jsmith]]

Dave - Can you nominate this for Aurora uplift? We have packaged apps on FxAndroid enabled on Fx22, so we are seeing this crash on Fx22 as well.

Comment 19

[Dave Hylands [:dhylands]]

This bug was caused by bug 838038, which as far as I can tell wasn't uplifted, so nothing else should be required.

Comment 20

[Dave Hylands [:dhylands]]

I tested:

    navigator.getDeviceStorage('');

on Firefox Aurora 22.0a2 (2013-04-08) and it seemed to work fine (i.e. returned null).