Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885243002:
[NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. (Closed)
Description[NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated.
Enforced in base QueryRegKeyValue function.
TBR=robertshield
BUG=714401
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2885243002
Cr-Commit-Position: refs/heads/master@{#490780}
Committed: https://chromium.googlesource.com/chromium/src/+/42c976267d61dceba3ad216d7e475288feeed409
Patch Set 1 #
Total comments: 4
Patch Set 2 : Code review fixes, part 1. #
Total comments: 23
Patch Set 3 : Code review fixes, part 2. #
Total comments: 10
Patch Set 4 : Code review fixes, part 3. #
Messages
Total messages: 38 (24 generated)
Description was changed from ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. BUG=714401 ========== to ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. BUG=714401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by pennymac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. BUG=714401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. Enforced in base QueryRegKeyValue function. BUG=714401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
pennymac@chromium.org changed reviewers: + robertshield@chromium.org
Hi Robert! Small patch to catch any sneaky business, resulting in a SZ or MULTI_SZ that is not null terminated. I wanted to do it in one place, in the base QueryRegKeyValue function, instead of up in wrapper functions. Thanks for your time!
https://codereview.chromium.org/2885243002/diff/1/chrome_elf/nt_registry/nt_r... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:780: *out_buffer = new BYTE[*out_size]; totally optional, as this looks right to me, but figured I'd mention it. I prefer to manage buffers of BYTEs using std::vector<BYTE> which minimizes the chance of a leak by callers of this function. equivalent code here would be something like bool QueryRegKeyValue(HANDLE key, const wchar_t* value_name, ULONG* out_type, std::vector<BYTE>* out_buffer) { [...] BYTE* data_offset = reinterpret_cast<BYTE*>(value_info) + value_info->DataOffset; out_buffer->assign(data_offset, data_offset + value_info->DataLength); out_buffer->back() = 0; [...] } } https://codereview.chromium.org/2885243002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:784: if (*out_type == REG_SZ || *out_type == REG_MULTI_SZ) how about REG_EXPAND_SZ?
grt@chromium.org changed reviewers: + grt@chromium.org
i have a somewhat different suggestion:
- i see QueryRegKeyValue as a raw "give me the bytes in the registry" function.
i would leave it that way, but make it "safe" in that it would allocate three
more bytes than needed for all reads and explicitly set them to zero. this will
ensure that any buffer returned by it will be terminated (even those with an odd
number of bytes), although the |out_size| may or may not include the terminator
(depending on whether the value in the registry has a terminator or not).
- QueryRegValueSZ is the "gimme a string" function, so it should be responsible
for returning a std::wstring that does not include trailing terminators in the
string data under any circumstances. it could have logic like this after the
call to QueryRegKeyValue:
if (ret_size < sizeof(wchar_t)) {
// The value is too small to hold even a single character.
out_sz->clear();
} else {
static_assert(sizeof(wchar_t) == 2, "Unexpected wchar_t size.");
// Ignore a trailing odd byte if there is one.
if (ret_size & 0x01)
--ret_size;
wchar_t* const value_start = reinterpret_cast<wchar_t*>(value_bytes);
// Ignore all trailing zero chars.
wchar_t* value_end = value_start + ret_size / sizeof(wchar_t);
while (value_end != value_start && value_end[-1] == L'\0')
--value_end;
// Return what is left (which may have embedded nulls).
out_sz->assign(value_start, value_end - value_start);
}
(the usual caveats apply: i haven't compiled or tested this.)
The CQ bit was checked by pennymac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks to you both! I've moved the termination sanity check out into the two SZ/MULTISZ wrapper functions. I also adjusted QueryRegKeyValue to take a vector of bytes - easier to manipulate internally as well. Added a few "malformed" test cases for both SZ and MULTISZ. Doing a dry run now. https://codereview.chromium.org/2885243002/diff/1/chrome_elf/nt_registry/nt_r... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:780: *out_buffer = new BYTE[*out_size]; On 2017/05/17 16:59:59, robertshield wrote: > totally optional, as this looks right to me, but figured I'd mention it. I > prefer to manage buffers of BYTEs using std::vector<BYTE> which minimizes the > chance of a leak by callers of this function. > > equivalent code here would be something like > bool QueryRegKeyValue(HANDLE key, const wchar_t* value_name, ULONG* out_type, > std::vector<BYTE>* out_buffer) { > [...] > > BYTE* data_offset = reinterpret_cast<BYTE*>(value_info) + > value_info->DataOffset; > out_buffer->assign(data_offset, data_offset + value_info->DataLength); > out_buffer->back() = 0; > > [...] > } > > } Done. For the query function, a vector of bytes is now used. Making the change actually made it easier for me to insert changes in the new string terminator helper function. :) https://codereview.chromium.org/2885243002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:784: if (*out_type == REG_SZ || *out_type == REG_MULTI_SZ) On 2017/05/17 16:59:59, robertshield wrote: > how about REG_EXPAND_SZ? Done. Thanks for noting this Robert. Mine as well support EXPAND_SZ as just any other SZ.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)
looks pretty good. a few comments below. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:556: for (size_t i = value_bytes->size(); i < terminator_size; i++) if (value_bytes->size() < terminator_size) { value_bytes->insert(value_bytes->end(), terminator_size - value_bytes->size(), 0); } https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:561: while (modulo) { value_bytes->insert(value_bytes->end(), modulo, 0); https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:580: for (size_t i = 0; i < terminator_size; i++) value_bytes->insert(value_bytes->end(), terminator_size, 0); https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:818: out_buffer->clear(); nit: by using assign() below, clear() is only needed in the !data_size case. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:822: for (size_t i = 0; i < data_size; i++) out_buffer->assign(data, data + data_size); https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:886: *out_sz = reinterpret_cast<wchar_t*>(value_bytes.data()); should this preserve embedded nulls? if yes, use out_sz->assign(value, value + len_less_terminator). https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:932: while (temp.length() != 0) { nit: !temp.empty() https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:933: (*out_multi_sz).push_back(temp); nit: avoid making another copy of |temp| with: pointer += temp.length() + 1; (*out_multi_sz).push_back(std::move(temp)); temp = pointer;
https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:929: std::wstring temp = pointer; Won't this truncate any characters after the first null char in value_bytes?
The CQ bit was checked by pennymac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:556: for (size_t i = value_bytes->size(); i < terminator_size; i++) On 2017/07/20 05:22:53, UTC plus 2 wrote: > if (value_bytes->size() < terminator_size) { > value_bytes->insert(value_bytes->end(), terminator_size - > value_bytes->size(), 0); > } Done. <sigh> Thank you for your 1337 c++ skillz Greg. Forgot all about insert() with an iterator. That's much cleaner - all through this CL. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:561: while (modulo) { On 2017/07/20 05:22:53, UTC plus 2 wrote: > value_bytes->insert(value_bytes->end(), modulo, 0); Done. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:580: for (size_t i = 0; i < terminator_size; i++) On 2017/07/20 05:22:53, UTC plus 2 wrote: > value_bytes->insert(value_bytes->end(), terminator_size, 0); Done. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:818: out_buffer->clear(); On 2017/07/20 05:22:53, UTC plus 2 wrote: > nit: by using assign() below, clear() is only needed in the !data_size case. Done. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:822: for (size_t i = 0; i < data_size; i++) On 2017/07/20 05:22:53, UTC plus 2 wrote: > out_buffer->assign(data, data + data_size); assign. thank you. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:886: *out_sz = reinterpret_cast<wchar_t*>(value_bytes.data()); On 2017/07/20 05:22:53, UTC plus 2 wrote: > should this preserve embedded nulls? if yes, use out_sz->assign(value, value + > len_less_terminator). I never expected the result of the QueryRegValueSZ wrapper function to preserve embedded nulls. That's a very abnormal use case for an SZ type. That's what multisz is really for. If there's a special case, caller can use the raw QueryRegKeyValue function. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:929: std::wstring temp = pointer; On 2017/07/20 17:14:04, robertshield wrote: > Won't this truncate any characters after the first null char in value_bytes? No. It merely creates a new wstring object, and copies in the string starting at pointer - up until the first \0. std::wstring initialization doesn't mangle the source. The C pointer |pointer| merely moves along the content of |value_bytes|, to the start of each string (which are separated by \0). The loop instantiates |temp| each time starting from |pointer|. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:932: while (temp.length() != 0) { On 2017/07/20 05:22:53, UTC plus 2 wrote: > nit: !temp.empty() I wasn't entirely sure whether an empty string ("" or \0) would be considered empty(). Am a bit confused about how std::wstring handles EoS characters. Wanted to be safe with length(). e.g. std::wstring blah = L""; vs std::wstring blah2(); Internally, is |blah| an "empty" object, or is that size() == 1 and length() == 0? Assuming this class even stores the null character as an element. Based on your advice, I at least assume empty() returns true for a string of length 0, size 1? <sigh> Gimme a C array any day of the week. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:933: (*out_multi_sz).push_back(temp); On 2017/07/20 05:22:53, UTC plus 2 wrote: > nit: avoid making another copy of |temp| with: > pointer += temp.length() + 1; > (*out_multi_sz).push_back(std::move(temp)); > temp = pointer; Done. I didn't even think push_back could take the result of a std::move - but TIL. Makes sense. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:886: *out_sz = reinterpret_cast<wchar_t*>(value_bytes.data()); On 2017/07/20 18:38:37, penny wrote: > On 2017/07/20 05:22:53, UTC plus 2 wrote: > > should this preserve embedded nulls? if yes, use out_sz->assign(value, value + > > len_less_terminator). > > I never expected the result of the QueryRegValueSZ wrapper function to preserve > embedded nulls. That's a very abnormal use case for an SZ type. That's what > multisz is really for. If there's a special case, caller can use the raw > QueryRegKeyValue function. Sounds reasonable to me. Please document this behavior in the doc comment for both QueryRegValueSZ functions in the .h. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:932: while (temp.length() != 0) { On 2017/07/20 18:38:37, penny wrote: > On 2017/07/20 05:22:53, UTC plus 2 wrote: > > nit: !temp.empty() > > I wasn't entirely sure whether an empty string ("" or \0) would be considered > empty(). Am a bit confused about how std::wstring handles EoS characters. > Wanted to be safe with length(). > e.g. > std::wstring blah = L""; > vs > std::wstring blah2(); > > Internally, is |blah| an "empty" object, or is that size() == 1 and length() == > 0? Assuming this class even stores the null character as an element. > > Based on your advice, I at least assume empty() returns true for a string of > length 0, size 1? <sigh> Gimme a C array any day of the week. The spec more-or-less says that a std::basic_string does not contain a terminator -- there is no guarantee that the memory returned by data() is terminated, only that the first length() characters in it are the string contents. So then what about its c_str() method? It returns a pointer to null-terminated char array that contains the length() characters of data() followed by a string terminator. It just so happens that all sane implementations put a terminator at the end of the data() buffer so that c_str() and data() return the same pointer value, but that's really an implementation detail. Back to length() == 0 vs empty() -- the latter basically boils down to this inline method: bool empty() const { return length() == 0; } In Chromium, we prefer to use empty() when testing for this since it's more explicit. In your examples, blah and blah2 are both empty and have length() == 0. Oh, and as for size() and length(), they're synonyms! I imagine the two exist because: - length() -- folks are used to thinking of how "long" a string is thanks to strlen - size() -- std::basic_string is a container (like a vector), so give it a size() member for use in other templates that take a container type Hope this helps clear up some of the ambiguity! https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:933: (*out_multi_sz).push_back(temp); On 2017/07/20 18:38:36, penny wrote: > On 2017/07/20 05:22:53, UTC plus 2 wrote: > > nit: avoid making another copy of |temp| with: > > pointer += temp.length() + 1; > > (*out_multi_sz).push_back(std::move(temp)); > > temp = pointer; > > Done. I didn't even think push_back could take the result of a std::move - but > TIL. Makes sense. Thanks! I didn't know either -- I just figured something like that existed and looked it up here: http://en.cppreference.com/w/cpp/container/vector/push_back https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:558: (terminator_size - value_bytes->size()), 0); nit: i would omit these extra parens around the arithmetic -- they don't disambiguate anything imo. https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:808: reinterpret_cast<KEY_VALUE_FULL_INFORMATION*>(new BYTE[size_needed]); please use std::unique_ptr where possible to avoid the possibility of memory leaks. it would be cleaner for this to be: std::unique_ptr<BYTE[]> buffer(new BYTE[size_needed]); KEY_VALUE_FULL_INFORMATION* value_info = reinterpret_cast<KEY_VALUE_FULL_INFORMATION*>(buffer.get()); and then get rid of delete[] on line 827. this also gets rid of the need for the |success| local var, since early-exit becomes safe. my personal choice would be to make the failure case return false so that the success case reaches the end of the function. (note the avoidance of base::MakeUnique for allocating the array, as it would zero-initialize it.) https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:879: (type != REG_SZ && type != REG_EXPAND_SZ)) { maybe document in .h that REG_EXPAND_SZ values are handled by this, but that no expansion takes place? https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:925: (*out_multi_sz).clear(); out_multi_sz->clear(); https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:933: (*out_multi_sz).push_back(std::move(temp)); out_multi_sz->push_back(std::move(temp));
The CQ bit was checked by pennymac@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Dry run underway! https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:886: *out_sz = reinterpret_cast<wchar_t*>(value_bytes.data()); On 2017/07/21 08:09:00, grt (UTC plus 2) wrote: > On 2017/07/20 18:38:37, penny wrote: > > On 2017/07/20 05:22:53, UTC plus 2 wrote: > > > should this preserve embedded nulls? if yes, use out_sz->assign(value, value > + > > > len_less_terminator). > > > > I never expected the result of the QueryRegValueSZ wrapper function to > preserve > > embedded nulls. That's a very abnormal use case for an SZ type. That's what > > multisz is really for. If there's a special case, caller can use the raw > > QueryRegKeyValue function. > > Sounds reasonable to me. Please document this behavior in the doc comment for > both QueryRegValueSZ functions in the .h. Done. https://codereview.chromium.org/2885243002/diff/20001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:932: while (temp.length() != 0) { On 2017/07/21 08:09:00, grt (UTC plus 2) wrote: > On 2017/07/20 18:38:37, penny wrote: > > On 2017/07/20 05:22:53, UTC plus 2 wrote: > > > nit: !temp.empty() > > > > I wasn't entirely sure whether an empty string ("" or \0) would be considered > > empty(). Am a bit confused about how std::wstring handles EoS characters. > > Wanted to be safe with length(). > > e.g. > > std::wstring blah = L""; > > vs > > std::wstring blah2(); > > > > Internally, is |blah| an "empty" object, or is that size() == 1 and length() > == > > 0? Assuming this class even stores the null character as an element. > > > > Based on your advice, I at least assume empty() returns true for a string of > > length 0, size 1? <sigh> Gimme a C array any day of the week. > > The spec more-or-less says that a std::basic_string does not contain a > terminator -- there is no guarantee that the memory returned by data() is > terminated, only that the first length() characters in it are the string > contents. > > So then what about its c_str() method? It returns a pointer to null-terminated > char array that contains the length() characters of data() followed by a string > terminator. It just so happens that all sane implementations put a terminator at > the end of the data() buffer so that c_str() and data() return the same pointer > value, but that's really an implementation detail. > > Back to length() == 0 vs empty() -- the latter basically boils down to this > inline method: > > bool empty() const { return length() == 0; } > > In Chromium, we prefer to use empty() when testing for this since it's more > explicit. > > In your examples, blah and blah2 are both empty and have length() == 0. > > Oh, and as for size() and length(), they're synonyms! I imagine the two exist > because: > - length() -- folks are used to thinking of how "long" a string is thanks to > strlen > - size() -- std::basic_string is a container (like a vector), so give it a > size() member for use in other templates that take a container type > > Hope this helps clear up some of the ambiguity! Done. Clear as mud! https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:558: (terminator_size - value_bytes->size()), 0); On 2017/07/21 08:09:00, grt (UTC plus 2) wrote: > nit: i would omit these extra parens around the arithmetic -- they don't > disambiguate anything imo. Done. https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:808: reinterpret_cast<KEY_VALUE_FULL_INFORMATION*>(new BYTE[size_needed]); On 2017/07/21 08:09:00, grt (UTC plus 2) wrote: > please use std::unique_ptr where possible to avoid the possibility of memory > leaks. it would be cleaner for this to be: > > std::unique_ptr<BYTE[]> buffer(new BYTE[size_needed]); > KEY_VALUE_FULL_INFORMATION* value_info = > reinterpret_cast<KEY_VALUE_FULL_INFORMATION*>(buffer.get()); > > and then get rid of delete[] on line 827. this also gets rid of the need for the > |success| local var, since early-exit becomes safe. my personal choice would be > to make the failure case return false so that the success case reaches the end > of the function. > > (note the avoidance of base::MakeUnique for allocating the array, as it would > zero-initialize it.) Done. https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:879: (type != REG_SZ && type != REG_EXPAND_SZ)) { On 2017/07/21 08:09:00, grt (UTC plus 2) wrote: > maybe document in .h that REG_EXPAND_SZ values are handled by this, but that no > expansion takes place? The .h function documentation declares support for REG_EXPAND_SZ. Registry APIs don't do expansion of this type. It's just a notification label if an application wants to label a reg value as needing to be expanded later. Registry APIs treat SZ and EXPAND_SZ the same. https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:925: (*out_multi_sz).clear(); On 2017/07/21 08:09:00, grt (UTC plus 2) wrote: > out_multi_sz->clear(); Done. https://codereview.chromium.org/2885243002/diff/40001/chrome_elf/nt_registry/... chrome_elf/nt_registry/nt_registry.cc:933: (*out_multi_sz).push_back(std::move(temp)); On 2017/07/21 08:09:00, grt (UTC plus 2) wrote: > out_multi_sz->push_back(std::move(temp)); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping, Robert or Greg. Thanks!
lgtm. apologies for the delay!
Description was changed from ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. Enforced in base QueryRegKeyValue function. BUG=714401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. Enforced in base QueryRegKeyValue function. TBR=robertshield BUG=714401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by pennymac@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501518487182180,
"parent_rev": "f591924ffd8aee0dc7b7e141df9c14582df886b7", "commit_rev":
"42c976267d61dceba3ad216d7e475288feeed409"}
Message was sent while issue was closed.
Description was changed from ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. Enforced in base QueryRegKeyValue function. TBR=robertshield BUG=714401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== [NtRegistry] Ensure REG_SZ and REG_MULTI_SZ are null terminated. Enforced in base QueryRegKeyValue function. TBR=robertshield BUG=714401 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2885243002 Cr-Commit-Position: refs/heads/master@{#490780} Committed: https://chromium.googlesource.com/chromium/src/+/42c976267d61dceba3ad216d7e47... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/42c976267d61dceba3ad216d7e47...
Message was sent while issue was closed.
Thanks very much for your review time Greg and Robert! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
