Jim Apple 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)
> (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 >= ...) {
> }
https://blog.regehr.org/archives/1292
shows
https://goo.gl/h7K89h
which shows
int set(char *p, int c, size_t n) {
memset(p, c, n);
return p == 0;
}
which compiles to
set:
subq $8, %rsp
call memset
xorl %eax, %eax
addq $8, %rsp
ret
which uses the fact that the first argument can't be NULL to optimize away the
comparison in the return statement.
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.
If DCHECKs are no-ops in release mode, the NULL pointer check will be missing,
which allows the compiler to have demons fly out of my nose.
https://groups.google.com/d/msg/comp.std.c/ycpVKxTZkgw/S2hHdTbv4d8J
--
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 16:11:07 +0000
Gerrit-HasComments: Yes
