Example 8. Incorrect type for storing the index
PRUint32 index = ieVersion.FindChar('.', 0);
if (index < 0)
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.
Created attachment 581915 [details] [diff] [review]
This was introduced in bug 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]
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.
(In reply to Gavin Sharp (use email@example.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.
Maybe adding a small comment about what the first code fragment is trying to do would be nice for future reference.
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]
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!
According to these MXR searches, kNotFound is defined in "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