797893 - ImageBridgeParent.cpp:60:32: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning:
{
Warning: -Wsign-compare in /mozilla/gfx/layers/ipc/ImageBridgeParent.cpp: comparison between signed and unsigned integer expressions

/mozilla/gfx/layers/ipc/ImageBridgeParent.cpp:60:32: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
343.27 ShadowLayerChild.cpp
}

That's for this chunk of code:
> 59   int numChildren = ManagedPImageContainerParent().Length();
> 60   for (unsigned int i = 0; i < numChildren; ++i) {
> 61     static_cast<ImageContainerParent*>(
> 62       ManagedPImageContainerParent()[i]
> 63     )->DoStop();

I don't know if ManagedPImageContainerParent().Length(); can return a negative value, but if it can, we're in trouble...  If numChildren is negative, it will be converted to a (gigantic) unsigned value for the "i < numChildren" comparison, and we'll iterate up to that gigantic value, indexing into bad memory.

We could fix this potential issue (and the build warning) by making "i" an int instead of an unsigned int.  Or instead, maybe we can make numChildren unsigned? (I can't find the  ManagedPImageContainerParent() definition, so I don't know if it returns int or unsigned).

Comment 1

[Daniel Holbert [:dholbert]]

Sorry, for got MXR link:
https://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ImageBridgeParent.cpp?rev=cf396cb6d4af&mark=59-62#57

Comment 2

[Nicolas Silva [:nical]]

Attachment: Warning fix

Indeed, numChildren should have been unsigned.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 668602 [details] [diff] [review]
Warning fix

Thanks for looking into this!

Looks good to me -- one question, though: what type does ManagedPImageContainerParent() return, and is its Length() definitely unsigned? (an MXR link would be great)

I'd like to be more sure on that before r+'ing this.

Comment 4

[Nicolas Silva [:nical]]

Sure, ManagedPImageContainerParent is in the IPDL generated code for PImageBridge. You can find it in objdir/ipc/ipdl/_ipdlheaders/mozilla/layers/PImageBridgeParent.h and at line 162 we see that ManagedPImageContainerParent returns a InfallibleTArray<PImageContainerParent*>& that we can look up on dxr here:http://dxr.lanedo.com/mozilla-central/xpcom/glue/nsTArray.h.html?string=InfallibleTArray#l202 and the Length argument is an uint32_t.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 668602 [details] [diff] [review]
Warning fix

Great, thanks for clarifying that!

r=me

Comment 6

[Paul Adenot (:padenot)]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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