Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/10948 )
Change subject: IMPALA-5031: Fix undefined behavior: memset NULL
......................................................................
Patch Set 2:
(1 comment)
Yeah I generally disagree with the idea of adding NULL checks to every
invocation of memset() - I think it makes the invariants of the code harder to
understand and adds runtime overhead. In practice I think all of the callsites
pass n == 0 when there's a null pointer and glibc memset() won't dereference
the pointer in that case.
There are more theoretical possibilities if the compiler decides to inline a
custom memset implementation but I find it unlikely in practice that that would
be compiled to anything strange since that code still has to handle the n == 0
case correctly by not dereferencing the pointer. You could have an
implementation like below
if (p == NULL) DoSomethingWild();
if (n >= ...) {
}
if (n >= ...) {
}
But something like below makes way more sense.
if (n >= ...) {
}
if (n >= ...) {
}
http://gerrit.cloudera.org:8080/#/c/10948/2/be/src/util/ubsan.h
File be/src/util/ubsan.h:
http://gerrit.cloudera.org:8080/#/c/10948/2/be/src/util/ubsan.h@29
PS2, Line 29: if (s == nullptr) return s;
We should DCHECK that n == 0 in this case since otherwise it's a bug.
Or actually, this check could be if (n == 0) and we could DCHECK != NULL - I
think that's closer to the intent.
--
To view, visit http://gerrit.cloudera.org:8080/10948
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18fa02dc887a42a94c6f81e4923d17568f2184f2
Gerrit-Change-Number: 10948
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <[email protected]>
Gerrit-Reviewer: Jim Apple <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]>
Gerrit-Comment-Date: Tue, 17 Jul 2018 15:56:27 +0000
Gerrit-HasComments: Yes
