A hackish patch to make Breakpad upload the log if it's impossible to create a file in the crashed …

A hackish patch to make Breakpad upload the log if it's impossible to create a
file in the crashed process
(e.g. in a sandboxed Chromium renderer)
Instead we pass the log string (mapped by the Breakpad user) as an out-of-line
Mach message to the crash inspector, which creates the temporary file
and adds it to the config dictionary.

My current concerns are:
 -- Is it the right way to deal with sandboxed processes (assuming we'll be
using the current Breakpad for some time)?
 -- Should it be possible to pass several log messages OOL (ASan will never need
to)?
 -- Should we have a common ancestor for MachMsgPortDescriptor and
MachMsgOOLDescriptor (probably yes)?
 -- How "random" should the temporary filenames be? I suppose there shouldn't be
any security problem if the file is called "asan-log%timestamp%"?

[glider]

Mark,

can you appraise the idea?
Please disregard the hardcoded text constants and the bitwise casting of
different descriptor types -- I'm going to polish the patch if this is the right
direction to move towards.

[Mark Mentovai]

To Jeremy for front-line review. He’s done work with both Breakpad and Mach
message passing. Once he thinks it passes muster, I’ll take a look too.

[jeremy]

Initial review comments, I need to look over MachIPC more closely, I'll do that
on the next iteration.

Also, just wanted to double-check with you that the code in this CL is tested
and works?

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad.h
File client/mac/Framework/Breakpad.h (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:224: //                                   from a
char buffer passed out-of-line.
I'd be wary of allowing the caller to specify the logfile prefix.  e.g. if
malicious code runs in the renderer it could conceivably get the inspector
process to overwrite some system file or add one in a system directory that gets
loaded at runtime.

Can we have the inspector write the logfile in a well known location e.g.
<crash_id>-log1.txt ? Then there's no issue as far as an owned renderer
manipulating the file location?

nit: I think the fact that the file is passed as an OOL message is an
implementation detail, how about just naming this BREAKPAD_EXTRA_LOG_FILE_PREFIX
or some such?

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:278: // Store logContents into a file and add it
to the list of Breakpad files.
Need to expand this comment a bit...

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:279: void
BreakpadCreateAndAddLogFile(BreakpadRef ref, char *logContents);
Pass data as void*/size_t ?

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad.mm
File client/mac/Framework/Breakpad.mm (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:214: char                    *crash_log_chars_;
 // Crash log, if present
nit: terminology needs to be straightened out in the comments, "ool log file",
"log file", or "extra log file"?
nit: .

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:713: if (crash_log_chars_) {
if (result == KERN_SUCCESS && crash_log_chars_) ?

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:719: result =
sender.SendMessage(logptr_message, 2000);
break on failure?

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:1025: NSString *value =
@"/Users/glider/asan-log";
change this?

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
File client/mac/crash_generation/Inspector.h (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.h:51: kMsgType_InspectorLogStringPtr = 4
how about // data is contents of <insert common term for ool log data file>.

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
File client/mac/crash_generation/Inspector.mm (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:267: MachMsgOOLDescriptor *desc =
(MachMsgOOLDescriptor*)ool_message.GetDescriptor(0);
>80 chars.

https://breakpad.appspot.com/386001/diff/14001/common/mac/MachIPC.h
File common/mac/MachIPC.h (right):

https://breakpad.appspot.com/386001/diff/14001/common/mac/MachIPC.h#newcode212
common/mac/MachIPC.h:212: bool AddOOLDescriptor(void* address, mach_msg_size_t
ool_mem_size, boolean_t deallocate, mach_msg_copy_options_t copy);
This needs  acomment.

[glider]

Please take another look. So for the slow response time, there was a bunch of
holidays here at the beginning of May.

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad.h
File client/mac/Framework/Breakpad.h (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:224: //                                   from a
char buffer passed out-of-line.
On 2012/04/29 11:14:47, jeremy wrote:
> I'd be wary of allowing the caller to specify the logfile prefix.  e.g. if
> malicious code runs in the renderer it could conceivably get the inspector
> process to overwrite some system file or add one in a system directory that
gets
> loaded at runtime.
> 
> Can we have the inspector write the logfile in a well known location e.g.
> <crash_id>-log1.txt ? Then there's no issue as far as an owned renderer
> manipulating the file location?
> 
> nit: I think the fact that the file is passed as an OOL message is an
> implementation detail, how about just naming this
BREAKPAD_EXTRA_LOG_FILE_PREFIX
> or some such?

Yes, it's better to use a fixed file name. I'll remove the prefix. OTOH I need a
bool flag to set in the case there actually is an extra log.

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:278: // Store logContents into a file and add it
to the list of Breakpad files.
On 2012/04/29 11:14:47, jeremy wrote:
> Need to expand this comment a bit...

Done.

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:279: void
BreakpadCreateAndAddLogFile(BreakpadRef ref, char *logContents);
On 2012/04/29 11:14:47, jeremy wrote:
> Pass data as void*/size_t ?

Done.

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad.mm
File client/mac/Framework/Breakpad.mm (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:214: char                    *crash_log_chars_;
 // Crash log, if present
Done.
I didn't put a period there to be consistent with the rest of the file. I can
fix it if you insist.

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:713: if (crash_log_chars_) {
On 2012/04/29 11:14:47, jeremy wrote:
> if (result == KERN_SUCCESS && crash_log_chars_) ?

Done.

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:719: result =
sender.SendMessage(logptr_message, 2000);
On 2012/04/29 11:14:47, jeremy wrote:
> break on failure?

Don't think we need to. There's no loop to break from, and returning true or
false here depends on |send_and_exit_|, so why not fall through?

https://breakpad.appspot.com/386001/diff/14001/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:1025: NSString *value =
@"/Users/glider/asan-log";
On 2012/04/29 11:14:47, jeremy wrote:
> change this?

Changed to @BREAKPAD_HAS_EXTRA_LOG.
I'm going to put the log into the same dir where the crash dumps are stored.

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
File client/mac/crash_generation/Inspector.h (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.h:51: kMsgType_InspectorLogStringPtr = 4
On 2012/04/29 11:14:47, jeremy wrote:
> how about // data is contents of <insert common term for ool log data file>.

Done.

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
File client/mac/crash_generation/Inspector.mm (right):

https://breakpad.appspot.com/386001/diff/14001/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:267: MachMsgOOLDescriptor *desc =
(MachMsgOOLDescriptor*)ool_message.GetDescriptor(0);
On 2012/04/29 11:14:47, jeremy wrote:
> >80 chars.

Done.

https://breakpad.appspot.com/386001/diff/14001/common/mac/MachIPC.h
File common/mac/MachIPC.h (right):

https://breakpad.appspot.com/386001/diff/14001/common/mac/MachIPC.h#newcode212
common/mac/MachIPC.h:212: bool AddOOLDescriptor(void* address, mach_msg_size_t
ool_mem_size, boolean_t deallocate, mach_msg_copy_options_t copy);
On 2012/04/29 11:14:47, jeremy wrote:
> This needs  acomment.

Done.

[jeremy]

I asked last time but didn't see your answer: This code is tested and working in
it's current state, right?

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad.h
File client/mac/Framework/Breakpad.h (right):

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:277: // cannot create files.
It's not clear from the comment how this is different from the previous
function,. Also to my understanding this function can add a blob of
out-of-bounds data to be uploaded to crash rather than manipulating a crash dump
which is what the previous function does which make this more confusing.

How about renaming this to something like:
BreakpadAppendOOBDataToCrashDump()

Another thing that should be clarified in the comment is how many of these I can
add (e.g. is there a limit in number or size).

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad.mm
File client/mac/Framework/Breakpad.mm (right):

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:162: assert(strlen(extra_crash_log_data_) ==
extra_crash_log_size_);
* Are we sure we want to limit input data to a c-string?
* Do you do any sanity checks on the input data size?  What if a corrupt/owned
renderer tries to give you a 1GB data file?

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:723: crash_len : extra_crash_log_size_;
Why is this min(strlen(data), data_len) rather than just data_len ?

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:1029: size_t size) {
Size should also undergo a sanity check.

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
File client/mac/crash_generation/Inspector.mm (right):

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:279:
stringByAppendingPathExtension:@"log"];
If this code block is used multiple times, how about splitting it out into a
function?

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:290: int logFileCounter = 0;
Is this incremented somewhere?

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:340: bool Inspector::InspectTask() {
Guessing this isn't part of your change?

https://breakpad.appspot.com/386001/diff/22007/common/mac/MachIPC.mm
File common/mac/MachIPC.mm (right):

https://breakpad.appspot.com/386001/diff/22007/common/mac/MachIPC.mm#newcode153
common/mac/MachIPC.mm:153: desc_array[GetDescriptorCount()] =
MachMsgOOLDescriptor(address, ool_mem_size, deallocate, copy);
>80 chars.

https://breakpad.appspot.com/386001/diff/22007/common/mac/MachIPC.mm#newcode161
common/mac/MachIPC.mm:161: 
nit: remove NL

[glider]

On 2012/05/16 08:13:06, jeremy wrote:
> I asked last time but didn't see your answer: This code is tested and working
in
> it's current state, right?
To some extent, yes. This code works with another patch to uploader.mm, which is
interface-independent and just changes the bytes being sent to the server
(namely the multipart boundaries). Mark has committed it for me, though another
small change is needed to fix it.
The current behavior of Chrome crash reporting without ASan is left untouched,
but I'm not sure whether it's so. Do you know how I can test it?
Crash reporting upon ASan reports works with another patch to the actual Chrome
code that installs the ASan error reporting callback.

[glider]

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad.h
File client/mac/Framework/Breakpad.h (right):

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:277: // cannot create files.
On 2012/05/16 08:13:07, jeremy wrote:
> It's not clear from the comment how this is different from the previous
> function,. Also to my understanding this function can add a blob of
> out-of-bounds data to be uploaded to crash rather than manipulating a crash
dump
> which is what the previous function does which make this more confusing.
> 
> How about renaming this to something like:
> BreakpadAppendOOBDataToCrashDump()
> 
> Another thing that should be clarified in the comment is how many of these I
can
> add (e.g. is there a limit in number or size).

Done.

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad.mm
File client/mac/Framework/Breakpad.mm (right):

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:162: assert(strlen(extra_crash_log_data_) ==
extra_crash_log_size_);
On 2012/05/16 08:13:07, jeremy wrote:
> * Are we sure we want to limit input data to a c-string?
Probably no. Removed this check. You're right, I'd better not use strlen.
> * Do you do any sanity checks on the input data size?  What if a corrupt/owned
> renderer tries to give you a 1GB data file?
I've put a check for the size to be less than 4 Mb. This seems to be the only
place where we need it right now.

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:723: crash_len : extra_crash_log_size_;
On 2012/05/16 08:13:07, jeremy wrote:
> Why is this min(strlen(data), data_len) rather than just data_len ?

Done.

https://breakpad.appspot.com/386001/diff/22007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:1029: size_t size) {
On 2012/05/16 08:13:07, jeremy wrote:
> Size should also undergo a sanity check.

This is done in SetCrashLogData

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
File client/mac/crash_generation/Inspector.mm (right):

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:279:
stringByAppendingPathExtension:@"log"];
On 2012/05/16 08:13:07, jeremy wrote:
> If this code block is used multiple times, how about splitting it out into a
> function?

Done.

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:290: int logFileCounter = 0;
On 2012/05/16 08:13:07, jeremy wrote:
> Is this incremented somewhere?

No, because there's currently only one additional log. Removed.

https://breakpad.appspot.com/386001/diff/22007/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:340: bool Inspector::InspectTask() {
On 2012/05/16 08:13:07, jeremy wrote:
> Guessing this isn't part of your change?

It is. I've splitted the minidump path generation code into a separate function.

https://breakpad.appspot.com/386001/diff/22007/common/mac/MachIPC.mm
File common/mac/MachIPC.mm (right):

https://breakpad.appspot.com/386001/diff/22007/common/mac/MachIPC.mm#newcode153
common/mac/MachIPC.mm:153: desc_array[GetDescriptorCount()] =
MachMsgOOLDescriptor(address, ool_mem_size, deallocate, copy);
On 2012/05/16 08:13:07, jeremy wrote:
> >80 chars.

Done.

https://breakpad.appspot.com/386001/diff/22007/common/mac/MachIPC.mm#newcode161
common/mac/MachIPC.mm:161: 
On 2012/05/16 08:13:07, jeremy wrote:
> nit: remove NL

Done.

[jeremy]

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad.h
File client/mac/Framework/Breakpad.h (right):

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.h:277: // the client process cannot create files.
* Could you document the size limit on uploaded data.
* Instead of the last sentence:
"""
This is useful if the crashing process wishes to upload additional information
along with the crash dump.
"""

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad.mm
File client/mac/Framework/Breakpad.mm (right):

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:160: extra_crash_log_data_ = data;
s/extra/oob/ here and throughout the file.

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:162: assert(extra_crash_log_size_ < (1 << 22));
 // 4 Mb should be enough.
I think the assert should be the first line of the function.

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:723: /* deallocate */ 0, /* copy options */ 0);
0 -> MACH_MSG_PHYSICAL_COPY ?

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:1026: size_t size) {
if (size > kOOBDataMaxSize) {
  LOG(ERROR) << "aargh, too much data" << size;
  return;
}

Or something to that effect :)

https://breakpad.appspot.com/386001/diff/35007/client/mac/Framework/Breakpad....
client/mac/Framework/Breakpad.mm:1027: if (data) {
if (!data)
  return;
?

https://breakpad.appspot.com/386001/diff/35007/client/mac/crash_generation/In...
File client/mac/crash_generation/Inspector.mm (right):

https://breakpad.appspot.com/386001/diff/35007/client/mac/crash_generation/In...
client/mac/crash_generation/Inspector.mm:277: NSString *logFileKey = [NSString
stringWithFormat:@"%@0",
Since we only currently have one log file, I don't think we need the extra 0.

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.h
File common/mac/MachIPC.h (right):

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.h#newcode211
common/mac/MachIPC.h:211: bool AddDescriptor(const MachMsgPortDescriptor &desc);
nit: newline here.

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.h#newcode213
common/mac/MachIPC.h:213: // process.
Can you document the function parameters.

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.mm
File common/mac/MachIPC.mm (right):

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.mm#newcode134
common/mac/MachIPC.mm:134: // returns true if successful otherwise there was not
enough space
nit: First letter of comment should be capitalized - *Returns, etc.

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.mm#newcode134
common/mac/MachIPC.mm:134: // returns true if successful otherwise there was not
enough space
nit: First letter of comment should be capitalized - *Returns, etc.

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.mm#newcode139
common/mac/MachIPC.mm:139: int size = CalculateSize();
size_t

https://breakpad.appspot.com/386001/diff/35007/common/mac/MachIPC.mm#newcode149
common/mac/MachIPC.mm:149: bcopy(p, p+sizeof(MachMsgOOLDescriptor),
GetDataLength()+2*sizeof(int32_t));
Can you calculate the parameters to bcopy in temp variables. Also I think there
should be spaces around the arithmetic operators.

[jeremy]

glider: are you still working on this?

[glider]

On 2012/07/23 09:59:49, jeremy wrote:
> glider: are you still working on this?

This is still on my plate, but I had to temporarily switch to the Linux crash
uploading for now.