Make custom info population before dump generation as an optional operation

Submitting this on behalf of Xiaoling Bao.

Make custom info population before dump generation as an optional operation. 
This is part of a security change to move the crash generation and upload out of
Google updater process.
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1186

[Ivan Penkov]

.

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
File src/client/windows/crash_generation/crash_generation_server.cc (right):

https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
src/client/windows/crash_generation/crash_generation_server.cc:839:
client_info->PopulateCustomInfo();
Do something useful with the return value (since you’re fixing it to be correct
now)?

[Mark Mentovai]

https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
File src/client/windows/crash_generation/client_info.h (right):

https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
src/client/windows/crash_generation/client_info.h:69: CustomClientInfo
custom_client_info() const { return custom_client_info_; }
This can return a const reference, although I don’t see it being used anywhere,
so maybe it can just be removed. Is this intended to be a public API that some
code outside of Breakpad will call?

[xiaolingbao]

On 2013/05/10 20:11:15, Mark Mentovai wrote:
>
https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
> File src/client/windows/crash_generation/client_info.h (right):
> 
>
https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
> src/client/windows/crash_generation/client_info.h:69: CustomClientInfo
> custom_client_info() const { return custom_client_info_; }
> This can return a const reference, although I don’t see it being used
anywhere,
> so maybe it can just be removed. Is this intended to be a public API that some
> code outside of Breakpad will call?

Hi Mark, you are right this is not used inside breakpad but will be used by
Omaha (a breakpad client). Basically we want to get the raw custom info pointer
in the crashed process space so we can read the custom info by ourselves
(actually we'll created another process with same privilege as the crashed
process to read the memory and generate dump for security reasons). The returned
structure is small (an integer + a pointer) and so return value is OK to avoid
potential object life time issue?

[Mark Mentovai]

The same lifetime issue exists regardless of whether this returns a const
reference, because CustomClientInfo carries an array of CustomInfoEntry* that
point to storage that’s owned by the ClientInfo object.

Does the future consumer of this API assume that custom_client_info() can
outlive the ClientInfo that was used to fetch it? If so, that needs to be
reworked. Otherwise, this makes it pretty evident that a const reference is
still correct for this getter.

[xiaolingbao]

On 2013/05/10 20:58:10, Mark Mentovai wrote:
> The same lifetime issue exists regardless of whether this returns a const
> reference, because CustomClientInfo carries an array of CustomInfoEntry* that
> point to storage that’s owned by the ClientInfo object.
> 
> Does the future consumer of this API assume that custom_client_info() can
> outlive the ClientInfo that was used to fetch it? If so, that needs to be
> reworked. Otherwise, this makes it pretty evident that a const reference is
> still correct for this getter.

My understanding is that storage you mentioned is not owned by ClientInfo
object. It seems to be in the crashed process memory space. So as long as the
crashed process still exists, the pointers will be valid (access via
ReadProcessMemory() of course). The memory owned by ClientInfo is another member
named custom_info_entries_.

So old API ClientInfo::GetCustomInfo() returns custom_info_entries_ and the
newly added API returns the raw pointer. We want to access the raw pointer so
that we can read the custom info by ourselves (inside a lower privilege
process).

[Ivan Penkov]

On 2013/05/10 21:29:50, xiaolingbao wrote:
> On 2013/05/10 20:58:10, Mark Mentovai wrote:
> > The same lifetime issue exists regardless of whether this returns a const
> > reference, because CustomClientInfo carries an array of CustomInfoEntry*
that
> > point to storage that’s owned by the ClientInfo object.
> > 
> > Does the future consumer of this API assume that custom_client_info() can
> > outlive the ClientInfo that was used to fetch it? If so, that needs to be
> > reworked. Otherwise, this makes it pretty evident that a const reference is
> > still correct for this getter.
> 
> My understanding is that storage you mentioned is not owned by ClientInfo
> object. It seems to be in the crashed process memory space. So as long as the
> crashed process still exists, the pointers will be valid (access via
> ReadProcessMemory() of course). The memory owned by ClientInfo is another
member
> named custom_info_entries_.
> 
> So old API ClientInfo::GetCustomInfo() returns custom_info_entries_ and the
> newly added API returns the raw pointer. We want to access the raw pointer so
> that we can read the custom info by ourselves (inside a lower privilege
> process).

Mark, on your first comment you said LGTM.  Should I consider this as green
light to proceed with the check-in or we need to push back and rework this?

[Mark Mentovai]

I said LGTM but I still think the const reference change should be made. LGTM
either wya.

[Ivan Penkov]

The const reference change is done.  I'm waiting for a test run and I'll commit
it shortly after that.

https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
File src/client/windows/crash_generation/client_info.h (right):

https://breakpad.appspot.com/586003/diff/1/src/client/windows/crash_generatio...
src/client/windows/crash_generation/client_info.h:69: CustomClientInfo
custom_client_info() const { return custom_client_info_; }
On 2013/05/10 20:11:15, Mark Mentovai wrote:
> This can return a const reference, although I don’t see it being used
anywhere,
> so maybe it can just be removed. Is this intended to be a public API that some
> code outside of Breakpad will call?

Done.