Handle invalid CFStrings in GetHFSDecomposedForm()

Handle invalid CFStrings in GetHFSDecomposedForm().
Error (caused by invalid UTF-8 sequence) might occur during the creation
of CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to
crash in CFStringGetMaximumSizeOfFileSystemRepresentation(..) function.
This CL checks if the creation of CFString is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly.

BUG=558847
Committed: https://crrev.com/03478a11686beb7e5aaed214d2791ef917b6b8aa
Cr-Commit-Position: refs/heads/master@{#384343}

[Jialiu Lin, 2016-03-28 18:20:05]

Description was changed from

==========
Check CFString allocation before proceed

BUG=558847
==========

to

==========
Error might occur during the creation of CFString (i.e.
CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. Need to check if
the creation is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation.

BUG=558847
==========

[Jialiu Lin, 2016-03-28 18:20:49]

jialiul@chromium.org changed reviewers:
+ rsesek@chromium.org

[Jialiu Lin, 2016-03-28 18:22:45]

Hi Robert, 
This is my best guess of what causes the crash in
base::FilePath::GetHFSDecomposedForm. Let me know if this makes sense.

[Robert Sesek, 2016-03-28 18:41:48]

I think your analysis is correct, so yes this makes sense. Could you write a
unittest (pre-patch) to see if you can reproduce the issue? Try creating an
invalid sequence of bytes and passing that to the API; attempting to convert
that to UTF-8 should trigger the crash.

How does this affect the higher-level API that's using this (in safe_browsing)?

Also, the "Subject" of a Rietveld issue doesn't make it into the commit
description, so please copy the subject into the Description field as the first
line. It's also nice to keep descriptions hard-wrapped to 80 columns.

[Jialiu Lin, 2016-03-28 18:44:04]

Description was changed from

==========
Error might occur during the creation of CFString (i.e.
CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. Need to check if
the creation is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation.

BUG=558847
==========

to

==========
Check if CFString created successfully before 
CFStringGetMaximumSizeOfFileSystemRepresentation
Error might occur during the creation of CFString (i.e.
CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. Need to check if
the
creation is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation.

BUG=558847
==========

[Jialiu Lin, 2016-03-30 01:09:12]

On 2016/03/28 at 18:41:48, rsesek wrote:
> I think your analysis is correct, so yes this makes sense. Could you write a
unittest (pre-patch) to see if you can reproduce the issue? Try creating an
invalid sequence of bytes and passing that to the API; attempting to convert
that to UTF-8 should trigger the crash.
> 
> How does this affect the higher-level API that's using this (in
safe_browsing)?
> 
> Also, the "Subject" of a Rietveld issue doesn't make it into the commit
description, so please copy the subject into the Description field as the first
line. It's also nice to keep descriptions hard-wrapped to 80 columns.

Tested on pre-patch code with invalid UTF-8 sequence, successfully reproduced
this crash. In this CL, also added a separate unit test to show now invalid
UTF-8 sequence will be handled by returning an empty string.
Given this crash happens quite rare, I don't think it will have significant
impact on safe browsing detection. 
CL description updated accordingly. 

Thanks!

[Jialiu Lin, 2016-03-30 01:11:26]

Description was changed from

==========
Check if CFString created successfully before 
CFStringGetMaximumSizeOfFileSystemRepresentation
Error might occur during the creation of CFString (i.e.
CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. Need to check if
the
creation is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation.

BUG=558847
==========

to

==========
Check if CFString created successfully before 
CFStringGetMaximumSizeOfFileSystemRepresentation
Error (caused by invalid UTF-8 sequence) might occur during the creation of
CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. 
This CL checks if the creation is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly. 

BUG=558847
==========

[Jialiu Lin, 2016-03-30 01:12:04]

Description was changed from

==========
Check if CFString created successfully before 
CFStringGetMaximumSizeOfFileSystemRepresentation
Error (caused by invalid UTF-8 sequence) might occur during the creation of
CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. 
This CL checks if the creation is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly. 

BUG=558847
==========

to

==========
Check if CFString created successfully before 
CFStringGetMaximumSizeOfFileSystemRepresentation
Error (caused by invalid UTF-8 sequence) might occur during the creation of
CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. 
This CL checks if the creation of CFString is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly. 

BUG=558847
==========

[Jialiu Lin, 2016-03-30 17:40:29]

jialiul@chromium.org changed reviewers:
+ thestig@chromium.org

[Jialiu Lin, 2016-03-30 17:40:30]

+thestig@, could you take a look at this CL that aims for solving the following
crash: 

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%27%20AND%...

Thanks!

[Robert Sesek, 2016-03-30 21:03:26]

LGTM

[Lei Zhang, 2016-03-30 21:53:08]

lgtm

https://codereview.chromium.org/1836923002/diff/50003/base/files/file_path_un...
File base/files/file_path_unittest.cc (right):

https://codereview.chromium.org/1836923002/diff/50003/base/files/file_path_un...
base/files/file_path_unittest.cc:1302: const struct UnaryBooleanTestData cases[]
= {
You are not using UnaryBooleanTestData's |expected| member at all. Just make
this a const FilePath::CharType* const array?

https://codereview.chromium.org/1836923002/diff/50003/base/files/file_path_un...
base/files/file_path_unittest.cc:1309: for (size_t i = 0; i < arraysize(cases);
i++) {
You can probably slightly simplify this to:

for (const auto& test_case : cases) {
  FilePath::StringType observed = FilePath::GetHFSDecomposedForm(test_case);
  EXPECT_TRUE(observed.empty());
}

[Lei Zhang, 2016-03-30 21:59:57]

I'd also suggest a more git friendly commit message. e.g.

Handle invalid CFStrings in GetHFSDecomposedForm()

Full CL details here. Stick with 72 chars / line.
...

BUG=558847

[Jialiu Lin, 2016-03-30 23:43:13]

Description was changed from

==========
Check if CFString created successfully before 
CFStringGetMaximumSizeOfFileSystemRepresentation
Error (caused by invalid UTF-8 sequence) might occur during the creation of
CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to crash in
CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. 
This CL checks if the creation of CFString is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly. 

BUG=558847
==========

to

==========
Handle invalid CFStrings in GetHFSDecomposedForm().
Error (caused by invalid UTF-8 sequence) might occur during the creation
of CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to
crash in CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. 
This CL checks if the creation of CFString is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly. 

BUG=558847
==========

[Jialiu Lin, 2016-03-31 00:44:51]

Thanks, thestig@! your comments are all addressed.

https://codereview.chromium.org/1836923002/diff/50003/base/files/file_path_un...
File base/files/file_path_unittest.cc (right):

https://codereview.chromium.org/1836923002/diff/50003/base/files/file_path_un...
base/files/file_path_unittest.cc:1302: const struct UnaryBooleanTestData cases[]
= {
On 2016/03/30 at 21:53:08, Lei Zhang wrote:
> You are not using UnaryBooleanTestData's |expected| member at all. Just make
this a const FilePath::CharType* const array?

Done

https://codereview.chromium.org/1836923002/diff/50003/base/files/file_path_un...
base/files/file_path_unittest.cc:1309: for (size_t i = 0; i < arraysize(cases);
i++) {
On 2016/03/30 at 21:53:08, Lei Zhang wrote:
> You can probably slightly simplify this to:
> 
> for (const auto& test_case : cases) {
>   FilePath::StringType observed = FilePath::GetHFSDecomposedForm(test_case);
>   EXPECT_TRUE(observed.empty());
> }
Done

[Jialiu Lin, 2016-03-31 18:31:51]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-03-31 18:31:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1836923002/#ps70001
(title: "fix test")

[commit-bot: I haz the power, 2016-03-31 18:32:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1836923002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1836923002/70001

[commit-bot: I haz the power, 2016-03-31 18:38:56]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-03-31 18:45:09]

Description was changed from

==========
Handle invalid CFStrings in GetHFSDecomposedForm().
Error (caused by invalid UTF-8 sequence) might occur during the creation
of CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to
crash in CFStringGetMaximumSizeOfFileSystemRepresentation(..) function. 
This CL checks if the creation of CFString is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly. 

BUG=558847
==========

to

==========
Handle invalid CFStrings in GetHFSDecomposedForm().
Error (caused by invalid UTF-8 sequence) might occur during the creation
of CFString (i.e.CFStringCreateWithBytesNoCopy(...)), which leads to
crash in CFStringGetMaximumSizeOfFileSystemRepresentation(..) function.
This CL checks if the creation of CFString is successful before calling
CFStringGetMaximumSizeOfFileSystemRepresentation to prevent crash.
A unit test is added to verify invalid sequences are handled correctly.

BUG=558847

Committed: https://crrev.com/03478a11686beb7e5aaed214d2791ef917b6b8aa
Cr-Commit-Position: refs/heads/master@{#384343}
==========

[commit-bot: I haz the power, 2016-03-31 18:45:12]

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