Adds imageWriterPrivate support for Windows

In order to support writing on windows we need to take advantage of the new privileged operation in the utility thread.  This code overlaps heavily with the Linux and OS X code, so all the writing for those is moved as well.

BUG=

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

[Drew Haven, 2013-11-20 01:12:16]

John,

Here's my initial attempt at organizing things so that the image-writing code is
in the utility thread for the recovery tool.  Could you give it a look and make
sure I'm organizing things properly?

A few questions I had: Is there a good place to put a common test utility
function like the image_writer_test_utils file I have?  Currently it's
duplicated because chrome/utility and chrome/browser don't share code.

Second, what's the namespace convention?  I see a lot of files in chrome/browser
without namespaces.  The utility code seems to generally be in chrome::module
format, which sounds logical for the chrome/browser code but instead it looks
like just ::module is standard.

Thanks for your help so far.

-- Drew

[jam, 2013-11-21 19:35:28]

Can you get someone from the extensions team to review the extensions changes,
and also someone from the chromeos team to review the new files in image_writer
for correctness? I looked at the general architecture and it looks fine.

To be explicitly clear, you're 100% sure at this point that this needs to be in
a separate process?


On 2013/11/20 01:12:16, Drew Haven wrote:
> John,
> 
> Here's my initial attempt at organizing things so that the image-writing code
is
> in the utility thread for the recovery tool.  Could you give it a look and
make
> sure I'm organizing things properly?
> 
> A few questions I had: Is there a good place to put a common test utility
> function like the image_writer_test_utils file I have?  Currently it's
> duplicated because chrome/utility and chrome/browser don't share code.

you can put it in chrome/test. we put test helpers there (i.e. not the actual
unittest or browesr test files, but any test-only code that they use)
> 
> Second, what's the namespace convention?  I see a lot of files in
chrome/browser
> without namespaces.  The utility code seems to generally be in chrome::module
> format, which sounds logical for the chrome/browser code but instead it looks
> like just ::module is standard.

in general, code in chrome is a leaf-node and shouldn't need a namespace. so
just don't use any. I realize some of the existing code doesn't do that.

> 
> Thanks for your help so far.
> 
> -- Drew

[jam, 2013-11-21 19:36:40]

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/extensions...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/image_writer_private/operation.h:64: virtual void
OnWriteImageSucceeded() OVERRIDE;
nit: convention is to put add

// ImageWriter::Delegate implementation:

before this

also, put this in the private section since it's not part of the public
interface of this class but is an implementation detail

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
File chrome/browser/image_writer/image_writer.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
chrome/browser/image_writer/image_writer.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
here and in other files, the new copyright header is at
http://www.chromium.org/developers/coding-style#TOC-File-headers, please use
that (i.e. no "(c)"


also, feel free to add OWNERS files with your name in the new directories you've
created

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
chrome/browser/image_writer/image_writer.h:15: // Decodes an image in a
sandboxed process.
update

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
chrome/browser/image_writer/image_writer.h:42: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
nit: put in private section

https://codereview.chromium.org/61643015/diff/60001/chrome/common/chrome_util...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:225:
IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_ImageWriter_Write,
ipc files have specific owners form the security team, so check out
chrome/common/OWNERS and pick one of them to review this file

https://codereview.chromium.org/61643015/diff/60001/chrome/common/image_write...
File chrome/common/image_writer/image_writer_client.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/common/image_write...
chrome/common/image_writer/image_writer_client.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
i don't see anyone using this file?

https://codereview.chromium.org/61643015/diff/60001/chrome/utility/image_writ...
File chrome/utility/image_writer/image_writer_handler.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/utility/image_writ...
chrome/utility/image_writer/image_writer_handler.h:18: class ImageWriterHandler
: public UtilityMessageHandler {
nit: please comment this class and also the methods below (and do this in all
new classes). at the very least, this would teach someone looking at this for
the first time that this means disk image, not image as in "picture"

[haven(GOOGLE), 2013-11-21 19:47:30]

Thanks for the feedback so far.  I understand the code is still a bit rough atm.

On 2013/11/21 19:35:28, jam wrote:
> Can you get someone from the extensions team to review the extensions changes,
> and also someone from the chromeos team to review the new files in
image_writer
> for correctness? I looked at the general architecture and it looks fine.
> 
> To be explicitly clear, you're 100% sure at this point that this needs to be
in
> a separate process?

I was under the impression it was the only way we would be able to do this with
any reasonable security.  Extra privileges appear to be required to write
directly to a storage device on most platforms (existing tools prompt, maps to
my understanding of OS permissions).  So we need to escalate privileges and we
also need to keep that out of the browser process.  I can't think of a way that
we can safely get those permissions in the browser.  There's a chance we could
use an installer-step to register with the OS to allow only the specific write
permission that we need, but I didn't think that was an option either.

> On 2013/11/20 01:12:16, Drew Haven wrote:
> > John,
> > 
> > Here's my initial attempt at organizing things so that the image-writing
code
> is
> > in the utility thread for the recovery tool.  Could you give it a look and
> make
> > sure I'm organizing things properly?
> > 
> > A few questions I had: Is there a good place to put a common test utility
> > function like the image_writer_test_utils file I have?  Currently it's
> > duplicated because chrome/utility and chrome/browser don't share code.
> 
> you can put it in chrome/test. we put test helpers there (i.e. not the actual
> unittest or browesr test files, but any test-only code that they use)
> > 
> > Second, what's the namespace convention?  I see a lot of files in
> chrome/browser
> > without namespaces.  The utility code seems to generally be in
chrome::module
> > format, which sounds logical for the chrome/browser code but instead it
looks
> > like just ::module is standard.
> 
> in general, code in chrome is a leaf-node and shouldn't need a namespace. so
> just don't use any. I realize some of the existing code doesn't do that.

Got it.


> > Thanks for your help so far.
> > 
> > -- Drew

[Drew Haven, 2013-11-26 02:10:42]

Thanks for the feedback.  I've gone through, addressed comments, cleaned up a
lot of the code and improved some of the tests.

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/extensions...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/image_writer_private/operation.h:64: virtual void
OnWriteImageSucceeded() OVERRIDE;
On 2013/11/21 19:36:41, jam wrote:
> nit: convention is to put add
> 
> // ImageWriter::Delegate implementation:
> 
> before this
> 
> also, put this in the private section since it's not part of the public
> interface of this class but is an implementation detail

Done.  I didn't realize you could put public methods of a public superclass in
the private section.  It makes sense, since it shouldn't be a public method of
this class as it is only a public method of the superclass.

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
File chrome/browser/image_writer/image_writer.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
chrome/browser/image_writer/image_writer.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/11/21 19:36:41, jam wrote:
> here and in other files, the new copyright header is at
> http://www.chromium.org/developers/coding-style#TOC-File-headers, please use
> that (i.e. no "(c)"
> 
> 
> also, feel free to add OWNERS files with your name in the new directories
you've
> created

Done.

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
chrome/browser/image_writer/image_writer.h:15: // Decodes an image in a
sandboxed process.
On 2013/11/21 19:36:41, jam wrote:
> update

Done.

https://codereview.chromium.org/61643015/diff/60001/chrome/browser/image_writ...
chrome/browser/image_writer/image_writer.h:42: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
On 2013/11/21 19:36:41, jam wrote:
> nit: put in private section

Done.

https://codereview.chromium.org/61643015/diff/60001/chrome/common/chrome_util...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/common/chrome_util...
chrome/common/chrome_utility_messages.h:225:
IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_ImageWriter_Write,
On 2013/11/21 19:36:41, jam wrote:
> ipc files have specific owners form the security team, so check out
> chrome/common/OWNERS and pick one of them to review this file

Will do.

https://codereview.chromium.org/61643015/diff/60001/chrome/common/image_write...
File chrome/common/image_writer/image_writer_client.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/common/image_write...
chrome/common/image_writer/image_writer_client.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/11/21 19:36:41, jam wrote:
> i don't see anyone using this file?

Oh, you're right.  Oops.

https://codereview.chromium.org/61643015/diff/60001/chrome/utility/image_writ...
File chrome/utility/image_writer/image_writer_handler.h (right):

https://codereview.chromium.org/61643015/diff/60001/chrome/utility/image_writ...
chrome/utility/image_writer/image_writer_handler.h:18: class ImageWriterHandler
: public UtilityMessageHandler {
On 2013/11/21 19:36:41, jam wrote:
> nit: please comment this class and also the methods below (and do this in all
> new classes). at the very least, this would teach someone looking at this for
> the first time that this means disk image, not image as in "picture"

Went through and documented all the .h files.

[Matt Perry, 2014-02-14 00:16:23]

Mostly LG, with a couple questions.

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
File chrome/browser/image_writer_client.cc (right):

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:53: message_loop_proxy_->PostTask(
Why is this done with PostTask but not so for Write?

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:55: // error_callback_.Run("Send message
failed.");
remove commented code

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:89:
utility_process_host_->DisableSandbox();
Did you mean to call this twice for !OS_WIN?

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right):

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_ipc_whitelist.cc:12:
ChromeUtilityMsg_ImageWriter_Verify::ID};
What does this do?

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.cc (right):

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:58:
SendFailed(error::kOperationAlreadyInProgress);
This will cancel the in-progress operation. Would it make more sense to let fail
the duplicate operation, but allow the current one to complete?

[Drew Haven, 2014-02-14 02:32:56]

Matt,

Thanks for the review.  Sorry about the delay.

I did my best to clean up some of the interaction between the utility
ImageWriter and ImageWriterHandler.  No more scoped_refptrs or any of that crap.
 Instead there's one ImageWriter that lives for the life of the
ImageWriterHandler.  I tried just combining them into one class, but that made
it really hard to test, so I'm keeping them separate for that reason.

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
File chrome/browser/image_writer_client.cc (right):

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:53: message_loop_proxy_->PostTask(
On 2014/02/14 00:16:23, Matt Perry wrote:
> Why is this done with PostTask but not so for Write?

Whoops, right.  We're running things here on the IO thread to talk to the
Utility process, but our caller is likely on a different thread.

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:55: // error_callback_.Run("Send message
failed.");
On 2014/02/14 00:16:23, Matt Perry wrote:
> remove commented code

Done.

https://codereview.chromium.org/61643015/diff/430001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:89:
utility_process_host_->DisableSandbox();
On 2014/02/14 00:16:23, Matt Perry wrote:
> Did you mean to call this twice for !OS_WIN?

Done.

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right):

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_ipc_whitelist.cc:12:
ChromeUtilityMsg_ImageWriter_Verify::ID};
On 2014/02/14 00:16:23, Matt Perry wrote:
> What does this do?

In Windows we have to do our writing to USB device with elevated privileges.  As
a security measure we only allow a limited set of approved IPC messages when
running elevated, that way we can avoid those operations that we're placing in
the utility process because they're buggy/unstable.

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.cc (right):

https://codereview.chromium.org/61643015/diff/430001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:58:
SendFailed(error::kOperationAlreadyInProgress);
On 2014/02/14 00:16:23, Matt Perry wrote:
> This will cancel the in-progress operation. Would it make more sense to let
fail
> the duplicate operation, but allow the current one to complete?

Good catch.  I completely missed that!

[Drew Haven, 2014-02-14 02:35:22]

Jorge,

Would you mind taking a look at this from a security standpoint?  You'll see the
new IPC messages and that they've been added to the whitelist.

In ImageWriterHandler I check that the device path is a removable device.  The
code there is basically a duplicate of the code in
removable_storage_provider_win and uses some Windows functions to get info about
the device.

[Drew Haven, 2014-02-14 18:05:32]

FYI, I'd like to get this in for M34.  Any tips to make sure that goes smoothly?
 I'm trying to make sure there aren't any small errors and that I'm resynced
constantly, but should I be pinging reviewers for the files that aren't yet
covered now, or wait until I have an LGTM from primary reviewers and then add
them just to approve?

[Matt Perry, 2014-02-14 20:13:40]

Changes LGTM. Feel free to get OWNERS approval whenever.

https://codereview.chromium.org/61643015/diff/480001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.cc (right):

https://codereview.chromium.org/61643015/diff/480001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:33: void
ImageWriterHandler::SendFailed(const std::string& message) {
looks like this doesn't get called now

https://codereview.chromium.org/61643015/diff/480001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:144: return false;
So this doesn't work on Linux or Mac at all? Why all the linux-specific code in
this CL then?

[Jorge Lucangeli Obes, 2014-02-14 20:42:13]

Did you end up implementing checks for the destination device of the write?

https://codereview.chromium.org/61643015/diff/480001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/61643015/diff/480001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.h:122: //
instantiate one client which should exist for the lifetime of
for the lifetime of... what?

[Drew Haven, 2014-02-14 21:34:33]

On 2014/02/14 20:42:13, Jorge Lucangeli Obes wrote:
> Did you end up implementing checks for the destination device of the write?
> 
>
https://codereview.chromium.org/61643015/diff/480001/chrome/browser/extension...
> File chrome/browser/extensions/api/image_writer_private/operation.h (right):
> 
>
https://codereview.chromium.org/61643015/diff/480001/chrome/browser/extension...
> chrome/browser/extensions/api/image_writer_private/operation.h:122: //
> instantiate one client which should exist for the lifetime of
> for the lifetime of... what?

Jorge: See ImageWriterHandler::IsValidDevice.

Right now I set it to just return false for everything except Windows which has
logic to query the device to see if it's removable.  Chrome OS uses a different
path (ImageBurnerClient) and next milestone I'll add in the code for OS X there.
 Linux I should be able to re-enable by using the same regexes as are used for
the ImageBurnerClient's back-end process, but the privileges in Linux are still
an open issue (I test locally by having a udev rule to make USB drives
read/write by my user).

[Jorge Lucangeli Obes, 2014-02-14 22:23:27]

What about checking that you're not asked to write a file that's not the
original image file?

Also, Matt left some comments on that file.

[Drew Haven, 2014-02-14 22:44:42]

On 2014/02/14 22:23:27, Jorge Lucangeli Obes wrote:
> What about checking that you're not asked to write a file that's not the
> original image file?
> 
> Also, Matt left some comments on that file.

In the utility process I'm not worried too much about what the original file is.
 The API has two sources for these files: fileSystem API files and things
downloaded from URL.  If we ever open up the API we will need to add host
patterns to the permissions for the URLs, but really it's anything the user
decides to write.  Once we're in the browser process do we need to worry about
something not being the original file, since the original file is basically
whatever is passed/downloaded in the browser process?  I think we agreed that we
didn't really care what gets written to the USB device as long as the target is
a USB device.  An attacker writing arbitrary things to the USB stick and using
that to attack the user somehow was out of our threat-model, I thought.

[Jorge Lucangeli Obes, 2014-02-14 22:51:05]

On 2014/02/14 22:44:42, Drew Haven wrote:
> On 2014/02/14 22:23:27, Jorge Lucangeli Obes wrote:
> > What about checking that you're not asked to write a file that's not the
> > original image file?
> > 
> > Also, Matt left some comments on that file.
> 
> In the utility process I'm not worried too much about what the original file
is.
>  The API has two sources for these files: fileSystem API files and things
> downloaded from URL.  If we ever open up the API we will need to add host
> patterns to the permissions for the URLs, but really it's anything the user
> decides to write.  Once we're in the browser process do we need to worry about
> something not being the original file, since the original file is basically
> whatever is passed/downloaded in the browser process?  I think we agreed that
we
> didn't really care what gets written to the USB device as long as the target
is
> a USB device.  An attacker writing arbitrary things to the USB stick and using
> that to attack the user somehow was out of our threat-model, I thought.

This is not a major concern. Still, the utility process with elevated privileges
will be able to read system files that the normal browser process won't be able
to. This is mitigated by the fact that we show UAC when the utility process
launches elevated, but it would still be good to think about how to remove this
detail from the code. If an IPC message says "write C:\Windows\System\something"
to an USB key, we should say no. Instead, we should only allow writing from
whitelisted locations.

[Drew Haven, 2014-02-14 23:00:56]

Responded to comments.

I have the update for that comment and some other small fixes waiting and ready
to go.  I made the mistake of

https://codereview.chromium.org/61643015/diff/480001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.h (right):

https://codereview.chromium.org/61643015/diff/480001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.h:122: //
instantiate one client which should exist for the lifetime of
On 2014/02/14 20:42:13, Jorge Lucangeli Obes wrote:
> for the lifetime of... what?

Oops, the operation.  E.g. one client per write/verify cycle.

https://codereview.chromium.org/61643015/diff/480001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.cc (right):

https://codereview.chromium.org/61643015/diff/480001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:33: void
ImageWriterHandler::SendFailed(const std::string& message) {
On 2014/02/14 20:13:41, Matt Perry wrote:
> looks like this doesn't get called now

It should be getting called from chrome/utility/image_writer/image_writer.cc
when there's a failure.

https://codereview.chromium.org/61643015/diff/480001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:144: return false;
On 2014/02/14 20:13:41, Matt Perry wrote:
> So this doesn't work on Linux or Mac at all? Why all the linux-specific code
in
> this CL then?

So, for this CL I was going to put in the catch-all false and then after the
milestone put in guards for Linux and OS X.  Those two should share most of the
same code but will have different permissions models.  I do a lot of testing on
my local linux box since I have udev rules set up to make USB media read/write
by my user.

[Drew Haven, 2014-02-14 23:26:06]

Julien,

Here is what has become of that utility-process USB writing I mentioned to you a
while ago.  Would you mind taking a look from a security perspective and
potentially giving the LGTM for the new IPC messages?

Thank you.

[Jorge Lucangeli Obes, 2014-02-14 23:29:00]

chrome_content_utility_ipc_whitelist.cc LGTM

[Drew Haven, 2014-02-14 23:29:36]

On 2014/02/14 22:51:05, Jorge Lucangeli Obes wrote:
> On 2014/02/14 22:44:42, Drew Haven wrote:
> > On 2014/02/14 22:23:27, Jorge Lucangeli Obes wrote:
> > > What about checking that you're not asked to write a file that's not the
> > > original image file?
> > > 
> > > Also, Matt left some comments on that file.
> > 
> > In the utility process I'm not worried too much about what the original file
> is.
> >  The API has two sources for these files: fileSystem API files and things
> > downloaded from URL.  If we ever open up the API we will need to add host
> > patterns to the permissions for the URLs, but really it's anything the user
> > decides to write.  Once we're in the browser process do we need to worry
about
> > something not being the original file, since the original file is basically
> > whatever is passed/downloaded in the browser process?  I think we agreed
that
> we
> > didn't really care what gets written to the USB device as long as the target
> is
> > a USB device.  An attacker writing arbitrary things to the USB stick and
using
> > that to attack the user somehow was out of our threat-model, I thought.
> 
> This is not a major concern. Still, the utility process with elevated
privileges
> will be able to read system files that the normal browser process won't be
able
> to. This is mitigated by the fact that we show UAC when the utility process
> launches elevated, but it would still be good to think about how to remove
this
> detail from the code. If an IPC message says "write
C:\Windows\System\something"
> to an USB key, we should say no. Instead, we should only allow writing from
> whitelisted locations.

How would we construct such a white-list?  If we're going to allow people to
write from local images, which is one of our features in previous versions of
the tools, then we basically have to allow them to write from anywhere that they
can get a file from the chooser.  One thing we could maybe do is limit it to
temporary directories and then our API-level code could copy the file into a
temporary directory before writing.  I'm not sure how we would code that check
though.  How does one say "is this directory a valid temp directory?"

[Drew Haven, 2014-02-14 23:32:59]

Lei,

Here's that CL with the image-writer-in-utility-thread that I mentioned.  Mind
taking a look to approve the chrome_content_utility_client.cc change and
chrome/browser additions?

[Jorge Lucangeli Obes, 2014-02-14 23:35:52]

https://codereview.chromium.org/61643015/diff/670001/chrome/common/chrome_uti...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/common/chrome_uti...
chrome/common/chrome_utility_messages.h:278: base::FilePath /* target file */)
Please amend the comment mentioning that the utility side verifies the path.

"/* target file, checked on receiving end. */" or something like that.

[Lei Zhang, 2014-02-15 00:01:12]

Can we put chrome/browser/image_writer_client.* under chrome/browser/extensions
somewhere? Reading the rest of the CL...

[Drew Haven, 2014-02-15 00:07:05]

Jorge: comments added.

https://codereview.chromium.org/61643015/diff/670001/chrome/common/chrome_uti...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/common/chrome_uti...
chrome/common/chrome_utility_messages.h:278: base::FilePath /* target file */)
On 2014/02/14 23:35:52, Jorge Lucangeli Obes wrote:
> Please amend the comment mentioning that the utility side verifies the path.
> 
> "/* target file, checked on receiving end. */" or something like that.

Done.  I added better comments to all the messages I added.

[Jorge Lucangeli Obes, 2014-02-15 00:13:59]

chrome/common/chrome_utility_messages.h lgtm.

https://codereview.chromium.org/61643015/diff/640003/chrome/common/chrome_uti...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/61643015/diff/640003/chrome/common/chrome_uti...
chrome/common/chrome_utility_messages.h:283: // was written to the target.  As
above the target will be restricted to
Nit (I'm a terrible human being): consistent spacing after full stop. Feel free
to land without fixing.

[Lei Zhang, 2014-02-15 00:26:59]

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.cc:201: int
progress = kProgressComplete * curr_bytes / total_bytes;
Can this overflow? Same with line 338.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_win.cc
(right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_win.cc:49:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
Consider switch to a blocking pool in a future CL.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/test_utils.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/test_utils.cc:115:
ReadFile(device_path, device_buffer.get(), kTestFileSize);
Why aren't you checking the return value here anymore? ReadFile() can return -1,
BTW. You don't want |image_bytes_read| to be -1....

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
File chrome/browser/image_writer_client.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:27: progress_callback_ =
progress_callback;
nit: Put these in the initializer list?

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:75: if (utility_process_host_ &&
not needed, already done in Send()

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:84:
base::MessageLoop::current()->message_loop_proxy();
base::MessageLoopProxy::current() ?

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
File chrome/browser/image_writer_client.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.h:17: typedef base::Callback<void()>
CancelCallback;
Better knowns as base::Closure

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.h:63: virtual bool Send(IPC::Message* msg);
Either it needs an OVERRIDE or it's not part of UtilityProcessHostClient impl

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_ipc_whitelist.cc:6: #include
"chrome/common/chrome_utility_messages.h"
nit: blank line above this

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/error_messages.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/error_messages.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
nit: it's 2014, same elsewhere.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/error_messages.h:27: }  // namespace extensions
nit: this comment is inconsistent with line 8. We decided to not bother with
"namespace chrome" BTW, since chrome/ is the top level project...

[Lei Zhang, 2014-02-15 00:49:15]

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.cc:134: bool ImageWriter::IsRunning() {
nit: const method

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.cc:174: bytes_processed_ += bytes_read;
Presumably it's ok to share |bytes_processed_| between Write and Verify because
only 1 operation can take place at any time?

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.cc:202: Error(error::kReadImage);
do you want to return in this case?

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.h:26: // Starts a write from image_path
to device_path.
nit: foo_path -> |foo_path|, same below.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:7: #include
"chrome/utility/image_writer/image_writer.h"
Already included in .h file.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:99:
CreateFile(device16.c_str(),
Just use device.value().c_str() ?

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:126: 1024,                  
         // Size of buffer.
Make 1024 a const. Can't tell how easy it is to do sizeof() with a
scoped_ptr<char[]>.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:143: // Other platforms will
have to be added as they are supported.
nit: Add a NOTIMPLEMENTED()

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.h:8: #include
"base/files/file_path.h"
nit: forward declare instead, move #include to .cc file

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.h:17: // handler in
|chrome::ChromeContentUtilityClient.
nit: remove '|'

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.h:33: // Small wrapper for
sendingon the UtilityProcess.
nit: sendingon -> sending on

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_unittest.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_unittest.cc:42:
file_util::WriteFile(path, buffer.get(), kTestFileSize);
check return results

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_unittest.cc:130:
base::DeleteFile(image_path_, false);
check return results, same below

[Lei Zhang, 2014-02-15 01:07:25]

https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/test_utils.cc (right):

https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/test_utils.cc:116: if
(image_bytes_read == 0)
This can't happen when you have requested 32 KB.

[Drew Haven, 2014-02-15 01:23:57]

Sending response to review so far.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.cc:201: int
progress = kProgressComplete * curr_bytes / total_bytes;
On 2014/02/15 00:26:59, Lei Zhang wrote:
> Can this overflow? Same with line 338.

My understanding is that kProgressComplete * curr_bytes will be in64.  Since
kProgressComplete is 100 this shouldn't overflow for any file size we'd
realistically encounter.  Then we divide by the total_bytes, with curr_bytes <
total_bytes (though maybe that should be asserted in some way) and so the result
should always been in [0,100].

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation_win.cc
(right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation_win.cc:49:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2014/02/15 00:26:59, Lei Zhang wrote:
> Consider switch to a blocking pool in a future CL.

tbarzic suggested that on another CL.  I already opened a bug for it.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/test_utils.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/test_utils.cc:115:
ReadFile(device_path, device_buffer.get(), kTestFileSize);
On 2014/02/15 00:26:59, Lei Zhang wrote:
> Why aren't you checking the return value here anymore? ReadFile() can return
-1,
> BTW. You don't want |image_bytes_read| to be -1....

I think i assumed the memcmp would do the work, but you're right, particularly
if both are -1.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
File chrome/browser/image_writer_client.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:27: progress_callback_ =
progress_callback;
On 2014/02/15 00:26:59, Lei Zhang wrote:
> nit: Put these in the initializer list?

Not a constructor here.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:75: if (utility_process_host_ &&
On 2014/02/15 00:26:59, Lei Zhang wrote:
> not needed, already done in Send()

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.cc:84:
base::MessageLoop::current()->message_loop_proxy();
On 2014/02/15 00:26:59, Lei Zhang wrote:
> base::MessageLoopProxy::current() ?

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
File chrome/browser/image_writer_client.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.h:17: typedef base::Callback<void()>
CancelCallback;
On 2014/02/15 00:26:59, Lei Zhang wrote:
> Better knowns as base::Closure

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/image_wri...
chrome/browser/image_writer_client.h:63: virtual bool Send(IPC::Message* msg);
On 2014/02/15 00:26:59, Lei Zhang wrote:
> Either it needs an OVERRIDE or it's not part of UtilityProcessHostClient impl

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_ipc_whitelist.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_ipc_whitelist.cc:6: #include
"chrome/common/chrome_utility_messages.h"
On 2014/02/15 00:26:59, Lei Zhang wrote:
> nit: blank line above this

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/error_messages.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/error_messages.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/02/15 00:26:59, Lei Zhang wrote:
> nit: it's 2014, same elsewhere.

I started this CL in 2013...

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/error_messages.h:27: }  // namespace extensions
On 2014/02/15 00:26:59, Lei Zhang wrote:
> nit: this comment is inconsistent with line 8. We decided to not bother with
> "namespace chrome" BTW, since chrome/ is the top level project...

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.cc:134: bool ImageWriter::IsRunning() {
On 2014/02/15 00:49:16, Lei Zhang wrote:
> nit: const method

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.cc:174: bytes_processed_ += bytes_read;
On 2014/02/15 00:49:16, Lei Zhang wrote:
> Presumably it's ok to share |bytes_processed_| between Write and Verify
because
> only 1 operation can take place at any time?

Correct. Since the PlatformFiles are ultimately the arbitrators this just shares
the same access restrictions.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.cc:202: Error(error::kReadImage);
On 2014/02/15 00:49:16, Lei Zhang wrote:
> do you want to return in this case?

Actually, I'm going to remove this because I should be able to count on
ReadPlatformFile returning 0 at the end of the file only for a regular file,
right?

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer.h:26: // Starts a write from image_path
to device_path.
On 2014/02/15 00:49:16, Lei Zhang wrote:
> nit: foo_path -> |foo_path|, same below.

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:7: #include
"chrome/utility/image_writer/image_writer.h"
On 2014/02/15 00:49:16, Lei Zhang wrote:
> Already included in .h file.

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:99:
CreateFile(device16.c_str(),
On 2014/02/15 00:49:16, Lei Zhang wrote:
> Just use device.value().c_str() ?

Huh, I wonder why I did it that way...

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:126: 1024,                  
         // Size of buffer.
On 2014/02/15 00:49:16, Lei Zhang wrote:
> Make 1024 a const. Can't tell how easy it is to do sizeof() with a
> scoped_ptr<char[]>.

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.cc:143: // Other platforms will
have to be added as they are supported.
On 2014/02/15 00:49:16, Lei Zhang wrote:
> nit: Add a NOTIMPLEMENTED()

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_handler.h (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.h:8: #include
"base/files/file_path.h"
On 2014/02/15 00:49:16, Lei Zhang wrote:
> nit: forward declare instead, move #include to .cc file

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.h:17: // handler in
|chrome::ChromeContentUtilityClient.
On 2014/02/15 00:49:16, Lei Zhang wrote:
> nit: remove '|'

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_handler.h:33: // Small wrapper for
sendingon the UtilityProcess.
On 2014/02/15 00:49:16, Lei Zhang wrote:
> nit: sendingon -> sending on

Done.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
File chrome/utility/image_writer/image_writer_unittest.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_unittest.cc:42:
file_util::WriteFile(path, buffer.get(), kTestFileSize);
On 2014/02/15 00:49:16, Lei Zhang wrote:
> check return results

I assume an ASSERT_TRUE is fine for a test since we don't want to continue if
the setup fails.

https://codereview.chromium.org/61643015/diff/670001/chrome/utility/image_wri...
chrome/utility/image_writer/image_writer_unittest.cc:130:
base::DeleteFile(image_path_, false);
On 2014/02/15 00:49:16, Lei Zhang wrote:
> check return results, same below

Done.

https://codereview.chromium.org/61643015/diff/640003/chrome/common/chrome_uti...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/61643015/diff/640003/chrome/common/chrome_uti...
chrome/common/chrome_utility_messages.h:283: // was written to the target.  As
above the target will be restricted to
On 2014/02/15 00:14:00, Jorge Lucangeli Obes wrote:
> Nit (I'm a terrible human being): consistent spacing after full stop. Feel
free
> to land without fixing.

I'm a double-spacer out of habit.  I specifically made the first one a
single-space to be consistent with the rest of the file, but missed the second.

(I'm a terrible human being)Double-space is still obviously superior.

[Lei Zhang, 2014-02-15 01:41:49]

lgtm

I have not checked all the browser <-> utility interactions. I assume mpcomplete
already looked over that part with you.

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.cc:201: int
progress = kProgressComplete * curr_bytes / total_bytes;
On 2014/02/15 01:23:58, Drew Haven wrote:
> On 2014/02/15 00:26:59, Lei Zhang wrote:
> > Can this overflow? Same with line 338.
> 
> My understanding is that kProgressComplete * curr_bytes will be in64.  Since
> kProgressComplete is 100 this shouldn't overflow for any file size we'd
> realistically encounter.  Then we divide by the total_bytes, with curr_bytes <
> total_bytes (though maybe that should be asserted in some way) and so the
result
> should always been in [0,100].

Do you trust the utility process to send you a valid |curr_bytes| ? If yes, then
ok. For some utility processes, such as parsers, we certainly don't trust them
because the parser code may have bugs. In this case,  I think you are separating
the code into the utility mostly for privilege escalation, so otherwise this
would be browser code that you' d trust.

[Jorge Lucangeli Obes, 2014-02-15 01:43:18]

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
File chrome/browser/extensions/api/image_writer_private/operation.cc (right):

https://codereview.chromium.org/61643015/diff/670001/chrome/browser/extension...
chrome/browser/extensions/api/image_writer_private/operation.cc:201: int
progress = kProgressComplete * curr_bytes / total_bytes;
On 2014/02/15 01:23:58, Drew Haven wrote:
> On 2014/02/15 00:26:59, Lei Zhang wrote:
> > Can this overflow? Same with line 338.
> 
> My understanding is that kProgressComplete * curr_bytes will be in64.  Since
> kProgressComplete is 100 this shouldn't overflow for any file size we'd
> realistically encounter.  Then we divide by the total_bytes, with curr_bytes <
> total_bytes (though maybe that should be asserted in some way) and so the
result
> should always been in [0,100].

I would add CHECKS for both values to be positive, and curr_bytes < total_bytes.

https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/test_utils.cc (right):

https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/test_utils.cc:129: return
memcmp(image_buffer.get(), device_buffer.get(), image_bytes_read) == 0;
This is converting signed -> size_t, but at least we have the >= check above.

https://codereview.chromium.org/61643015/diff/1260001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer.cc (right):

https://codereview.chromium.org/61643015/diff/1260001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer.cc:15: const int kBurningBlockSize = 1
<< 20;
Why signed? Block size cannot be negative.

[Drew Haven, 2014-02-15 02:15:14]

https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/test_utils.cc (right):

https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/test_utils.cc:116: if
(image_bytes_read == 0)
On 2014/02/15 01:07:26, Lei Zhang wrote:
> This can't happen when you have requested 32 KB.

Done.

https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/test_utils.cc:129: return
memcmp(image_buffer.get(), device_buffer.get(), image_bytes_read) == 0;
On 2014/02/15 01:43:19, Jorge Lucangeli Obes wrote:
> This is converting signed -> size_t, but at least we have the >= check above.

Good point.  How convinient that ReadFile takes an int and so cannot read more
than fits in a signed int.

https://codereview.chromium.org/61643015/diff/1260001/chrome/utility/image_wr...
File chrome/utility/image_writer/image_writer.cc (right):

https://codereview.chromium.org/61643015/diff/1260001/chrome/utility/image_wr...
chrome/utility/image_writer/image_writer.cc:15: const int kBurningBlockSize = 1
<< 20;
On 2014/02/15 01:43:19, Jorge Lucangeli Obes wrote:
> Why signed? Block size cannot be negative.

You're right.  It should be a size_t, if I understand correctly.  As I visit
this code int he future I'll make sure I'm using size_t.

[Jorge Lucangeli Obes, 2014-02-15 02:20:07]

On 2014/02/15 02:15:14, Drew Haven wrote:
>
https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
> File chrome/browser/extensions/api/image_writer_private/test_utils.cc (right):
> 
>
https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
> chrome/browser/extensions/api/image_writer_private/test_utils.cc:116: if
> (image_bytes_read == 0)
> On 2014/02/15 01:07:26, Lei Zhang wrote:
> > This can't happen when you have requested 32 KB.
> 
> Done.
> 
>
https://codereview.chromium.org/61643015/diff/1260001/chrome/browser/extensio...
> chrome/browser/extensions/api/image_writer_private/test_utils.cc:129: return
> memcmp(image_buffer.get(), device_buffer.get(), image_bytes_read) == 0;
> On 2014/02/15 01:43:19, Jorge Lucangeli Obes wrote:
> > This is converting signed -> size_t, but at least we have the >= check
above.
> 
> Good point.  How convinient that ReadFile takes an int and so cannot read more
> than fits in a signed int.
> 
>
https://codereview.chromium.org/61643015/diff/1260001/chrome/utility/image_wr...
> File chrome/utility/image_writer/image_writer.cc (right):
> 
>
https://codereview.chromium.org/61643015/diff/1260001/chrome/utility/image_wr...
> chrome/utility/image_writer/image_writer.cc:15: const int kBurningBlockSize =
1
> << 20;
> On 2014/02/15 01:43:19, Jorge Lucangeli Obes wrote:
> > Why signed? Block size cannot be negative.
> 
> You're right.  It should be a size_t, if I understand correctly.  As I visit
> this code int he future I'll make sure I'm using size_t.

Please file a bug to track the integer issues. We should also try to make the
Progress IPC message send an unsigned int.

[Drew Haven, 2014-02-15 03:14:23]

https://code.google.com/p/chromium/issues/detail?id=344117


On Fri, Feb 14, 2014 at 6:20 PM, <jorgelo@chromium.org> wrote:

> On 2014/02/15 02:15:14, Drew Haven wrote:
>
> https://codereview.chromium.org/61643015/diff/1260001/
> chrome/browser/extensions/api/image_writer_private/test_utils.cc
>
>> File chrome/browser/extensions/api/image_writer_private/test_utils.cc
>> (right):
>>
>
>
> https://codereview.chromium.org/61643015/diff/1260001/
> chrome/browser/extensions/api/image_writer_private/test_
> utils.cc#newcode116
>
>> chrome/browser/extensions/api/image_writer_private/test_utils.cc:116: if
>> (image_bytes_read == 0)
>> On 2014/02/15 01:07:26, Lei Zhang wrote:
>> > This can't happen when you have requested 32 KB.
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.org/61643015/diff/1260001/
> chrome/browser/extensions/api/image_writer_private/test_
> utils.cc#newcode129
>
>> chrome/browser/extensions/api/image_writer_private/test_utils.cc:129:
>> return
>> memcmp(image_buffer.get(), device_buffer.get(), image_bytes_read) == 0;
>> On 2014/02/15 01:43:19, Jorge Lucangeli Obes wrote:
>> > This is converting signed -> size_t, but at least we have the >= check
>>
> above.
>
>  Good point.  How convinient that ReadFile takes an int and so cannot read
>> more
>> than fits in a signed int.
>>
>
>
> https://codereview.chromium.org/61643015/diff/1260001/
> chrome/utility/image_writer/image_writer.cc
>
>> File chrome/utility/image_writer/image_writer.cc (right):
>>
>
>
> https://codereview.chromium.org/61643015/diff/1260001/
> chrome/utility/image_writer/image_writer.cc#newcode15
>
>> chrome/utility/image_writer/image_writer.cc:15: const int
>> kBurningBlockSize =
>>
> 1
>
>> << 20;
>> On 2014/02/15 01:43:19, Jorge Lucangeli Obes wrote:
>> > Why signed? Block size cannot be negative.
>>
>
>  You're right.  It should be a size_t, if I understand correctly.  As I
>> visit
>> this code int he future I'll make sure I'm using size_t.
>>
>
> Please file a bug to track the integer issues. We should also try to make
> the
> Progress IPC message send an unsigned int.
>
> https://codereview.chromium.org/61643015/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Drew Haven, 2014-02-19 00:50:54]

Ping on final security sign-off.  Perhaps if Julien is busy we can find someone
else.  I can go through the owners file, but perhaps you could recommend
someone, Jorge?

[jln (very slow on Chromium), 2014-02-19 01:07:38]

On 2014/02/19 00:50:54, Drew Haven wrote:
> Ping on final security sign-off.  Perhaps if Julien is busy we can find
someone
> else.  I can go through the owners file, but perhaps you could recommend
> someone, Jorge?

chrome/common/chrome_utility_messages.h lgtm rubberstamp based on Jorge review.

[Drew Haven, 2014-02-19 01:15:22]

Thank you, everyone!  I wouldn't be able to do this without all the input from
every one of you.

[Drew Haven, 2014-02-20 19:13:14]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-20 19:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/61643015/1780002

[commit-bot: I haz the power, 2014-02-20 20:40:51]

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

[commit-bot: I haz the power, 2014-02-20 20:40:52]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Drew Haven, 2014-02-20 22:23:44]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-20 22:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/61643015/2150001

[commit-bot: I haz the power, 2014-02-21 00:04:08]

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

[commit-bot: I haz the power, 2014-02-21 00:04:09]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Drew Haven, 2014-02-21 01:06:11]

The CQ bit was checked by haven@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:11:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haven@chromium.org/61643015/2150001

[commit-bot: I haz the power, 2014-02-21 02:28:38]

Change committed as 252466