463039 - validate values that get passed to clearWatch

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

We should ensure that the value passed to clearWatch is something that can be in the array.  Test for > 0 and < Max

Comment 1

[Doug Turner (:dougt)]

i am also going to convert our code from using unsigned shorts to longs to make the current specification (which uses signed ints).  This may change, but until it does we should do the right thing.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 346483 [details] [diff] [review]
patch v.1

nsGeolocation::ClearWatch(PRInt32 aWatchId):

+  PRUint32 count = mWatchingCallbacks.Length();
+
+  if (aWatchId < 0 || count == 0 || aWatchId > count + 1)
+    return NS_ERROR_FAILURE;

That should be aWatchId > count, not count + 1.

r+sr=jst with that.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346483 [details] [diff] [review]
patch v.1

a=beltzner since it comes in a bundle with the blockers

Comment 5

[Doug Turner (:dougt)]

pushed a1364547a182.

Comment 6

[Daniel Holbert [:dholbert]]

This patch added this compile warning:
mozilla/dom/src/geolocation/nsGeolocation.cpp:720: warning: comparison between signed and unsigned integer expressions

The responsible comparison is  "aWatchId > count", since aWatchId is signed and count is signed.

We should be fine to avoid this warning by casting aWatchId to unsigned, because if we actually hit this comparison, that implies we've already failed the "if (aWatchId < 0)" comparison, and so aWatchId is non-negative.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: followup patch to fix build warning

Comment 8

[timeless]

Comment on attachment 375752 [details] [diff] [review]
followup patch to fix build warning

r=me. i wrote the same patch and was going to file a bug.