1293327 - Make indexDB CreateStorageConnection get file protocol handler directly

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Honza Bambas (:mayhemer)]

Example, on "Storage I/O" thread:

 	xul.dll!nsWeakReference::QueryReferent({...}, 0x1e6fefac) Line 127	C++
 	xul.dll!CallQueryReferent<nsIWeakReference,nsIProtocolHandler>(0x0ec0aac0, 0x1e6fefac) Line 30	C++
>	xul.dll!mozilla::net::nsIOService::GetCachedProtocolHandler(0x09e1700c, 0x1e6fefac, 0, 0) Line 495	C++
 	xul.dll!mozilla::net::nsIOService::GetProtocolHandler(0x09e1700c, 0x1e6fefac) Line 511	C++
 	xul.dll!mozilla::net::nsIOService::NewFileURI(0x11ab8760, 0x1e6ff094) Line 654	C++
 	xul.dll!NS_NewFileURI(0x1e6ff094, 0x11ab8760, 0x01067420) Line 147	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::GetDatabaseFileURL(0x11ab8760, PERSISTENCE_TYPE_PERSISTENT, {...}, {...}, 0, 0x1e6ff40c) Line 4115	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::CreateStorageConnection(0x11ab8760, 0x1283b1c0, {...}, PERSISTENCE_TYPE_PERSISTENT, {...}, {...}, 0, 0x1e6ff5c4) Line 4415	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::OpenDatabaseOp::DoDatabaseWork() Line 20855	C++
 	xul.dll!mozilla::dom::indexedDB::`anonymous namespace'::FactoryOp::Run() Line 20612	C++
 	xul.dll!nsThread::ProcessNextEvent(false, 0x1e6ff7d1) Line 1058	C++
 	xul.dll!NS_ProcessNextEvent(0x1e2d2740, false) Line 290	C++

This is a result of run with the patch from bug 956338.

Solution: let the indexedDB create the URL on the main thread (not ideal too.. our URLs are not threadsafe as well..)

Other, more universal way is to have a really thread safe weak reference implementation that can be used here.  Still, nsIOService::mWeakHandler array would also have to be protected by a mutex.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1 (grab file protocol directly, not from IO service weak cache)

Problem here is that NS_NewFileUri goes to the IO service that inspects its internal weak array cache.  That array can only be access on the main thread, since 1) weak references are not thread safe 2) the array is not guarded.

Hence, getting the file protocol directly as a service is the correct thread safe way here.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8779340 [details] [diff] [review]
v1 (grab file protocol directly, not from IO service weak cache)

Jan, can you take this?

Comment 3

[Jan Varga [:janv]]

Comment on attachment 8779340 [details] [diff] [review]
v1 (grab file protocol directly, not from IO service weak cache)

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

Looks good, r=me

::: dom/indexedDB/ActorsParent.cpp
@@ +70,5 @@
>  #include "nsIAppsService.h"
>  #include "nsIEventTarget.h"
>  #include "nsIFile.h"
>  #include "nsIFileURL.h"
> +#include "nsIFileProtocolHandler.h"

Nit: should go before nsIFileURL.h

@@ +4120,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIFileProtocolHandler> fileHandler(
> +    do_QueryInterface(protocolHandler, &rv));

Nit: we seem to never check rv of do_QueryInterface, the check below can be just |if (NS_WARN_IF(!fileHandler)) ...|

Comment 4

[Andrew McCreight [:mccr8]]

This needs a security rating before it can be checked in. How bad of a security issue do you think this is in practice?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4d0d597bce

Comment 6

[Honza Bambas (:mayhemer)]

only real problem here is concurrent access on the weak cache array in IO service, nothing else.  that's probably not likely to be exploitable.

use-after-free is of a very low if even any chance at all, since file protocol handler is a service and lives as long as xpcom.  and the code we are fixing here is triggered just during startup (AFAIU).

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e4d0d597bce