If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla19

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: nical)

Tracking

(Blocks: 1 bug)

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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).
(Reporter)

Comment 1

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

Comment 2

5 years ago
Created attachment 668602 [details] [diff] [review]
Warning fix

Indeed, numChildren should have been unsigned.
Assignee: nobody → nical.bugzilla
Attachment #668602 - Flags: review?(dholbert)
(Reporter)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
Comment on attachment 668602 [details] [diff] [review]
Warning fix

Great, thanks for clarifying that!

r=me
Attachment #668602 - Flags: review?(dholbert) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e7f060ac7a
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b0e7f060ac7a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.