1186264 - Uninitialized variable in GrallocTextureHostOGL::GetAsSurface causes memory-safety bug

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[q1]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Steps to reproduce:

GrallocTextureHostOGL::GetAsSurface (gfx\layers\opengl\GrallocTextureHost.cpp) attempts to allocate and lock a graphics buffer, but then uses a pointer to the locked buffer without checking for failure. Since it does not initialize the pointer, and the pointer is an automatic variable, it is uninitialized, unless android::GraphicBuffer::lock (which appears to be undocumented, along with the rest of android::GraphicBuffer) sets it to nullptr on failure.

Apparently the buffer is only read, so potential damage may be limited to leakage of sensitive information and/or a crash.

231:  TemporaryRef<gfx::DataSourceSurface>
232:  GrallocTextureHostOGL::GetAsSurface() {
233:    android::GraphicBuffer* graphicBuffer = GetGraphicBufferFromDesc(mGrallocHandle).get();
234:    uint8_t* grallocData;
235:    graphicBuffer->lock(GRALLOC_USAGE_SW_READ_OFTEN, reinterpret_cast<void**>(&grallocData));
236:    RefPtr<gfx::DataSourceSurface> grallocTempSurf =
237:      gfx::Factory::CreateWrappingDataSourceSurface(grallocData,
238:                                                    graphicBuffer->getStride() * android::bytesPerPixel(graphicBuffer->getPixelFormat()),
239:                                                    GetSize(), GetFormat());
240:    RefPtr<gfx::DataSourceSurface> surf = CreateDataSourceSurfaceByCloning(grallocTempSurf);
241: 
242:    graphicBuffer->unlock();
243:
244:    return surf.forget();
245:  }

Comment 1

[Sotaro Ikeda [:sotaro]]

Yea, error check to GrallocTextureHostOGL::GetAsSurface() is necessary. GrallocTextureHostOGL::GetAsSurface() is used when MOZ_DUMP_PAINTING is enabled.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add error check to GrallocTextureHostOGL::GetAsSurface()

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8640016 [details] [diff] [review]
patch - Add error check to GrallocTextureHostOGL::GetAsSurface()

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

As Sotaro said, this code is only used in debugging tools, so no need to flag the bug as security sensitive.

Comment 4

[Pulsebot]

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

Comment 5

[Carsten Book [:Tomcat]]

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