Fixes an issue when the owner key is empty chrome would crash trying to access bogus memory.

Fixes an issue when the owner key is empty chrome would crash trying to access bogus memory.

BUG=128410
TEST=OwnerKeyUtilsTest.ImportPublicKeyFailed

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

[pastarmovj, 2012-05-21 14:20:39]

Please review this fix.

[Mattias Nissler (ping if slow), 2012-05-21 14:33:27]

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_utils.cc:129: return true;
It seems returning true here is actually the wrong thing to do. Is that
intentional? If the file doesn't exist, we return false.

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_utils_unittest.cc (right):

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_utils_unittest.cc:84: // will require
quite some free space on the test bots.
No. And this shouldn't be a TODO probably. If you want to make a test, you could
want to write a pyauto that uses a fuse filesystem to fake a >4GB file.

[pastarmovj, 2012-05-21 14:38:23]

Thanks for the quick review. PTAL.

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_utils.cc:129: return true;
On 2012/05/21 14:33:27, Mattias Nissler wrote:
> It seems returning true here is actually the wrong thing to do. Is that
> intentional? If the file doesn't exist, we return false.

This was intentional as can be seen from the unit test too. My reasoning is that
this function does not argue about the correctness of the file contents (as far
as they are loadable, hence the <4GB limit) only about the existence. I think
the caller should care if an empty string is fine or not rather than this util
function.

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_utils_unittest.cc (right):

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_utils_unittest.cc:84: // will require
quite some free space on the test bots.
On 2012/05/21 14:33:27, Mattias Nissler wrote:
> No. And this shouldn't be a TODO probably. If you want to make a test, you
could
> want to write a pyauto that uses a fuse filesystem to fake a >4GB file.

Done.

[Mattias Nissler (ping if slow), 2012-05-21 15:14:28]

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_utils.cc:129: return true;
On 2012/05/21 14:38:23, pastarmovj wrote:
> On 2012/05/21 14:33:27, Mattias Nissler wrote:
> > It seems returning true here is actually the wrong thing to do. Is that
> > intentional? If the file doesn't exist, we return false.
> 
> This was intentional as can be seen from the unit test too. My reasoning is
that
> this function does not argue about the correctness of the file contents (as
far
> as they are loadable, hence the <4GB limit) only about the existence. I think
> the caller should care if an empty string is fine or not rather than this util
> function.

Have you checked the caller? A quick look in OwnerManager::LoadOwnerKey() seems
to indicate that the caller is assuming that the length of the loaded key
material will be non-zero if you return true, which you are obviously violating
now.

[pastarmovj, 2012-05-21 16:26:02]

PTAL.

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/10399104/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/owner_key_utils.cc:129: return true;
On 2012/05/21 15:14:28, Mattias Nissler wrote:
> On 2012/05/21 14:38:23, pastarmovj wrote:
> > On 2012/05/21 14:33:27, Mattias Nissler wrote:
> > > It seems returning true here is actually the wrong thing to do. Is that
> > > intentional? If the file doesn't exist, we return false.
> > 
> > This was intentional as can be seen from the unit test too. My reasoning is
> that
> > this function does not argue about the correctness of the file contents (as
> far
> > as they are loadable, hence the <4GB limit) only about the existence. I
think
> > the caller should care if an empty string is fine or not rather than this
util
> > function.
> 
> Have you checked the caller? A quick look in OwnerManager::LoadOwnerKey()
seems
> to indicate that the caller is assuming that the length of the loaded key
> material will be non-zero if you return true, which you are obviously
violating
> now.
> 

Changed as discussed.

[Mattias Nissler (ping if slow), 2012-05-21 16:30:34]

LGTM. Can you please file a bug for handling the situation of bad keys/policy
gracefully in all weird cases?

http://codereview.chromium.org/10399104/diff/7001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/10399104/diff/7001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/owner_key_utils.cc:128: LOG(WARNING) << "Public
key file is empty. This seems wrong.";
nit: Remove "This seems wrong.", it is redundant.

[commit-bot: I haz the power, 2012-05-21 16:39:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10399104/7001

[commit-bot: I haz the power, 2012-05-21 20:51:27]

Try job failure for 10399104-7001 (retry) on win_rel for steps "base_unittests,
sync_unit_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 07:43:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10399104/7001