Issue 744002: In iOS, BREAKPAD_OUTPUT_DUMP_FILE value contains a malformed full path to the dump file (Closed)
|
Created:
10 years, 12 months ago by Akiva Modified:
10 years, 11 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionIn iOS, BREAKPAD_OUTPUT_DUMP_FILE value contains a malformed full path to the dump file. The std::string dumpFilename already contains the full pathname to the dump file. Appending it to the dumpDirAsNSString creates a string with the path portion duplicated, e.g.: /var/mobile/Applications/516BE756-DFD4-4F9B-85D5-85966B0038F7/Library/Caches/Breakpad/var/mobile/Applications/516BE756-DFD4-4F9B-85D5-85966B0038F7/Library/Caches/Breakpad/0A406D28-437D-48EE-9989-23F7F871818E.dmp Instead of this: /var/mobile/Applications/516BE756-DFD4-4F9B-85D5-85966B0038F7/Library/Caches/Breakpad/0A406D28-437D-48EE-9989-23F7F871818E.dmp Patch Set 1 #
Total comments: 3
MessagesTotal messages: 13
I just re-read the README which contains the instructions for making changes to the Breakpad code base. I read it weeks ago, but forgot that it discussed this. I created this issue by reading posts from others in the group. Anyway, I don't have read/write access, but I assume someone else can commit the code if it is okay. I don't have a reviewer, but it is a pretty simple change. Please add reviewers as necessary. Many thanks
Sign in to reply to this message.
Added thestig and markus as reviewers. Feel free to recommend other reviewers instead.
Sign in to reply to this message.
Deferring to qsr@
Sign in to reply to this message.
https://breakpad.appspot.com/744002/diff/1/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/744002/diff/1/src/client/ios/Breakpad.mm#newcode514 src/client/ios/Breakpad.mm:514: I suspect, I am not the best reviewer, as I am entirely unfamiliar with both IOS and Objective-C. Having said that, I would normally err on the side of being conservative, and write code that checks whether "dumpFilename" is an absolute path. Based on whether it is, I would either use it directly or prepend our known-good path. This way, you avoid the risk of ever accidentally writing into the current directory -- which is probably not where you want the file to go. Of course, if either the path or the filename are user-controllable, things get trickier, as there are always possibilities of security problems. In that case, I would probably make the filename canonical first. No idea how to do this in IOS though. Feel free to ignore all of these comments, if they sound inappropriate. They are just common good practice, that might or might not apply.
Sign in to reply to this message.
https://breakpad.appspot.com/744002/diff/1/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/744002/diff/1/src/client/ios/Breakpad.mm#newcode514 src/client/ios/Breakpad.mm:514: Line 490: the path to the dump directory is retrieved (as an NSString). This is potentially set by the user; regardless, it is the path to the directory where the minidump will be stored. Line 493: the path to the dump directory is converted to a C string. Line 498: a unique filename is created. the dump directory from line 493 is used as input. The return value is a std:string that contains "some sort of path" to the filename where the minidump will be stored. Line 499: the path from line 498 is used to actually write the minidump. If the path is relative, absolute, correct, incorrect, or whatever.... that is the path actually used to write the file. Line 513: My change simply uses the same path to the filename to report where the minidump was actually stored. The original code will always report the location incorrectly. My change does not influence where the file is written; it only corrects how the file location is reported (in the returned Dictionary).
Sign in to reply to this message.
https://breakpad.appspot.com/744002/diff/1/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/744002/diff/1/src/client/ios/Breakpad.mm#newcode514 src/client/ios/Breakpad.mm:514: Thank you. That explanation makes a lot of sense and makes me feel more confident about your code. Still not 100% sure whether I am the best reviewer, but I'd be happy to give you an LGTM -- which might not do you any good, as I don't have any particular privileges for this particular file :-)
Sign in to reply to this message.
I picked you randomly as a reviewer. Although I think this code change is necessary, I would rather have it reviewed by someone who knows the iOS implementation and who has permissions to change it :-) Who would that be? Can you add someone like that.
Sign in to reply to this message.
On 2013/11/15 09:29:43, Akiva wrote: > I picked you randomly as a reviewer. Although I think this code change is > necessary, I would rather have it reviewed by someone who knows the iOS > implementation and who has permissions to change it :-) > > Who would that be? Can you add someone like that. I already added qsr@, since he wrote a lot of this code in the first place.
Sign in to reply to this message.
On 2013/11/16 10:18:37, aurisanchez2 wrote: Guys (gsr, aurisanchez2, etc.), Can we get some movement on this? Please take a look at it.
Sign in to reply to this message.
LGTM (Sorry, was OoO)
Sign in to reply to this message.
On 2013/11/25 16:32:19, qsr wrote: > LGTM (Sorry, was OoO) Good to hear. What is the procedure to get it committed?
Sign in to reply to this message.
On 2013/11/26 10:30:58, Akiva wrote: > On 2013/11/25 16:32:19, qsr wrote: > > LGTM (Sorry, was OoO) > > Good to hear. What is the procedure to get it committed? Just did it for you. I cannot close the review though, you will have to do it yourself.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
