Issue 408002: Add the capability to include an arbitrary data stream within minidumps (Closed)
|
Created:
11 years, 10 months ago by Cris Neckar Modified:
11 years, 9 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdd the capability to include an arbitrary data stream within minidumps This is supplied via a custom field "custom-data-stream" Committed: https://code.google.com/p/google-breakpad/source/detail?r=984 Patch Set 1 #
Total comments: 15
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 4
MessagesTotal messages: 21
nealsid, I want to provide a little bit of context on this. Carlos and I have discussed this and came up with this solution. In general we would like the ability to include an arbitrary stream of application specific data in crash reports. Specifically for Chrome we want to include all of the resource urls for the crashing process instead of just a lazily updated guess at the url that caused the crash. Currently the strategy for including even that url is a hack and really doesn't seem like it is what custom data fields were intended for (given the 64 character limit). It is a bit ugly to pass a pointer and size in a custom data field, but given that we can't extend the IPC between the client and server this seems to be the best way to do it. Thanks
Sign in to reply to this message.
Changing reviewers. Mark, would you mind taking a look at this?
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... File src/client/windows/crash_generation/client_info.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/client_info.cc:217: const_cast<wchar_t*>(&custom_client_info_.entries[i].value[0]); What’s the const_cast about? Can’t address_str just be a const wchar_t*? http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/client_info.cc:230: new u_int8_t[sizeof(CustomDataStream) + size]); Since stream in CustomDataSize is size [1], the sizeof will be +1 from what you’re probably expecting here. Want to subtract it back out? http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... File src/client/windows/crash_generation/minidump_generator.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/minidump_generator.cc:167: // Leave room in user_stream_array for a possible assertion info stream. Revise comment to make clear that this might be more than (or different from) assertion information. http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/minidump_generator.cc:213: user_stream_array[2].Type = MD_CUSTOM_DATA_STREAM; What if !assert_info && custom_data_stream. Is 2 right? I think this should be [user_streams.UserStreamCount] instead of [2].
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... File src/client/windows/crash_generation/client_info.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/client_info.cc:224: LPVOID address = reinterpret_cast<LPVOID>(wcstol(address_str, NULL, 16)); is LPVOID a common pattern in this codebase? http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... File src/client/windows/crash_generation/minidump_generator.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/minidump_generator.cc:63: DWORD process_id, too many parameters now. Consider switching to passing a struct. http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/minidump_generator.cc:174: user_streams.UserStreamCount = 1; I didn't realize breakpad was already using user streams. Cool. Which calls into question the fitness of me doing reviews here but whatever.
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/1/src/client/windows/unittests/minidu... File src/client/windows/unittests/minidump_test.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/unittests/minidu... src/client/windows/unittests/minidump_test.cc:333: } // namespace hmmm. It should be relatively straightforward to write a test here... right?
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... File src/client/windows/crash_generation/client_info.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/client_info.cc:217: const_cast<wchar_t*>(&custom_client_info_.entries[i].value[0]); On 2012/07/09 20:33:46, Mark Mentovai wrote: > What’s the const_cast about? Can’t address_str just be a const wchar_t*? Whoa, I need to copy this buffer. opps http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/client_info.cc:224: LPVOID address = reinterpret_cast<LPVOID>(wcstol(address_str, NULL, 16)); On 2012/07/09 21:10:54, cpu1 wrote: > is LPVOID a common pattern in this codebase? Done. http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/client_info.cc:230: new u_int8_t[sizeof(CustomDataStream) + size]); On 2012/07/09 20:33:46, Mark Mentovai wrote: > Since stream in CustomDataSize is size [1], the sizeof will be +1 from what > you’re probably expecting here. Want to subtract it back out? Done. http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... File src/client/windows/crash_generation/minidump_generator.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/minidump_generator.cc:63: DWORD process_id, On 2012/07/09 21:10:54, cpu1 wrote: > too many parameters now. Consider switching to passing a struct. Hrrm a bunch of things in exception_handler.cc should probably be changed as well. Maybe best done in another patch? http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/minidump_generator.cc:167: // Leave room in user_stream_array for a possible assertion info stream. On 2012/07/09 20:33:46, Mark Mentovai wrote: > Revise comment to make clear that this might be more than (or different from) > assertion information. Done. http://breakpad.appspot.com/408002/diff/1/src/client/windows/crash_generation... src/client/windows/crash_generation/minidump_generator.cc:213: user_stream_array[2].Type = MD_CUSTOM_DATA_STREAM; On 2012/07/09 20:33:46, Mark Mentovai wrote: > What if !assert_info && custom_data_stream. Is 2 right? I think this should be > [user_streams.UserStreamCount] instead of [2]. Done. http://breakpad.appspot.com/408002/diff/1/src/client/windows/unittests/minidu... File src/client/windows/unittests/minidump_test.cc (right): http://breakpad.appspot.com/408002/diff/1/src/client/windows/unittests/minidu... src/client/windows/unittests/minidump_test.cc:333: } // namespace On 2012/07/09 21:11:50, cpu1 wrote: > hmmm. It should be relatively straightforward to write a test here... right? Done.
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... File src/client/windows/crash_generation/client_info.cc (right): http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... src/client/windows/crash_generation/client_info.cc:219: wchar_t* address_str = value; What’s the point of address_str when you already have value, other than maybe being a better name? If you just meant to use value as a buffer and have a few pointers point to things inside it, you might be better served by giving it a more traditional name with “buffer” in it, instead of “value.” http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... src/client/windows/crash_generation/client_info.cc:227: size_t size = wcstol(size_str, NULL, 16); I might be slightly more comfortable calling “size” a “long” which is what wcstol gives you anyway, and then, since it’s signed, making sure that it’s > 0 instead of just != 0. (This doesn’t need to deal with 64-bit code because the rest of Windows Breakpad doesn’t work with 64-bit crash processes, does it?) http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... File src/client/windows/crash_generation/crash_generation_server.cc (right): http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... src/client/windows/crash_generation/crash_generation_server.cc:896: client.custom_data_stream(), OK with me to defer this to a subsequent change. I don’t really care if this doesn’t move into a struct either, for what it’s worth.
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... File src/client/windows/crash_generation/client_info.cc (right): http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... src/client/windows/crash_generation/client_info.cc:219: wchar_t* address_str = value; On 2012/07/09 22:33:01, Mark Mentovai wrote: > What’s the point of address_str when you already have value, other than maybe > being a better name? If you just meant to use value as a buffer and have a few > pointers point to things inside it, you might be better served by giving it a > more traditional name with “buffer” in it, instead of “value.” Done. http://breakpad.appspot.com/408002/diff/7002/src/client/windows/crash_generat... src/client/windows/crash_generation/client_info.cc:227: size_t size = wcstol(size_str, NULL, 16); On 2012/07/09 22:33:01, Mark Mentovai wrote: > I might be slightly more comfortable calling “size” a “long” which is what > wcstol gives you anyway, and then, since it’s signed, making sure that it’s > 0 > instead of just != 0. > > (This doesn’t need to deal with 64-bit code because the rest of Windows Breakpad > doesn’t work with 64-bit crash processes, does it?) Makes sense.
Sign in to reply to this message.
Breakpad works in 64 bits. Notice GoogleCrashHandler64.exe eating cpu in your windows machine as we speak. It was done for NaCL. The struct refactoring, I am fine if mm is fine with it.
Sign in to reply to this message.
On 2012/07/09 23:46:32, cpu1 wrote: > Breakpad works in 64 bits. Notice GoogleCrashHandler64.exe eating cpu in your > windows machine as we speak. It was done for NaCL. > > The struct refactoring, I am fine if mm is fine with it. The max size for this is already well below 32bits so I think this can be a long (especially since I'm parsing with wcstol() anyway). The worst thing that can really happen here is we get garbage in the custom stream or the ReadProcessMemory() fails and either way that is probably fine. Parsing the pointer as a long though won't work on 64 bit. I guess we could just force a 64 bit process to map this data in low memory (again failure to do this would just result in garbage in the stream or failure). I think I would prefer this to trying to handle reading a 64bit pointer as high and low dwords or something. What do you think Mark?
Sign in to reply to this message.
On Mon, Jul 9, 2012 at 6:33 PM, <mark@chromium.org> wrote: > (This doesn’t need to deal with 64-bit code because the rest of Windows > Breakpad doesn’t work with 64-bit crash processes, does it?) Breakpad works fine on Win64, FWIW (except for stack unwind info in symbol dumping). -Ted
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/13004/src/client/windows/crash_genera... File src/client/windows/crash_generation/client_info.cc (right): http://breakpad.appspot.com/408002/diff/13004/src/client/windows/crash_genera... src/client/windows/crash_generation/client_info.cc:225: void* address = reinterpret_cast<void*>(wcstol(address_str, NULL, 16)); If you want to handle something wider than long: _wcstoi64?
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/13004/src/client/windows/crash_genera... File src/client/windows/crash_generation/client_info.cc (right): http://breakpad.appspot.com/408002/diff/13004/src/client/windows/crash_genera... src/client/windows/crash_generation/client_info.cc:225: void* address = reinterpret_cast<void*>(wcstol(address_str, NULL, 16)); On 2012/07/10 13:01:56, Mark Mentovai wrote: > If you want to handle something wider than long: _wcstoi64? Ah, that's what I was looking for thanks.
Sign in to reply to this message.
http://breakpad.appspot.com/408002/diff/1010/src/google_breakpad/common/minid... File src/google_breakpad/common/minidump_format.h (right): http://breakpad.appspot.com/408002/diff/1010/src/google_breakpad/common/minid... src/google_breakpad/common/minidump_format.h:335: MD_CUSTOM_DATA_STREAM = 0x47670003 /* MDRawCustomDataStream */ FYI, this overlaps with the Linux-specific stream types: http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/linux... I have a patch that I just wrote recently that moves those into minidump_format.h to make that less confusing. (I was looking at adding another stream type and hit the same issue.)
Sign in to reply to this message.
On 2012/07/13 00:55:51, Ted Mielczarek wrote: > http://breakpad.appspot.com/408002/diff/1010/src/google_breakpad/common/minid... > File src/google_breakpad/common/minidump_format.h (right): > > http://breakpad.appspot.com/408002/diff/1010/src/google_breakpad/common/minid... > src/google_breakpad/common/minidump_format.h:335: MD_CUSTOM_DATA_STREAM > = 0x47670003 /* MDRawCustomDataStream */ > FYI, this overlaps with the Linux-specific stream types: > http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/linux... > > I have a patch that I just wrote recently that moves those into > minidump_format.h to make that less confusing. (I was looking at adding another > stream type and hit the same issue.) Ack good catch. Feel free to change value for this in your patch. Nothing uses this code yet so it should be safe.
Sign in to reply to this message.
I’m trying to figure out why we’re having trouble rolling Breakpad DEPS to r995 or so. http://build.chromium.org/p/chromium/builders/NACL%20Tests/builds/30138/steps... shows failures, with an indication (per native_client/tests/inbrowser_crash_test/crash_dump_tester.py) that crash_service itself is crashing. I don’t think that this change is responsible, although I’d appreciate a second set of eyes on the matter. I did find this new[]/delete confusion problem, which I doubt is responsible. Nothing is using custom-data-stream yet, right? https://breakpad.appspot.com/408002/diff/1010/src/client/windows/crash_genera... File src/client/windows/crash_generation/client_info.cc (right): https://breakpad.appspot.com/408002/diff/1010/src/client/windows/crash_genera... src/client/windows/crash_generation/client_info.cc:93: delete custom_data_stream_; This was allocated with new[]. Shouldn’t you have used delete[]? Line 236 as well.
Sign in to reply to this message.
This change had to be backed out at r999. It caused Chrome test failures on the NACL Tests bots. For example: http://build.chromium.org/p/chromium/builders/NACL%20Tests/builds/30190 http://build.chromium.org/p/chromium/builders/NACL%20Tests%20%28x64%29/builds... native_client/tests/inbrowser_crash_test/crash_dump_tester.py failed (output captured below). The comments in the script indicate that when it fails in this way, it’s because crash_service.exe itself has crashed. After a few speculative back-outs, we found that backing out this change made the NACL Tests start passing again. On a subsequent re-review, I found the new[]/delete confusion I mentioned yesterday. When this is re-committed, that should be fixed for sure. I doubt that was the cause of the problem, because I didn’t think this new code was being hit yet, so it should be carefully checked to make sure it’s safe before being relanded, and when it is relanded, we’ll ideally roll it into Chrome alone to isolate it from any other changes. Test failure output follows. -- Func(["scons-out\nacl-x86-32\obj\tests\inbrowser_crash_test\inbrowser_trusted_crash_in_startup_test.out.tmp\inbrowser_crash_test.nmf"], []) E:\b\depot_tools\python_bin\python_slave.exe E:\b\build\slave\NACL_Tests\build\src\native_client\tools\command_tester.py --name nacl_newlib.run_inbrowser_trusted_crash_in_startup_test --report E:\b\build\slave\NACL_Tests\build\src\native_client\scons-out\nacl-x86-32/test_results/inbrowser_trusted_crash_in_startup_test.out --time_warning 1800 --time_error 18000 --perf_env_description x86-32_nnacl_newlib_static --subarch 32 --osenv NACL_CRASH_TEST=1 --arch x86 --capture_output 0 E:\b\depot_tools\python_bin\python_slave.exe E:\b\build\slave\NACL_Tests\build\src\native_client\tests\inbrowser_crash_test\crash_dump_tester.py --browser_path E:\b\build\slave\NACL_Tests\build\src\build\Release\chrome.exe --url trusted_crash_in_startup.html --timeout 30 --file E:\b\build\slave\NACL_Tests\build\src\native_client\scons-out\nacl-x86-32\obj\tests\inbrowser_crash_test\inbrowser_crash_test_x86-32.nexe --file E:\b\build\slave\NACL_Tests\build\src\native_client\tests\inbrowser_crash_test\trusted_crash_in_startup.html --serving_dir E:\b\build\slave\NACL_Tests\build\src\native_client\toolchain\win_x86_newlib/x86_64-nacl\lib32 --serving_dir E:\b\build\slave\NACL_Tests\build\src\native_client\scons-out\nacl-x86-32/lib --map_file inbrowser_crash_test.nmf E:\b\build\slave\NACL_Tests\build\src\native_client\scons-out\nacl-x86-32\obj\tests\inbrowser_crash_test\inbrowser_trusted_crash_in_startup_test.out.tmp\inbrowser_crash_test.nmf --expected_crash_dumps=1 [ RUN ] nacl_newlib.run_inbrowser_trusted_crash_in_startup_test ====================================================================== setting environment ====================================================================== [NACL_CRASH_TEST] = [1] ====================================================================== running ['E:\\b\\depot_tools\\python_bin\\python_slave.exe', 'E:\\b\\build\\slave\\NACL_Tests\\build\\src\\native_client\\tests\\inbrowser_crash_test\\crash_dump_tester.py', '--browser_path', 'E:\\b\\build\\slave\\NACL_Tests\\build\\src\\build\\Release\\chrome.exe', '--url', 'trusted_crash_in_startup.html', '--timeout', '30', '--file', 'E:\\b\\build\\slave\\NACL_Tests\\build\\src\\native_client\\scons-out\\nacl-x86-32\\obj\\tests\\inbrowser_crash_test\\inbrowser_crash_test_x86-32.nexe', '--file', 'E:\\b\\build\\slave\\NACL_Tests\\build\\src\\native_client\\tests\\inbrowser_crash_test\\trusted_crash_in_startup.html', '--serving_dir', 'E:\\b\\build\\slave\\NACL_Tests\\build\\src\\native_client\\toolchain\\win_x86_newlib/x86_64-nacl\\lib32', '--serving_dir', 'E:\\b\\build\\slave\\NACL_Tests\\build\\src\\native_client\\scons-out\\nacl-x86-32/lib', '--map_file', 'inbrowser_crash_test.nmf', 'E:\\b\\build\\slave\\NACL_Tests\\build\\src\\native_client\\scons-out\\nacl-x86-32\\obj\\tests\\inbrowser_crash_test\\inbrowser_trusted_crash_in_startup_test.out.tmp\\inbrowser_crash_test.nmf', '--expected_crash_dumps=1'] ====================================================================== NACL_CRASH_TEST=1 E:\b\depot_tools\python_bin\python_slave.exe E:\b\build\slave\NACL_Tests\build\src\native_client\tests\inbrowser_crash_test\crash_dump_tester.py --browser_path E:\b\build\slave\NACL_Tests\build\src\build\Release\chrome.exe --url trusted_crash_in_startup.html --timeout 30 --file E:\b\build\slave\NACL_Tests\build\src\native_client\scons-out\nacl-x86-32\obj\tests\inbrowser_crash_test\inbrowser_crash_test_x86-32.nexe --file E:\b\build\slave\NACL_Tests\build\src\native_client\tests\inbrowser_crash_test\trusted_crash_in_startup.html --serving_dir E:\b\build\slave\NACL_Tests\build\src\native_client\toolchain\win_x86_newlib/x86_64-nacl\lib32 --serving_dir E:\b\build\slave\NACL_Tests\build\src\native_client\scons-out\nacl-x86-32/lib --map_file inbrowser_crash_test.nmf E:\b\build\slave\NACL_Tests\build\src\native_client\scons-out\nacl-x86-32\obj\tests\inbrowser_crash_test\inbrowser_trusted_crash_in_startup_test.out.tmp\inbrowser_crash_test.nmf --expected_crash_dumps=1 ENV: TMP=C:\DOCUME~1\CHROME~2\LOCALS~1\Temp NACL_CRASH_TEST=1 PROMPT=$P$G COMSPEC=C:\WINDOWS\system32\cmd.exe TEMP=C:\DOCUME~1\CHROME~2\LOCALS~1\Temp SYSTEMROOT=C:\WINDOWS SYSTEMDRIVE=C: PATH=E:\b\build\slave\NACL_Tests\build\src\native_client\toolchain\win_x86_newlib\bin;C:\WINDOWS\System32 NACL_ENABLE_PPAPI_DEV=1 CYGWIN=nodosfilewarning CHROME_HEADLESS=1 PATHEXT=.COM;.EXE;.BAT;.CMD CHROME_BREAKPAD_PIPE_NAME=\\.\pipe\nacl_crash_dump_tester_cqsr5c_crash_service LAUNCHING: E:\b\build\slave\NACL_Tests\build\src\build\Release\chrome.exe --disable-web-resources --disable-preconnect --no-first-run --no-default-browser-check --enable-logging --log-level=1 --safebrowsing-disable-auto-update --explicitly-allowed-ports=3347 --user-data-dir=c:\docume~1\chrome~2\locals~1\temp\browserprofile_4z66fi --log-net-log=c:\docume~1\chrome~2\locals~1\temp\browserprofile_4z66fi\netlog.json --enable-nacl http://192.168.111.3:3347/trusted_crash_in_startup.html [2840:3456:2161885328:ERROR:gpu_info_collector_win.cc(93)] Can't retrieve a valid WinSAT assessment. [2840:3156:2161886625:ERROR:nacl_process_host.cc(162)] NaCl process exited with status -1073741819 (0xc0000005) PID 2840 [ 546 ms] |||| GET /trusted_crash_in_startup.html (/trusted_crash_in_startup.html) [ 562 ms] |||| GET /nacltest.js (/nacltest.js) [ 592 ms] |||| [STARTUP] [ 608 ms] |||| [ 625 ms] |||| [trusted_crash_during_startup BEGIN] [ 750 ms] |||| Registering ForcePluginLoadOnTimeout (Bugs: NaCl 2428, Chrome 103588) [ 764 ms] |||| GET /favicon.ico (/favicon.ico) [ 952 ms] |||| GET /inbrowser_crash_test.nmf (/inbrowser_crash_test.nmf) [ 1092 ms] |||| GET /inbrowser_crash_test_x86-32.nexe (/inbrowser_crash_test_x86-32.nexe) [ 1498 ms] |||| [trusted_crash_during_startup PASS] [ 1514 ms] |||| [ 1514 ms] |||| [SHUTDOWN] 1 passed, 0 failed, 0 errors KILLING the browser ERROR: Command returned exit status 1 (0x1) but we expected 0 (0x0) [ FAILED ] nacl_newlib.run_inbrowser_trusted_crash_in_startup_test (453 ms) Traceback (most recent call last): File "E:\b\build\slave\NACL_Tests\build\src\native_client\tests\inbrowser_crash_test\crash_dump_tester.py", line 165, in <module> sys.exit(Main()) File "E:\b\build\slave\NACL_Tests\build\src\native_client\tests\inbrowser_crash_test\crash_dump_tester.py", line 119, in Main cleanup_func() File "E:\b\build\slave\NACL_Tests\build\src\native_client\tests\inbrowser_crash_test\crash_dump_tester.py", line 63, in Cleanup proc.terminate() File "E:\b\depot_tools\python_bin\lib\subprocess.py", line 911, in terminate TerminateProcess(self._handle, 1) WindowsError: [Error 5] Access is denied scons: *** [scons-out\nacl-x86-32\test_results\inbrowser_trusted_crash_in_startup_test.out] Error 1 --
Sign in to reply to this message.
Just a drive-by review based on Mark's comment about new[]/delete... https://breakpad.appspot.com/408002/diff/1010/src/client/windows/crash_genera... File src/client/windows/crash_generation/client_info.cc (right): https://breakpad.appspot.com/408002/diff/1010/src/client/windows/crash_genera... src/client/windows/crash_generation/client_info.cc:93: delete custom_data_stream_; On 2012/07/24 21:25:03, Mark Mentovai wrote: > This was allocated with new[]. Shouldn’t you have used delete[]? Line 236 as > well. Also, this is delete'ing it as a CustomDataStream, but it was allocated as an array of u_int8_t's. Since CustomDataStream is POD, that might not be causing damage (i.e. no destructor) but it's definitely not good to do. https://breakpad.appspot.com/408002/diff/1010/src/client/windows/crash_genera... src/client/windows/crash_generation/client_info.cc:231: new u_int8_t[sizeof(CustomDataStream) + size - 1]); Technically, I would add a placement new here (e.g. "new (custom_data_stream) CustomDataStream;"), but if there's zero chance CustomDataStream will become non-POD some day then that might be considered overkill.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
