5 ms·

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

by 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(...
  - phkamp 7y ago
    Yes, the FOSS industry needs to grow up about software quality. It is natural to have very expensive checks in development and test-environments, they help and aid development and QA to no end, and of course they should be disabled in production. But the run-of-the-mill assert which documents integer ranges, pointer non-null-ness, data structure relationships and so on, cost nothing in performance and should be left in production code. And as I said: We developed Varnish Cache to prove this point, and we overwhelmingly have: One bad CVE since 2006, and "only" about 20% of all web-traffic goes through Varnish at some point or other. Re Huawei: The report claims that there where no checks at all, and I'm saying they have not documented evidence for that claim, because the lack of checking code could be either disabled asserts in production code or the compiler reducing it as dead code.

by Buge 7y ago

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(...
- phkamp 7y ago
  Yes, the FOSS industry needs to grow up about software quality. It is natural to have very expensive checks in development and test-environments, they help and aid development and QA to no end, and of course they should be disabled in production. But the run-of-the-mill assert which documents integer ranges, pointer non-null-ness, data structure relationships and so on, cost nothing in performance and should be left in production code. And as I said: We developed Varnish Cache to prove this point, and we overwhelmingly have: One bad CVE since 2006, and "only" about 20% of all web-traffic goes through Varnish at some point or other. Re Huawei: The report claims that there where no checks at all, and I'm saying they have not documented evidence for that claim, because the lack of checking code could be either disabled asserts in production code or the compiler reducing it as dead code.