Get correct version info from registry [crashpad/crashpad : main]
Bruce Dawson (Gerrit)
Attention is currently required from: Joshua Peraza.
Patch set 2:-Commit-Queue
1 comment:
Patchset:
kernel32.dll is no longer a good way to get Windows version information - the registry is the recommended alternative. Any thoughts on this?
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Attention is currently required from: Bruce Dawson.
4 comments:
Patchset:
Otherwise LGTM!
File snapshot/win/system_snapshot_win.cc:
is to query the registry?
Patch Set #2, Line 120:
= ERROR_SUCCESS) {needs formatting
wchar_t reg_build_number[100];
DWORD type;
DWORD size = sizeof(reg_build_number);
if (RegQueryValueEx(key, L"CurrentBuildNumber", nullptr, &type, reinterpret_cast<BYTE*>(®_build_number), &size) == ERROR_SUCCESS &&
type == REG_SZ) {
unsigned current_build_number;
if (swscanfDocumentation suggests ReqQueryValueEx() doesn't guarantee null-termination: https://docs.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regqueryvalueexw
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Bruce Dawson.
10 comments:
File snapshot/win/system_snapshot_win.cc:
Patch Set #2, Line 114:
// kernel32.dll used to be a good way to get a non-lying version number, butYou still trust it for major/minor, though? Do those appear anywhere reliable in the registry?
Therefore the
// recommended way to get the os_version_bugfix_ and os_version_build_
// numbers.I think this sentence is missing something, like “the registry”?
Patch Set #2, Line 120:
if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion", 0, KEY_QUERY_VALUE, &key) == ERROR_SUCCESS) {80 columns (git cl format?)
Also on lines 127 and 139.
Patch Set #2, Line 120:
L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"Does this key (and the two values you’re looking up) exist and is it reliable as far back as our minimum supported Windows version?
Extra {scope} doesn’t seem necessary. Same on line 135.
Patch Set #2, Line 124:
wchar_t reg_build_number[100];
Since you’re expecting an unsigned, size this at [11] and let RegQueryValueEx return an error if the value doesn’t fit.
Patch Set #2, Line 130:
if (swscanf(reg_build_number, L"%u", ¤t_build_number) == 1)#include "util/stdlib/string_number_conversion.h" and use StringToNumber instead.
Patch Set #2, Line 131:
os_version_bugfix_ = current_build_number;Since the stuff above was sorted most-to-least significant, putting os_version_build_ before os_version_bugfix_, I think this should follow suit.
Patch Set #2, Line 139:
if (RegQueryValueEx(key, L"UBR", nullptr, &type, reinterpret_cast<BYTE*>(®_ubr), &size2) == ERROR_SUCCESS &&Can you expand “UBR” in a comment?
Patch Set #2, Line 141:
os_version_build_ = base::StringPrintf("%lu", reg_ubr);Funny, the thing we wanted as a number came as REG_SZ, and the thing we wanted as a string came as REG_DWORD. :)
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Bruce Dawson (Gerrit)
Attention is currently required from: Joshua Peraza, Mark Mentovai.
13 comments:
Patchset:
Okay, patch updated. Three issues are responded to without necessarily being finished, so comments are appreciated.
File snapshot/win/system_snapshot_win.cc:
Patch Set #2, Line 114:
// kernel32.dll used to be a good way to get a non-lying version number, butYou still trust it for major/minor, though? Do those appear anywhere reliable in the registry?
That is the obvious question: should we get all of it from the registry? In that registry directory we have:
CurrentMajorVersionNumber.CurrentMinorVersionNumber.CurrentBuildNumber.UBR
So, yes, it is all available, on Windows 10. These are all stored as REG_DWORD except for CurrentBuildNumber - odd.
However the first two numbers are not in the registry in that form on Windows 7, so sticking with kernel32.dll to get them makes sense, I think.
Therefore the
// recommended way to get the os_version_bugfix_ and os_version_build_
// numbers.I think this sentence is missing something, like “the registry”?
Done
is to query the registry?
Done
Patch Set #2, Line 120:
if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion", 0, KEY_QUERY_VALUE, &key) == ERROR_SUCCESS) {
80 columns (git cl format?) […]
Whoops! I'm so used to the "git cl upload" format reminders that I completely depend on them, and I didn't get one from the crashpad repo. Fixed.
Patch Set #2, Line 120:
= ERROR_SUCCESS) {needs formatting
Done
Patch Set #2, Line 120:
L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"
Does this key (and the two values you’re looking up) exist and is it reliable as far back as our min […]
Excellent question. I just checked on Windows 7 and it has CurrentBuildNumber and UBR, but it lacks the other two. The odd thing is that UBR is 24136 in the registry, but kernel32.dll (64-bit version) is 6.1.7601.24384. That is, in this case kernel32.dll is more recent than what the registry says. winver doesn't list the last version number in this case, it just says "Version 6.1 (Build 7601: Service Pack 1)"
In this case of Windows 7 the motivating issue (kernel32.dll not being updated on all updates) is less likely so we could skip the registry reads on Windows 7, or take the larger of the two numbers. I think max() makes sense.
Extra {scope} doesn’t seem necessary. Same on line 135.
I put the extra scope in because while experimenting with this it was easy to accidentally pass the wrong variable address, and that problem goes away with the scopes. That said, I'll remove them since they aren't as important once the code is working.
Patch Set #2, Line 124:
wchar_t reg_build_number[100];
Since you’re expecting an unsigned, size this at [11] and let RegQueryValueEx return an error if the […]
Done
wchar_t reg_build_number[100];
DWORD type;
DWORD size = sizeof(reg_build_number);
if (RegQueryValueEx(key, L"CurrentBuildNumber", nullptr, &type, reinterpret_cast<BYTE*>(®_build_number), &size) == ERROR_SUCCESS &&
type == REG_SZ) {
unsigned current_build_number;
if (swscanf
Documentation suggests ReqQueryValueEx() doesn't guarantee null-termination: https://docs.microsoft. […]
Done
Patch Set #2, Line 130:
if (swscanf(reg_build_number, L"%u", ¤t_build_number) == 1)#include "util/stdlib/string_number_conversion.h" and use StringToNumber instead.
It turns out this is messy because RegQueryValueEx returns WCHAR data and StringToNumber wants char. RegQueryValueExA works, but it feels like choosing between two imperfect solutions. This patch has both options, with #ifdefs. Opinions welcomed.
Patch Set #2, Line 131:
os_version_bugfix_ = current_build_number;
Since the stuff above was sorted most-to-least significant, putting os_version_build_ before os_vers […]
os_version_build_ is the least significant, so the order is correct I believe.
Patch Set #2, Line 139:
if (RegQueryValueEx(key, L"UBR", nullptr, &type, reinterpret_cast<BYTE*>(®_ubr), &size2) == ERROR_SUCCESS &&Can you expand “UBR” in a comment?
Apparently it us Update Build Revision. Go figure.
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Joshua Peraza, Bruce Dawson.
9 comments:
Patchset:
Thanks for the discussion. Along with https://crbug.com/1248324 and https://groups.google.com/a/google.com/g/chrome-stability/c/vSARwM5KVt0, I think it helps illuminate the path forward.
A bit of history: this Windows-version-comes-from-kernel32.dll thing goes way back to https://codereview.chromium.org/936333004, where I asked “what if we’re not manifested?” and this was the outcome.
File snapshot/win/system_snapshot_win.cc:
Patch Set #2, Line 114:
// kernel32.dll used to be a good way to get a non-lying version number, but
That is the obvious question: should we get all of it from the registry? In that registry directory we have:
CurrentMajorVersionNumber.CurrentMinorVersionNumber.CurrentBuildNumber.UBR
So, yes, it is all available, on Windows 10. These are all stored as REG_DWORD except for CurrentBuildNumber - odd.
However the first two numbers are not in the registry in that form on Windows 7, so sticking with kernel32.dll to get them makes sense, I think.
I think that the right move is:
If all four are in the registry, just use them, and forget about kernel32.dll (for reasons described in https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3434090/comments/60d61093_5cd90012).
If fewer than four are in the registry, kernel32.dll is probably trustworthy, so just use it.
Under this approach, there should be no need to try to reconcile the two data sources.
Patch Set #2, Line 120:
L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"
Excellent question. I just checked on Windows 7 and it has CurrentBuildNumber and UBR, but it lacks the other two. The odd thing is that UBR is 24136 in the registry, but kernel32.dll (64-bit version) is 6.1.7601.24384. That is, in this case kernel32.dll is more recent than what the registry says. winver doesn't list the last version number in this case, it just says "Version 6.1 (Build 7601: Service Pack 1)"
In this case of Windows 7 the motivating issue (kernel32.dll not being updated on all updates) is less likely so we could skip the registry reads on Windows 7, or take the larger of the two numbers. I think max() makes sense.
At first, max seems somewhat reasonable, but specifically, how? Max of each component? Probably wrong on its face. Max of, in effect, the 128-bit number created by merging all four components? Seems more right. Actually, I think both may be wrong. In light of your thread at https://groups.google.com/a/google.com/g/chrome-stability/c/vSARwM5KVt0 and my response, I wonder: what happens if you experience the “Windows 10 problem” and the “Windows 11 problem” simultaneously? It seems that the kernel32.dll version number has only a loose connection to reality for at least two reasons now. Maybe it’s better to not consult it at all when something more authoritative exists.
Patch Set #2, Line 130:
if (swscanf(reg_build_number, L"%u", ¤t_build_number) == 1)
It turns out this is messy because RegQueryValueEx returns WCHAR data and StringToNumber wants char. RegQueryValueExA works, but it feels like choosing between two imperfect solutions. This patch has both options, with #ifdefs. Opinions welcomed.
Ooh, I don’t like either of those! I guess I have a slight preference for RegQueryValueExA, but between these two, it’s not so strong that I’d insist. Your call.
Patch Set #2, Line 131:
os_version_bugfix_ = current_build_number;
os_version_build_ is the least significant, so the order is correct I believe.
Yes, I must have misread.
File snapshot/win/system_snapshot_win.cc:
Patch Set #3, Line 147:
type == REG_SZ) {As you point out: weird that this isn’t REG_DWORD! Are we “confident” that this will remain REG_SZ? As in, are others relying on this to the point that Microsoft’s probably bound to leave it alone?
Patch Set #3, Line 149:
reg_build_number[size / sizeof(reg_build_number[0])] = 0;We like char literals to be typed in this code, so if you’re using the char form, use `'\0'`, and if wchar_t, `L'\0'`.
Patch Set #3, Line 160:
DWORD size2 = sizeof(reg_ubr);You don’t need a new variable for this, you can recycle `size`.
On Windows 10 and above it is common for a monthly update to ship,
// thus increasing the value of the UBR key, without updating
// kernel32.dllBased on your description, I thought that this form of the problem applied to Windows 11, not Windows 10.
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Bruce Dawson (Gerrit)
Attention is currently required from: Joshua Peraza, Mark Mentovai.
3 comments:
File snapshot/win/system_snapshot_win.cc:
Patch Set #2, Line 114:
// kernel32.dll used to be a good way to get a non-lying version number, but
> That is the obvious question: should we get all of it from the registry? In that registry director […]
I think that makes a lot of sense. I'll do that.
File snapshot/win/system_snapshot_win.cc:
Patch Set #3, Line 147:
type == REG_SZ) {
As you point out: weird that this isn’t REG_DWORD! Are we “confident” that this will remain REG_SZ? […]
I'm pretty confident, but ultimately predicting the future is difficult. If they change this then we'll find out about it quickly enough.
On Windows 10 and above it is common for a monthly update to ship,
// thus increasing the value of the UBR key, without updating
// kernel32.dll
Based on your description, I thought that this form of the problem applied to Windows 11, not Window […]
I think that in theory it could happen on Windows 10. I don't know if it has happened in practice. I'll word-smith the comment a bit.
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Bruce Dawson (Gerrit)
Attention is currently required from: Joshua Peraza, Mark Mentovai.
8 comments:
Patchset:
After a productive discussion I think the code is in good shape now. PTAL.
File snapshot/win/system_snapshot_win.cc:
Patch Set #2, Line 114:
// kernel32.dll used to be a good way to get a non-lying version number, but
I think that makes a lot of sense. I'll do that.
Done
Patch Set #2, Line 120:
L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"
> Excellent question. […]
Now that I've switched to getting version information from either the registry _or_ kernel32.dll this issue goes away.
Patch Set #2, Line 130:
if (swscanf(reg_build_number, L"%u", ¤t_build_number) == 1)
> It turns out this is messy because RegQueryValueEx returns WCHAR data and StringToNumber wants cha […]
Done
File snapshot/win/system_snapshot_win.cc:
Patch Set #3, Line 147:
type == REG_SZ) {
I'm pretty confident, but ultimately predicting the future is difficult. […]
Done
Patch Set #3, Line 149:
reg_build_number[size / sizeof(reg_build_number[0])] = 0;
We like char literals to be typed in this code, so if you’re using the char form, use `'\0'`, and if […]
Done
Patch Set #3, Line 160:
DWORD size2 = sizeof(reg_ubr);You don’t need a new variable for this, you can recycle `size`.
Done
On Windows 10 and above it is common for a monthly update to ship,
// thus increasing the value of the UBR key, without updating
// kernel32.dll
I think that in theory it could happen on Windows 10. I don't know if it has happened in practice. […]
The code got refactored enough that this comment disappeared, but the replacement should be fine.
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Attention is currently required from: Bruce Dawson, Mark Mentovai.
Patch set 5:Code-Review +1
1 comment:
File snapshot/win/system_snapshot_win.cc:
Patch Set #5, Line 140:
crashpad::We're already in crashpad's namespace.
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Bruce Dawson.
6 comments:
File snapshot/win/system_snapshot_win.cc:
Patch Set #5, Line 75:
bool ReadValueDW(HKEY key, const wchar_t* name, int* out_value) {Name this so that it’s clear at the call site that it’s talking to the registry, like ReadRegistryDWORD.
Strike “as” now.
Patch Set #5, Line 143:
REGSZREG_SZ
char reg_build_number[11];
DWORD type;
// Leave space for a terminating zero.
DWORD size = sizeof(reg_build_number) - sizeof(reg_build_number[0]);
// Use the 'A' version of this function so that we can use
// StringToNumber.
if (RegQueryValueExA(key,
"CurrentBuildNumber",
nullptr,
&type,
reinterpret_cast<BYTE*>(®_build_number),
&size) == ERROR_SUCCESS &&
type == REG_SZ) {
// Make sure the string is null-terminated.
reg_build_number[size / sizeof(reg_build_number[0])] = '\0';
unsigned current_build_number;
if (StringToNumber(reg_build_number, ¤t_build_number)) {Why don’t you refactor this whole thing into something like ReadRegistryDWORDFromSZ? Then you could write this whole thing as `bool version_data_found = Read(…) && Read(…) && Read(…) && Read(…);`.
std::string flags_string = GetStringForFileFlags(ffi.dwFileFlags);
std::string os_name = GetStringForFileOS(ffi.dwFileOS);Is there a registry source that we could use for these now too?
%u to match what was here before and to match the way the rest will be formatted in the following statement.
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Bruce Dawson (Gerrit)
Attention is currently required from: Mark Mentovai.
8 comments:
Patchset:
The CQ DRY RUN keeps failing and I'm not sure why - it seems unrelated. The error is in snapshot/win/end_to_end_test.py where the winext\uext.dll fails to load which then means that !peb is not available. It feels like an environmental issue. Any thoughts? Here is the relevant debug spew:
Unable to add extension DLL: uext
The call to LoadLibrary(ext) failed, Win32 error 0n2
"The system cannot find the file specified."
Please check your debugger configuration and/or network access.
This dump file has an exception of interest stored in it.
The stored exception information can be accessed via .ecxr.
(e64.524): Access violation - code c0000005 (first/second chance not available)
*** WARNING: Unable to verify checksum for ntdll.dll
*** WARNING: Unable to verify checksum for KERNELBASE.dll
eax=00000000 ebx=0096ec64 ecx=ff896b59 edx=00000000 esi=00000000 edi=0096eed0
eip=779e205c esp=0096ec24 ebp=0096ec88 iopl=0 nv up ei pl nz na pe nc
cs=0023 ss=002b ds=002b es=002b fs=0053 gs=002b efl=00000206
ntdll!NtDelayExecution+0xc:
779e205c c20800 ret 8
0:000> cdb: Reading initial command '!peb;q'
The call to LoadLibrary(ext) failed, Win32 error 0n2
"The system cannot find the file specified."
Please check your debugger configuration and/or network access.
No export peb found
quit:
File snapshot/win/system_snapshot_win.cc:
Patch Set #5, Line 75:
bool ReadValueDW(HKEY key, const wchar_t* name, int* out_value) {
Name this so that it’s clear at the call site that it’s talking to the registry, like ReadRegistryDW […]
Done
Strike “as” now.
Done, although I felt I needed to add "so" later in the sentence to make it work.
Patch Set #5, Line 140:
crashpad::We're already in crashpad's namespace.
Done
Patch Set #5, Line 143:
REGSZREG_SZ
Done
char reg_build_number[11];
DWORD type;
// Leave space for a terminating zero.
DWORD size = sizeof(reg_build_number) - sizeof(reg_build_number[0]);
// Use the 'A' version of this function so that we can use
// StringToNumber.
if (RegQueryValueExA(key,
"CurrentBuildNumber",
nullptr,
&type,
reinterpret_cast<BYTE*>(®_build_number),
&size) == ERROR_SUCCESS &&
type == REG_SZ) {
// Make sure the string is null-terminated.
reg_build_number[size / sizeof(reg_build_number[0])] = '\0';
unsigned current_build_number;
if (StringToNumber(reg_build_number, ¤t_build_number)) {
Why don’t you refactor this whole thing into something like ReadRegistryDWORDFromSZ? Then you could […]
Yeah, that does work better. It hadn't occurred to me to make a function that was called once, but it works very well.
std::string flags_string = GetStringForFileFlags(ffi.dwFileFlags);
std::string os_name = GetStringForFileOS(ffi.dwFileOS);Is there a registry source that we could use for these now too?
Not that I can find. However:
dwFileFlags is typically zero, which converts to an empty string. It's not clear if we would lose much by hard-coding that.
GetStringForFileOS is practically hard-coded to "Windows NT" - the only other option is "Unknown".So, I don't know if there is any value in the GetStringForFileFlags() function.
%u to match what was here before and to match the way the rest will be formatted in the following st […]
Done
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Bruce Dawson.
5 comments:
Patchset:
The CQ DRY RUN keeps failing and I'm not sure why - it seems unrelated. The error is in snapshot/win/end_to_end_test.py where the winext\uext.dll fails to load which then means that !peb is not available. It feels like an environmental issue. Any thoughts? Here is the relevant debug spew:
Unable to add extension DLL: uext
The call to LoadLibrary(ext) failed, Win32 error 0n2
"The system cannot find the file specified."
Please check your debugger configuration and/or network access.
This dump file has an exception of interest stored in it.
The stored exception information can be accessed via .ecxr.
(e64.524): Access violation - code c0000005 (first/second chance not available)
*** WARNING: Unable to verify checksum for ntdll.dll
*** WARNING: Unable to verify checksum for KERNELBASE.dll
eax=00000000 ebx=0096ec64 ecx=ff896b59 edx=00000000 esi=00000000 edi=0096eed0
eip=779e205c esp=0096ec24 ebp=0096ec88 iopl=0 nv up ei pl nz na pe nc
cs=0023 ss=002b ds=002b es=002b fs=0053 gs=002b efl=00000206
ntdll!NtDelayExecution+0xc:
779e205c c20800 ret 8
0:000> cdb: Reading initial command '!peb;q'
The call to LoadLibrary(ext) failed, Win32 error 0n2
"The system cannot find the file specified."
Please check your debugger configuration and/or network access.
No export peb found
quit:
I don’t know, the win_toolchain was touched somewhat recently in https://chromium-review.googlesource.com/c/3339382 (finally stuck the landing on the eighth try), but all tests passed when it landed. The waterfall at https://ci.chromium.org/p/crashpad/g/main/ looks clean. The Windows try builders started showing this yesterday afternoon (https://ci.chromium.org/p/crashpad/builders/try/crashpad_win_x64_dbg build 358 at 2022-02-09 21:17:25Z, https://ci.chromium.org/p/crashpad/builders/try/crashpad_win_x64_rel build 348 at 2022-02-09 21:23:26Z) but only on this change. I see that you fed https://chromium-review.googlesource.com/c/3449168/1 to them and they liked it, so it does seem to be related to what you’re doing here. Are you able to reproduce locally?
I suppose that I’m not convinced that `"The system cannot find the file specified."` is related, although you can speak more to that. If you’re certain that `!peb` doesn’t work unless `uext.dll` can be loaded, then I’ll believe you, but `!peb` isn’t listed at https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/user-mode-extensions, it appears in “General Extensions” which I think is `exts.dll` (https://docs.microsoft.com/en-us/windows-hardware/drivers/debugger/-peb). We may be seeing that string appear every time we use `cdb`, but because we’re matching permissively, it may not be bothering us provided that we do find the output we’re looking for.
In that case, it would seem that there really is something badly-structured about the minidumps that this change produces.
It seems like your last good try run was patch set 5, and your first bad one was patch set 6, so something in https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3434090/5..6/snapshot/win/system_snapshot_win.cc?
Maybe this is caused by the implausible garbage that’s accidentally winding up in `os_version_bugfix_`, since that bug was introduced in patch set 5→6.
File snapshot/win/system_snapshot_win.cc:
std::string flags_string = GetStringForFileFlags(ffi.dwFileFlags);
std::string os_name = GetStringForFileOS(ffi.dwFileOS);
Not that I can find. However:
dwFileFlags is typically zero, which converts to an empty string. It's not clear if we would lose much by hard-coding that.
GetStringForFileOS is practically hard-coded to "Windows NT" - the only other option is "Unknown".
So, I don't know if there is any value in the GetStringForFileFlags() function.
OK to leave as-is then.
File snapshot/win/system_snapshot_win.cc:
Patch Set #7, Line 159:
int os_version_build = 0;No need to initialize me, all codepaths should write to me before I’m read. Let me be a sanitizer error if someone mishandles.
if (ReadRegistryDWORD(key, L"CurrentMajorVersionNumber", &os_version_major_) &&
ReadRegistryDWORD(key, L"CurrentMinorVersionNumber", &os_version_minor_) &&git cl format one more time before submitting.
Patch Set #7, Line 175:
ReadRegistryDWORDFromSZ(key, "CurrentBuildNumber", &os_version_build) &&`&os_version_bugfix_` for this one?
This is confusing because now it’s called CurrentBuildNumber, but what we call build has moved to what’s now called “UBR”. And to think that we originally chose these names to match Windows nomenclature at the time!
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Bruce Dawson (Gerrit)
Attention is currently required from: Mark Mentovai.
Patch set 9:Commit-Queue +1
5 comments:
Patchset:
> The CQ DRY RUN keeps failing and I'm not sure why - it seems unrelated. […]
Wow. That was really weird. I couldn't reproduce it locally because my Python environment wouldn't run that test script. I guess it was the bug that you noticed, but I totally don't understand how the symptoms ended up like that.
Patchset:
Thank you for your patience and for finding that last bug. PTAL.
File snapshot/win/system_snapshot_win.cc:
Patch Set #7, Line 159:
int os_version_build = 0;
No need to initialize me, all codepaths should write to me before I’m read. […]
The compiler insists on zero initialization, otherwise builds on the bots give this warning/error:
C:\b\s\w\ir\x\w\crashpad\snapshot\win\system_snapshot_win.cc(198) : error C2220: the following warning is treated as an error
C:\b\s\w\ir\x\w\crashpad\snapshot\win\system_snapshot_win.cc(198) : warning C4701: potentially uninitialized local variable 'os_version_build' used
if (ReadRegistryDWORD(key, L"CurrentMajorVersionNumber", &os_version_major_) &&
ReadRegistryDWORD(key, L"CurrentMinorVersionNumber", &os_version_minor_) &&git cl format one more time before submitting.
Done
Patch Set #7, Line 175:
ReadRegistryDWORDFromSZ(key, "CurrentBuildNumber", &os_version_build) &&
`&os_version_bugfix_` for this one? […]
Whoops! Wow. I totally missed that. Thanks.
And yeah, the names are a mess. UBR, also known as QFE, also known as ???
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Bruce Dawson.
Patch set 9:Code-Review +1
3 comments:
Commit Message:
Patch Set #9, Line 7:
Get correct version info from registryCan you prefix this with win: before landing, so the extent of the impact is very obvious from the `--pretty=oneline` log?
Patchset:
Wow. That was really weird. I couldn't reproduce it locally because my Python environment wouldn't run that test script. I guess it was the bug that you noticed, but I totally don't understand how the symptoms ended up like that.
It’s reasonable for `!peb` to need to know the version of the OS that produced the minidump. It’s maybe also reasonable for it to throw its hands up in the air if that reported version is newer than anything it’s ever heard of, or if it doesn’t pass some sanity checking.
Patchset:
Thank you for your patience and for finding that last bug. PTAL.
Bugs found, bugs fixed! Happy to help.
To view, visit change 3434090. To unsubscribe, or for help writing mail filters, visit settings.