710975 - Possible bad index checking in nsIEProfileMigrator::TestForIE7()

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Justin Dolske [:Dolske]]

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] (inactive)]

Attachment: patch v1

Trivial patch.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

This was introduced in bug 399206.

Comment 3

[Marco Bonardo [:mak]]

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: [e-mail]]]

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]]

(In reply to Gavin Sharp (use [e-mail] 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: [e-mail]]]

indeed!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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]]

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] (inactive)]

Attachment: 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)]

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)]

Attachment: 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)]

https://hg.mozilla.org/integration/fx-team/rev/371c513e15d7

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Thanks, Jared!

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