Make base::ReadFileToString() return false on I/O error.

Make base::ReadFileToString() return false on I/O error.

It seems inconsistent that ReadFileToString() currently returns false when encountering a file open error but true upon file read error.  This CL forces the return value to false upon encountering any kind of error and documents this behaviour.

At that occasion, a test for path traversal prevention is added.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267507

[Thiemo Nagel, 2014-04-30 14:30:30]

Hi Will,

may I kindly ask you to take a look at this CL?  In case you agree with the CL,
would you consider it useful/necessary to send a heads-up to chromium-dev@ since
technically, the CL changes a (hitherto undocumented) part of the
ReadFileToString() interface?

Thank you and best regards,
Thiemo

[willchan no longer on Chromium, 2014-04-30 15:14:39]

This change LGTM, mod my nit on the test assertion.

If you want to be safe, feel free to email chromium-dev@. But I don't consider
it necessary in this case.

Thanks for the cleanup, I think it's a good change.

https://codereview.chromium.org/254333002/diff/1/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

https://codereview.chromium.org/254333002/diff/1/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:2052: EXPECT_EQ(data.length(), 0u);
Ordering is (expected, actual), so 0u should go first. gTest uses this ordering
in its test output messages.

[Thiemo Nagel, 2014-04-30 16:11:32]

Thank you!  I won't email to chromium-dev if you don't consider it necessary.

https://codereview.chromium.org/254333002/diff/1/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

https://codereview.chromium.org/254333002/diff/1/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:2052: EXPECT_EQ(data.length(), 0u);
On 2014/04/30 15:14:40, willchan wrote:
> Ordering is (expected, actual), so 0u should go first. gTest uses this
ordering
> in its test output messages.

Thank you!  I was tricked into this by the surrounding code...

[Thiemo Nagel, 2014-04-30 16:57:28]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-04-30 16:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tnagel@chromium.org/254333002/20001

[commit-bot: I haz the power, 2014-04-30 17:03:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:03:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Thiemo Nagel, 2014-05-01 08:23:29]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-05-01 08:23:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tnagel@chromium.org/254333002/20001

[commit-bot: I haz the power, 2014-05-01 10:40:56]

Change committed as 267507