Fix bug found by /analyze: incorrect return checking.

Fix bug found by /analyze: incorrect return checking.

SHGetPathFromIDList was being treated as if it returned an HRESULT when
it actually returns a BOOL.

Warning was:

src\chrome\utility\importer\ie_importer_win.cc
(200) : warning C6215: Cast between semantically different integer types:  a Boolean type to HRESULT.

R=isherman@chromium.org

BUG=427616
Committed: https://crrev.com/5fa5ca33ec2f2e71df5499fe91e4d20bc72ff805
Cr-Commit-Position: refs/heads/master@{#302452}

[brucedawson, 2014-10-31 20:50:37]

brucedawson@chromium.org changed reviewers:
+ isherman@chromium.org
- gab@chromium.org

[brucedawson, 2014-10-31 20:51:07]

Changed reviewer since gab is on leave.

[Ilya Sherman, 2014-10-31 22:15:55]

This seems ok to me, but I'm not familiar enough with Windows APIs to be
comfortable approving.  Please first get approval from someone who knows the
Windows APIs well.  I'm then happy to provide an OWNERS stamp if need be.

[brucedawson, 2014-10-31 22:31:42]

On 2014/10/31 22:15:55, Ilya Sherman wrote:
> This seems ok to me, but I'm not familiar enough with Windows APIs to be
> comfortable approving.  Please first get approval from someone who knows the
> Windows APIs well.  I'm then happy to provide an OWNERS stamp if need be.

Here's what I know. Here is the definition of FAILED:

#define FAILED(hr) (((HRESULT)(hr)) < 0)

HRESULT and BOOL are both defined as signed 32-bit values (HRESULT is a long,
BOOL is an int, but no matter).

Microsoft tends to abuse BOOL and use it for peculiar values but the
documentation for SHGetPathFromIDList says that they return TRUE or FALSE, so 1
or 0.

So...

The use of FAILED() means that any failures in the call to SHGetPathFromIDList
would be ignored. Both TRUE and FALSE would be treated as success. Those
failures will now be handled.

I also asked a couple of other Windows programmers if one of them could comment.

[Ilya Sherman, 2014-10-31 22:37:21]

On 2014/10/31 22:31:42, brucedawson wrote:
> On 2014/10/31 22:15:55, Ilya Sherman wrote:
> > This seems ok to me, but I'm not familiar enough with Windows APIs to be
> > comfortable approving.  Please first get approval from someone who knows the
> > Windows APIs well.  I'm then happy to provide an OWNERS stamp if need be.
> 
> Here's what I know. Here is the definition of FAILED:
> 
> #define FAILED(hr) (((HRESULT)(hr)) < 0)
> 
> HRESULT and BOOL are both defined as signed 32-bit values (HRESULT is a long,
> BOOL is an int, but no matter).
> 
> Microsoft tends to abuse BOOL and use it for peculiar values but the
> documentation for SHGetPathFromIDList says that they return TRUE or FALSE, so
1
> or 0.
> 
> So...
> 
> The use of FAILED() means that any failures in the call to SHGetPathFromIDList
> would be ignored. Both TRUE and FALSE would be treated as success. Those
> failures will now be handled.
> 
> I also asked a couple of other Windows programmers if one of them could
comment.

I'm more uncertain about the null-termination change than the HRESULT/BOOL
change.  If the data wasn't initially null-terminated, is it really safe to
overwrite the last character as a null terminator?  That seems surprising to me,
but I don't know the API very well -- it's surprising to me that we could get
non-null-terminated data back without any indication of an error in the first
place.

[scottmg, 2014-10-31 22:40:53]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2014-10-31 22:40:53]

lgtm

https://codereview.chromium.org/695553003/diff/1/chrome/utility/importer/ie_i...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/695553003/diff/1/chrome/utility/importer/ie_i...
chrome/utility/importer/ie_importer_win.cc:200: if (!idlist ||
!SHGetPathFromIDList(idlist, item_filename))
Yup, that just looks wrong.

https://codereview.chromium.org/695553003/diff/1/chrome/utility/importer/ie_i...
chrome/utility/importer/ie_importer_win.cc:880: buffer[arraysize(buffer) - 1] =
0;
the docs
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724911(v=vs.85).aspx
appear to imply that it may not always be null terminated? might be better to
change RegKey::ReadValue instead then.

[Ilya Sherman, 2014-10-31 22:45:06]

https://codereview.chromium.org/695553003/diff/1/chrome/utility/importer/ie_i...
File chrome/utility/importer/ie_importer_win.cc (right):

https://codereview.chromium.org/695553003/diff/1/chrome/utility/importer/ie_i...
chrome/utility/importer/ie_importer_win.cc:880: buffer[arraysize(buffer) - 1] =
0;
On 2014/10/31 22:40:53, scottmg wrote:
> the docs
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms724911(v=vs.85).aspx
> appear to imply that it may not always be null terminated? might be better to
> change RegKey::ReadValue instead then.

Agreed, using a function that guarantees a null-terminated result seems safer.

[brucedawson, 2014-10-31 23:50:16]

I changed the patch to just fix the return type issue. I'll look into a more
comprehensive fix for a lack of null-termination in RegKey::ReadValue for a
future patch.

[Ilya Sherman, 2014-10-31 23:51:21]

Thanks.  LGTM.

[cpu_(ooo_6.6-7.5), 2014-11-03 17:20:18]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2014-11-03 17:21:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/695553003/20001

[commit-bot: I haz the power, 2014-11-03 18:47:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-03 18:48:51]

Patchset 2 (id:??) landed as
https://crrev.com/5fa5ca33ec2f2e71df5499fe91e4d20bc72ff805
Cr-Commit-Position: refs/heads/master@{#302452}