Last Comment Bug 710975 - Possible bad index checking in nsIEProfileMigrator::TestForIE7()
: Possible bad index checking in nsIEProfileMigrator::TestForIE7()
Status: RESOLVED FIXED
[pvs-studio]
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: Firefox 12
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
Depends on:
Blocks: 710966 399206
  Show dependency treegraph
 
Reported: 2011-12-14 22:52 PST by Justin Dolske [:Dolske]
Modified: 2012-01-08 06:22 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.33 KB, patch)
2011-12-15 01:32 PST, Tim Taubert [:ttaubert]
gavin.sharp: review+
Details | Diff | Splinter Review
patch v2 (1.59 KB, patch)
2012-01-06 04:01 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
Patch for bug 710975 v3 (2.43 KB, patch)
2012-01-07 21:20 PST, Jared Wein [:jaws] (please needinfo? me)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2011-12-14 22:52:58 PST
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.
Comment 1 Tim Taubert [:ttaubert] 2011-12-15 01:32:55 PST
Created attachment 581915 [details] [diff] [review]
patch v1

Trivial patch.
Comment 2 Tim Taubert [:ttaubert] 2011-12-15 01:33:54 PST
This was introduced in bug 399206.
Comment 3 Marco Bonardo [::mak] 2011-12-15 03:36:17 PST
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 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-15 11:30:50 PST
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.
Comment 5 Marco Bonardo [::mak] 2011-12-15 11:35:34 PST
(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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-16 12:06:41 PST
indeed!
Comment 7 Tim Taubert [:ttaubert] 2011-12-21 05:12:41 PST
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?
Comment 8 Marco Bonardo [::mak] 2011-12-21 06:05:41 PST
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.
Comment 9 Tim Taubert [:ttaubert] 2012-01-06 04:01:47 PST
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!
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-01-06 04:33:20 PST
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.
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-01-07 21:20:20 PST
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
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2012-01-08 03:09:19 PST
https://hg.mozilla.org/integration/fx-team/rev/371c513e15d7
Comment 13 Tim Taubert [:ttaubert] 2012-01-08 06:22:06 PST
Thanks, Jared!

https://hg.mozilla.org/mozilla-central/rev/371c513e15d7

Note You need to log in before you can comment on or make changes to this bug.