Possible bad index checking in nsIEProfileMigrator::TestForIE7()

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Migration
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Dolske, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 12
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pvs-studio])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
From http://www.viva64.com/en/a/0078/

Example 8. Incorrect type for storing the index

PRBool 
nsIEProfileMigrator::TestForIE7()
{
  ...
  PRUint32 index = ieVersion.FindChar('.', 0);
  if (index < 0)
    return PR_FALSE;
  ...
}

PVS-Studio diagnostic message: V547 Expression 'index < 0' is always false. Unsigned type value is never < 0. nsieprofilemigrator.cpp 622

The function won't return PR_FALSE if there is no dot in the string and will continue handling incorrect data. The error here is that an unsigned data type was used for the 'index' variable. Checking that (index < 0) is meaningless.
(Reporter)

Updated

6 years ago
Blocks: 710966
Created attachment 581915 [details] [diff] [review]
patch v1

Trivial patch.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #581915 - Flags: review?(gavin.sharp)
This was introduced in bug 399206.
Blocks: 399206
another good reason to use the correct ==/!= kNotFound instead of random checks (>0, <0, !=whatever)

the "if (index > 0)" check looks strange, maybe not wrong, but since the complete lack of comments it's hard to even tell what the code tries to handle.
I suppose it's trying to support something like this:
"C:\Program Files\Internet Explorer\IEXPLORE.EXE" %1
So removing the first ", and then the last one and everything after it.
In such a case, I don't understand why it is not using RFindChar (and kNotFound)
Comment on attachment 581915 [details] [diff] [review]
patch v1

Using RFindChar for the first call would change the behavior if there are multiple quotes, so I don't think it's worth doing.

The second call should compare against kNotFound as Marco suggests.
Attachment #581915 - Flags: review?(gavin.sharp) → review+
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> Using RFindChar for the first call would change the behavior if there are
> multiple quotes, so I don't think it's worth doing.

oh, right!
Maybe adding a small comment about what the first code fragment is trying to do would be nice for future reference.
indeed!
With the first check being (index > 0) the string '""' would be returned as '"' - because FindChar() returns 0. That's not really a valid example that occurs but shouldn't we compare against kNotFound here as well?
The first check is probably trying to enforce that the string is not empty by using > 0, so if you have '"" %1 "somearg"', the cut will change it to '" %1 "somearg"' FindChar will return 0 and the if will be skipped (currently). Instead using != kNotFound the if would be executed and would build an empty string.
In both cases, the next call to NS_NewLocalFile will likely fail.
Created attachment 586381 [details] [diff] [review]
patch v2

This doesn't build on Windows because 'kNotFound' is not defined. Alas, I don't have a Windows build machine (I should get one) and I really don't want to waste try server cycles for finding the right header to include.

Can someone please take this, add the missing header and push it? Thanks!
Attachment #581915 - Attachment is obsolete: true
Assignee: ttaubert → nobody
According to these MXR searches, kNotFound is defined in "nsAString.h":
http://mxr.mozilla.org/mozilla-central/ident?i=kNotFound
http://mxr.mozilla.org/mozilla-central/search?string=nsAString.h

I don't have time to take this at the moment, but it looks like we will just need to add an include for nsAString.h to the CPP file.
Created attachment 586757 [details] [diff] [review]
Patch for bug 710975 v3

I tried including nsAString.h, but that file in not accessible from external-linkage code. Alas, I tried including nsStringAPI.h but that file does not include the #define for kNotFound. According to https://developer.mozilla.org/en/Migrating_from_Internal_Linkage_to_Frozen_Linkage#section_8, external-linkage code should add their own #define for kNotFound.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2849df408396
Assignee: nobody → jwein
Attachment #586381 - Attachment is obsolete: true
Attachment #586757 - Flags: review?(gavin.sharp)
Attachment #586757 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/integration/fx-team/rev/371c513e15d7
Whiteboard: [pvs-studio] → [pvs-studio][fixed-in-fx-team]
Thanks, Jared!

https://hg.mozilla.org/mozilla-central/rev/371c513e15d7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [pvs-studio][fixed-in-fx-team] → [pvs-studio]
Target Milestone: --- → Firefox 12
You need to log in before you can comment on or make changes to this bug.