3 ms·

I think the analysis glosses over compile time checks. Take the "VOS_memcpy_s" function they condemn for not checking one of the arguments on page 35. If the

by phkamp 7y ago

I think the analysis glosses over compile time checks. Take the "VOS_memcpy_s" function they condemn for not checking one of the arguments on page 35. If the source contains a check that the destination length is no less than the source length, for instance in an assert(), and the compiler is able to conclude that will always be the case, it is perfectly justified in not generating code for the check. The trouble is that the check code would also be missing if the "production code" was compiled with asserts disabled. To make the conclusion they do, they have to have checked that: A) All implementations lack the check and all are in not-link-time-optimized library locations. (in which case asserts was probably disabled) or B) All distributed implementations (because VOS_memcpy_s was either static inline or #defined) all lack the check and at least one of them is called with arguments where the check does not hold or cannot be determined at compile time. (in which case asserts was probably also disabled.) If they cannot prove either of these two points, it may just be that the code is in fact correct, and that the compiler was able to determine this and did not need to emit the checks. For all their bragging about how state of the art their tools are, I see no indication that they have analyzed that deep and comprehensive. That said, Huawei's code is still has atrocious quality. Or as the C-team calls it: "Industry Best Practice"

Buge 7y ago
For both A and B, why do they need to check all locations? Wouldn't finding a single location be enough to prove that it's happening? Also in A and B you seem to be saying it's difficult for them to see whether all the locations lack the check. But it sounds to me like their static analysis makes that fairly easy, and that that's what they found. You are saying that VOS_memcpy_s's source code could be safe but have its safety checks hidden by the compiler if the compiler has strong optimizations enabled while simultaneously having asserts enabled and also every single call to VOS_memcpy_s has an assert before it. That seems like a pretty rare configuration, because AFAIK, asserts are usually disabled when strong optimizations are enabled. And it seems unlikely to me that Huawei would put an assert before every single call to VOS_memcpy_s, for multiple reasons, (1) because Huawei is sloppy, (2) because there's no reason to put an assert before VOS_memcpy_s if it has safety guarantees. So I conclude there's very little likelihood there are any checks in VOS_memcpy_s's source code.
- phkamp 7y ago
  No, you would put the assert inside the VOS_memcpy_s() implementation, and you would not make that a library function, but a static inline function, so the compiler can see the arguments every time it is called. As for disabling asserts in the code you ship: That is such a quaint 1980'ies thing to do, and people should have stopped doing it long time ago. First, asserts are usually free, in the sense that the values checked are already in registers. Second, any assert the compiler can evaluate is free, because no code will be generated. Third, asserts can often enable the compiler to produce better and faster code, because it knows more about the values it produces code for. And this is not theory: Nobody has ever accused Varnish Cache of being slow, despite the fact that approx 10% of all source lines are asserts which cannot be compiled out. My money is on Huaweis function doing the check with an assert which was compiled out for deliverable code. Why else would the have the function in the first place ? The fact that code is generated for the function as shown, indicates that it was not a static inline function, because the compiler would have reduced that to just the regular memcpy call, so it was probably in a library. So yeah: Industry Best Practice Incompetence.
  - Buge 7y ago
    >As for disabling asserts in the code you ship: That is such a quaint 1980'ies thing to do, and people should have stopped doing it long time ago. Sqlite makes all its asserts noop in release mode[1]. Chrome disables DCHECK() in release mode[2], and it's used many times[3]. RE2 has debug only checking[4]. In Rust debug mode, signed integer overflow panics, in release mode it wraps[5]. Firefox disables MOZ_ASSERT() in release mode[6], and it's used many times[7]. I doubt Huawei is more modern and less 1980s than all these other projects. >My money is on Huaweis function doing the check with an assert which was compiled out for deliverable code. Are you saying it was compiled out because asserts were disabled, or because the compiler could reason about the arguments at every callsite? It seems unlikely to me that the compiler could reason about the arguments at every callsite. [1] https://www.sqlite.org/assert.html https://www.sqlite.org/assert.html [2] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#check_dcheck_and-notreached https://chromium.googlesource.com/chromium/src/+/master/styl... [3] https://cs.chromium.org/search/?q=DCHECK%5C(+case:yes&sq=package:chromium&type=cs https://cs.chromium.org/search/?q=DCHECK%5C(+case:yes&sq=pac... [4] https://github.com/google/re2/blob/master/util/logging.h https://github.com/google/re2/blob/master/util/logging.h [5] http://huonw.github.io/blog/2016/04/myths-and-legends-about-integer-overflow-in-rust/ http://huonw.github.io/blog/2016/04/myths-and-legends-about-... [6] https://dxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#378 https://dxr.mozilla.org/mozilla-central/source/mfbt/Assertio... [7] https://dxr.mozilla.org/mozilla-central/search?q=MOZ_ASSERT(&redirect=true https://dxr.mozilla.org/mozilla-central/search?q=MOZ_ASSERT(...

by phkamp 7y ago

Buge 7y ago

For both A and B, why do they need to check all locations? Wouldn't finding a single location be enough to prove that it's happening? Also in A and B you seem to be saying it's difficult for them to see whether all the locations lack the check. But it sounds to me like their static analysis makes that fairly easy, and that that's what they found. You are saying that VOS_memcpy_s's source code could be safe but have its safety checks hidden by the compiler if the compiler has strong optimizations enabled while simultaneously having asserts enabled and also every single call to VOS_memcpy_s has an assert before it. That seems like a pretty rare configuration, because AFAIK, asserts are usually disabled when strong optimizations are enabled. And it seems unlikely to me that Huawei would put an assert before every single call to VOS_memcpy_s, for multiple reasons, (1) because Huawei is sloppy, (2) because there's no reason to put an assert before VOS_memcpy_s if it has safety guarantees. So I conclude there's very little likelihood there are any checks in VOS_memcpy_s's source code.

phkamp 7y ago
No, you would put the assert inside the VOS_memcpy_s() implementation, and you would not make that a library function, but a static inline function, so the compiler can see the arguments every time it is called. As for disabling asserts in the code you ship: That is such a quaint 1980'ies thing to do, and people should have stopped doing it long time ago. First, asserts are usually free, in the sense that the values checked are already in registers. Second, any assert the compiler can evaluate is free, because no code will be generated. Third, asserts can often enable the compiler to produce better and faster code, because it knows more about the values it produces code for. And this is not theory: Nobody has ever accused Varnish Cache of being slow, despite the fact that approx 10% of all source lines are asserts which cannot be compiled out. My money is on Huaweis function doing the check with an assert which was compiled out for deliverable code. Why else would the have the function in the first place ? The fact that code is generated for the function as shown, indicates that it was not a static inline function, because the compiler would have reduced that to just the regular memcpy call, so it was probably in a library. So yeah: Industry Best Practice Incompetence.
- Buge 7y ago
  >As for disabling asserts in the code you ship: That is such a quaint 1980'ies thing to do, and people should have stopped doing it long time ago. Sqlite makes all its asserts noop in release mode[1]. Chrome disables DCHECK() in release mode[2], and it's used many times[3]. RE2 has debug only checking[4]. In Rust debug mode, signed integer overflow panics, in release mode it wraps[5]. Firefox disables MOZ_ASSERT() in release mode[6], and it's used many times[7]. I doubt Huawei is more modern and less 1980s than all these other projects. >My money is on Huaweis function doing the check with an assert which was compiled out for deliverable code. Are you saying it was compiled out because asserts were disabled, or because the compiler could reason about the arguments at every callsite? It seems unlikely to me that the compiler could reason about the arguments at every callsite. [1] https://www.sqlite.org/assert.html https://www.sqlite.org/assert.html [2] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#check_dcheck_and-notreached https://chromium.googlesource.com/chromium/src/+/master/styl... [3] https://cs.chromium.org/search/?q=DCHECK%5C(+case:yes&sq=package:chromium&type=cs https://cs.chromium.org/search/?q=DCHECK%5C(+case:yes&sq=pac... [4] https://github.com/google/re2/blob/master/util/logging.h https://github.com/google/re2/blob/master/util/logging.h [5] http://huonw.github.io/blog/2016/04/myths-and-legends-about-integer-overflow-in-rust/ http://huonw.github.io/blog/2016/04/myths-and-legends-about-... [6] https://dxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#378 https://dxr.mozilla.org/mozilla-central/source/mfbt/Assertio... [7] https://dxr.mozilla.org/mozilla-central/search?q=MOZ_ASSERT(&redirect=true https://dxr.mozilla.org/mozilla-central/search?q=MOZ_ASSERT(...