Meta bug for tracking issues raised from undefined behavior sanitizers.
What is the value to fixing these? JSC uses C++ as if it was a structured assembler. If the things that the various committees view as UB were actually removed from the language then it wouldn’t be possible to implement JSC.
(In reply to Filip Pizlo from comment #1) > What is the value to fixing these? JSC uses C++ as if it was a structured > assembler. If the things that the various committees view as UB were > actually removed from the language then it wouldn’t be possible to implement > JSC. We'd like to enable Control Flow Integrity https://clang.llvm.org/docs/ControlFlowIntegrity.html on the PlayStation port as a threat mitigation. We have a compiler team here that works on LLVM and is interested in enabling CFI with WebKit. Our biggest attack vector for hacking our console is WebKit. We understand that JSC has some bits of code that actively rely on undefined behavior. Others might be false positives. For those we can blacklist them so CFI doesn't report any issues. See Yusuke's patch for https://bugs.webkit.org/show_bug.cgi?id=188741 as an example. Others might be legit bugs. When we started running Undefined Behavior Sanitizer over WebKit Yusuke felt some results warranted action. See https://trac.webkit.org/changeset/235307/webkit and https://trac.webkit.org/changeset/234855/webkit for examples. You can also search for ubsan in trac.
(In reply to Don Olmstead from comment #2) > (In reply to Filip Pizlo from comment #1) > > What is the value to fixing these? JSC uses C++ as if it was a structured > > assembler. If the things that the various committees view as UB were > > actually removed from the language then it wouldn’t be possible to implement > > JSC. > > We'd like to enable Control Flow Integrity > https://clang.llvm.org/docs/ControlFlowIntegrity.html on the PlayStation > port as a threat mitigation. We have a compiler team here that works on LLVM > and is interested in enabling CFI with WebKit. Our biggest attack vector for > hacking our console is WebKit. Can you help fix security bugs in WebKit? > > We understand that JSC has some bits of code that actively rely on undefined > behavior. Others might be false positives. For those we can blacklist them > so CFI doesn't report any issues. See Yusuke's patch for > https://bugs.webkit.org/show_bug.cgi?id=188741 as an example. I don’t like the idea of those blacklist code changes. It’s just noise to most people. > > Others might be legit bugs. When we started running Undefined Behavior > Sanitizer over WebKit Yusuke felt some results warranted action. See > https://trac.webkit.org/changeset/235307/webkit and > https://trac.webkit.org/changeset/234855/webkit for examples. You can also > search for ubsan in trac. The first of those is just not a bug. CPUs we target ignore the high bits of a shift amount. This code would only be recompiled if the shift amount ended up being a constant. The second one seems like an asymptomatic bug. It’s nice to fix but I don’t think t makes sense to put effort towards finding those.
(In reply to Filip Pizlo from comment #3) > (In reply to Don Olmstead from comment #2) > > (In reply to Filip Pizlo from comment #1) > > > What is the value to fixing these? JSC uses C++ as if it was a structured > > > assembler. If the things that the various committees view as UB were > > > actually removed from the language then it wouldn’t be possible to implement > > > JSC. > > > > We'd like to enable Control Flow Integrity > > https://clang.llvm.org/docs/ControlFlowIntegrity.html on the PlayStation > > port as a threat mitigation. We have a compiler team here that works on LLVM > > and is interested in enabling CFI with WebKit. Our biggest attack vector for > > hacking our console is WebKit. > > Can you help fix security bugs in WebKit? I'm on the security mailing list as well as a handful of people over here at Sony. If these things turn up security issues then we'd definitely help fix. In the long term we'd like to spin up bots running the different sanitizers to continuously look for issues. An issue there is that I don't know that we could lock down the logs to a bot if it were attached to build.webkit.org. I've talked a bit about this with Brent. Another issue would be collating the stacks. Not sure how we'd do that yet. Anyways I think this is a solvable issue. > > We understand that JSC has some bits of code that actively rely on undefined > > behavior. Others might be false positives. For those we can blacklist them > > so CFI doesn't report any issues. See Yusuke's patch for > > https://bugs.webkit.org/show_bug.cgi?id=188741 as an example. > > I don’t like the idea of those blacklist code changes. It’s just noise to > most people. Benefit is grepping for source but yea I don't have a strong opinion on where the blacklist will live. > > Others might be legit bugs. When we started running Undefined Behavior > > Sanitizer over WebKit Yusuke felt some results warranted action. See > > https://trac.webkit.org/changeset/235307/webkit and > > https://trac.webkit.org/changeset/234855/webkit for examples. You can also > > search for ubsan in trac. > > The first of those is just not a bug. CPUs we target ignore the high bits of > a shift amount. This code would only be recompiled if the shift amount ended > up being a constant. > > The second one seems like an asymptomatic bug. It’s nice to fix but I don’t > think t makes sense to put effort towards finding those. Chrome has a page about their work with CFI https://www.chromium.org/developers/testing/control-flow-integrity so they're seeing value with using CFI. We don't really know what issues crop up until we have things running throughout. As I stated we're happy to help sort out anything it manages to find.
(In reply to Filip Pizlo from comment #3) > The first of those is just not a bug. CPUs we target ignore the high bits of > a shift amount. This code would only be recompiled if the shift amount ended > up being a constant. I think the problem of UB is not CPU related thing. CPU behavior is really nice, and meets our expectation. Rather, I think the typical UB-related problem is caused because of the C compiler's assumption "dev never does UB" (clearly, it is wrong). This assumption introduces restriction on some value's range (like, "you are doing "v << x", so, x should be [0, 64), and let's use this information to do further optimizations"), it leads to "aggressively" optimized code, which does not meet our expected behavior. One of the issue I remember is that https://trac.webkit.org/changeset/195906/webkit, GCC leverages our UB behavior and does "optimizations" which makes B3 broken. My thought on UB is, 1. If we can easily avoid UB, we should do that. 2. If we require some massively ugly annotations, I don't have strong opinion. 3. If the fix introduces performance regression, we should not accept such a change. What do you think of?
(In reply to Yusuke Suzuki from comment #5) > (In reply to Filip Pizlo from comment #3) > > The first of those is just not a bug. CPUs we target ignore the high bits of > > a shift amount. This code would only be recompiled if the shift amount ended > > up being a constant. > > I think the problem of UB is not CPU related thing. CPU behavior is really > nice, and meets our expectation. > Rather, I think the typical UB-related problem is caused because of the C > compiler's assumption "dev never does UB" (clearly, it is wrong). > This assumption introduces restriction on some value's range (like, "you are > doing "v << x", so, x should be [0, 64), and let's use this information to > do further optimizations"), it leads to "aggressively" optimized code, which > does not meet our expected behavior. > One of the issue I remember is that > https://trac.webkit.org/changeset/195906/webkit, GCC leverages our UB > behavior and does "optimizations" which makes B3 broken. > > My thought on UB is, > > 1. If we can easily avoid UB, we should do that. In particular, signed integer overflow, too large shift amount, and use of uninitialized value.
(In reply to Yusuke Suzuki from comment #6) > (In reply to Yusuke Suzuki from comment #5) > > (In reply to Filip Pizlo from comment #3) > > > The first of those is just not a bug. CPUs we target ignore the high bits of > > > a shift amount. This code would only be recompiled if the shift amount ended > > > up being a constant. > > > > I think the problem of UB is not CPU related thing. CPU behavior is really > > nice, and meets our expectation. > > Rather, I think the typical UB-related problem is caused because of the C > > compiler's assumption "dev never does UB" (clearly, it is wrong). > > This assumption introduces restriction on some value's range (like, "you are > > doing "v << x", so, x should be [0, 64), and let's use this information to > > do further optimizations"), it leads to "aggressively" optimized code, which > > does not meet our expected behavior. > > One of the issue I remember is that > > https://trac.webkit.org/changeset/195906/webkit, GCC leverages our UB > > behavior and does "optimizations" which makes B3 broken. > > > > My thought on UB is, > > > > 1. If we can easily avoid UB, we should do that. > > In particular, signed integer overflow, Possible with -fwrapv and no code changes. > too large shift amount, Sounds like llvm should just fix that. All CPUs we care about agree on the behavior of large shift amounts. It's dumb that llvm might make the code wrong if you do something that the CPU thinks is right. > and use of > uninitialized value. Sounds like llvm should just fix that, too.
I think that Sony's goals here are really well aligned with Apple's. WebKit is the primary source of system exploits on our systems, too, so anything we can do to deny attackers an avenue for exploit should be taken. I support the idea of getting WebKit to be UBSAN clean, just like we want to be ASAN clean, Guard Malloc clean, etc. We should clean all the SANs! Currently we are prioritizing things with probably paths to exploit, but I am in favor of making code changes to silence ASAN/TSAN/UBSAN warnings if it reduces noise so we can identify real problems or new regressions.
(In reply to Filip Pizlo from comment #7) > (In reply to Yusuke Suzuki from comment #6) > > (In reply to Yusuke Suzuki from comment #5) > > > (In reply to Filip Pizlo from comment #3) > > > > The first of those is just not a bug. CPUs we target ignore the high bits of > > > > a shift amount. This code would only be recompiled if the shift amount ended > > > > up being a constant. > > > > > > I think the problem of UB is not CPU related thing. CPU behavior is really > > > nice, and meets our expectation. > > > Rather, I think the typical UB-related problem is caused because of the C > > > compiler's assumption "dev never does UB" (clearly, it is wrong). > > > This assumption introduces restriction on some value's range (like, "you are > > > doing "v << x", so, x should be [0, 64), and let's use this information to > > > do further optimizations"), it leads to "aggressively" optimized code, which > > > does not meet our expected behavior. > > > One of the issue I remember is that > > > https://trac.webkit.org/changeset/195906/webkit, GCC leverages our UB > > > behavior and does "optimizations" which makes B3 broken. > > > > > > My thought on UB is, > > > > > > 1. If we can easily avoid UB, we should do that. > > > > In particular, signed integer overflow, > > Possible with -fwrapv and no code changes. > > > too large shift amount, > > Sounds like llvm should just fix that. All CPUs we care about agree on the > behavior of large shift amounts. It's dumb that llvm might make the code > wrong if you do something that the CPU thinks is right. > > > and use of > > uninitialized value. > > Sounds like llvm should just fix that, too. Even if we avoid such an optimization in LLVM, we need to do the same things for GCC and VC++, or leave them broken. If we leave them broken, then, anyway, folks using GCC need UB fix with COMPILER(GCC) / COMPILER(MSVC), and the situation is not changed. I think the ideal way is fixing C spec. But, I think fixing the spec takes toooooooooooooooooooooooooooooooooo long time since we need to convince folks who believe the optimizations introduced by these UB are actually useful. Until the spec is fixed, what we can do is limited. Fix UBs or leave them broken. Given that UB actually broke B3 previously, I think the relatively safe path for now is fixing UBs. As we see, UB codes found by UBSAN are not so many. We already avoid many UBs by the other warnings / sanitizers etc. And for the particularly important 3 cases, signed integer overflow, too large shift amount, uninitialized value, we have very easy fixes: (1) cast to unsigned, (2) mask the shift amount if it can be too large, (3) initialize it. I don't think this mess the code so much.
(In reply to Brent Fulgham from comment #8) > I think that Sony's goals here are really well aligned with Apple's. WebKit > is the primary source of system exploits on our systems, too, so anything we > can do to deny attackers an avenue for exploit should be taken. There's a cost/benefit trade-off. Running sanitizers on a VM seems like a lot of cost. > > I support the idea of getting WebKit to be UBSAN clean, just like we want to > be ASAN clean, Guard Malloc clean, etc. We should clean all the SANs! JavaScriptCore relies on features on the C/C++ language that are sometimes considered UB, depending on who you ask. > > Currently we are prioritizing things with probably paths to exploit, but I > am in favor of making code changes to silence ASAN/TSAN/UBSAN warnings if it > reduces noise so we can identify real problems or new regressions. I think that if we find a significant amount of UB in JSC then we should use compiler flags to simply turn that UB off.
Simple question: has anyone perf tested JSC with -fwrapv? I believe both clang and gcc support it. If that is perf-neutral, then we won't have to fix any more signed overflow bugs, ever. I'm not sure I understand the argument that says that running a sanitizer and fixing those bugs one-off is superior to just simply adding one compiler flag. If that is successful, maybe we can just ask for an option in clang to mask shift amounts. I could write an llvm pass to do that in an afternoon.
(In reply to Filip Pizlo from comment #11) > Simple question: has anyone perf tested JSC with -fwrapv? I believe both > clang and gcc support it. > > If that is perf-neutral, then we won't have to fix any more signed overflow > bugs, ever. I don't think this option causes performance regression. We also need VC++ support too since we build WebKit with VC++ too. I've found that undocumented flag `/d2UndefIntOverflow` and I think it should work[1]. Without this option, VC++ leverages UBs of signed integer overflow to aggressively optimize the code. I agree to enabling this option now to make signed overflow well-defined. [1]: https://devblogs.microsoft.com/cppblog/new-code-optimizer/ > > I'm not sure I understand the argument that says that running a sanitizer > and fixing those bugs one-off is superior to just simply adding one compiler > flag. My main concern is about leaving UBs. If we can avoid UBs by either fixing UBs or removing UBs from compiler behavior, I think either is fine. Removing UBs can be achieved by either (1) adding a flag to compiler to make some UBs well-defined or (2) fixing the spec. If we take (1) approach, I think we also need to ensure that all our supported compilers have flags to make UBs well-defined. > If that is successful, maybe we can just ask for an option in clang to mask > shift amounts. I could write an llvm pass to do that in an afternoon. I think this pass should not cause any performance problem in x86. I searched and learned that UBs in shift amount is because of some CPU behavior (PowerPC), and having a flag to align the behavior to x86 behavior does not cause any additional code emission in x86 I think. Maybe, it could emit masking ops for PowerPC, but we don't care the performance on PowerPC. Anyway, I think we need a flag for this to explicitly make this type of UBs well-defined. But..., we need to add this flag to GCC and VC++ too to remove UBs.
(In reply to Yusuke Suzuki from comment #12) > (In reply to Filip Pizlo from comment #11) > > Simple question: has anyone perf tested JSC with -fwrapv? I believe both > > clang and gcc support it. > > > > If that is perf-neutral, then we won't have to fix any more signed overflow > > bugs, ever. > > I don't think this option causes performance regression. > We also need VC++ support too since we build WebKit with VC++ too. > I've found that undocumented flag `/d2UndefIntOverflow` and I think it > should work[1]. > Without this option, VC++ leverages UBs of signed integer overflow to > aggressively optimize the code. > I agree to enabling this option now to make signed overflow well-defined. > > [1]: https://devblogs.microsoft.com/cppblog/new-code-optimizer/ > > > > > I'm not sure I understand the argument that says that running a sanitizer > > and fixing those bugs one-off is superior to just simply adding one compiler > > flag. > > My main concern is about leaving UBs. If we can avoid UBs by either fixing > UBs or removing UBs from compiler behavior, I think either is fine. > Removing UBs can be achieved by either (1) adding a flag to compiler to make > some UBs well-defined or (2) fixing the spec. > If we take (1) approach, I think we also need to ensure that all our > supported compilers have flags to make UBs well-defined. Consider the case of signed integer overflow. UBSAN will only find the cases where either by folding constants or by the way that the result is used, you have an obvious case of UB. So it's not like running UBSAN and fixing the bugs it reports will mean that we are avoiding UBs. We still have them. On the other hand, -fwrapv completely eliminates that class of UB. Therefore, if we are concerned about leaving UBs, we should use compiler flags to disable them. > > > If that is successful, maybe we can just ask for an option in clang to mask > > shift amounts. I could write an llvm pass to do that in an afternoon. > > I think this pass should not cause any performance problem in x86. > I searched and learned that UBs in shift amount is because of some CPU > behavior (PowerPC), and having a flag to align the behavior to x86 behavior > does not cause any additional code emission in x86 I think. Maybe, it could > emit masking ops for PowerPC, but we don't care the performance on PowerPC. Exactly. > > Anyway, I think we need a flag for this to explicitly make this type of UBs > well-defined. > But..., we need to add this flag to GCC and VC++ too to remove UBs. If we only added it to llvm then we would use it there. Most folks who use this code will use a version compiled by llvm. Since we're not talking about something that would cause top crashes, I'm not sure we have to worry too much about minority compilers doing the right thing.
(In reply to Filip Pizlo from comment #13) > Consider the case of signed integer overflow. UBSAN will only find the > cases where either by folding constants or by the way that the result is > used, you have an obvious case of UB. So it's not like running UBSAN and > fixing the bugs it reports will mean that we are avoiding UBs. We still > have them. > > On the other hand, -fwrapv completely eliminates that class of UB. > > Therefore, if we are concerned about leaving UBs, we should use compiler > flags to disable them. I think that we use these sanitizers with Debug build with the hope of disabling such optimizations. Then, we can find such a dangerous constant foldings performed in Release build. For example, B3 issue was only caused in Release build[1]. I think that issue can be caught by Debug+UBSAN effectively. But, I strongly agree that removing UBs with the flag is more effective to remove such bugs than removing random bugs found at runtime. [1]: https://bugs.webkit.org/show_bug.cgi?id=153647#c3 > > > If that is successful, maybe we can just ask for an option in clang to mask > > > shift amounts. I could write an llvm pass to do that in an afternoon. > > > > I think this pass should not cause any performance problem in x86. > > I searched and learned that UBs in shift amount is because of some CPU > > behavior (PowerPC), and having a flag to align the behavior to x86 behavior > > does not cause any additional code emission in x86 I think. Maybe, it could > > emit masking ops for PowerPC, but we don't care the performance on PowerPC. > > Exactly. > > > > > Anyway, I think we need a flag for this to explicitly make this type of UBs > > well-defined. > > But..., we need to add this flag to GCC and VC++ too to remove UBs. > > If we only added it to llvm then we would use it there. Most folks who use > this code will use a version compiled by llvm. > > Since we're not talking about something that would cause top crashes, I'm > not sure we have to worry too much about minority compilers doing the right > thing. If the UB-related issue occurs in GCC and VC++ due to missing flags, folks using these compilers will add some annotation / cast / masking or something like that to avoid the issues. If it does not matter, I think it is OK. But now, I'm a bit optimistic than before. Fortunately, all the supported compilers already have a option to remove UBs of signed integer overflow (I did not know /d2UndefIntOverflow on VC++). I saw many UB-related issues are caused by this UB, and I believe that UB-related issues can decrease a lot if we enable this option.
(In reply to Yusuke Suzuki from comment #14) > (In reply to Filip Pizlo from comment #13) > > Consider the case of signed integer overflow. UBSAN will only find the > > cases where either by folding constants or by the way that the result is > > used, you have an obvious case of UB. So it's not like running UBSAN and > > fixing the bugs it reports will mean that we are avoiding UBs. We still > > have them. > > > > On the other hand, -fwrapv completely eliminates that class of UB. > > > > Therefore, if we are concerned about leaving UBs, we should use compiler > > flags to disable them. > > I think that we use these sanitizers with Debug build with the hope of > disabling such optimizations. > Then, we can find such a dangerous constant foldings performed in Release > build. > For example, B3 issue was only caused in Release build[1]. I think that > issue can be caught by Debug+UBSAN effectively. I see what you mean. Ignore what I said. UBSAN will catch all dynamic instances of the issues we care about. But if that means turning: a << b into: a << (b & 31) in more than 1 or 2 places then we should ask for a compiler flag. Also, if it means replacing a lot of signed arithmetic with unsigned arithmetic, then same deal: fwrapv is better. > > But, I strongly agree that removing UBs with the flag is more effective to > remove such bugs than removing random bugs found at runtime. > > [1]: https://bugs.webkit.org/show_bug.cgi?id=153647#c3 > > > > > If that is successful, maybe we can just ask for an option in clang to mask > > > > shift amounts. I could write an llvm pass to do that in an afternoon. > > > > > > I think this pass should not cause any performance problem in x86. > > > I searched and learned that UBs in shift amount is because of some CPU > > > behavior (PowerPC), and having a flag to align the behavior to x86 behavior > > > does not cause any additional code emission in x86 I think. Maybe, it could > > > emit masking ops for PowerPC, but we don't care the performance on PowerPC. > > > > Exactly. > > > > > > > > Anyway, I think we need a flag for this to explicitly make this type of UBs > > > well-defined. > > > But..., we need to add this flag to GCC and VC++ too to remove UBs. > > > > If we only added it to llvm then we would use it there. Most folks who use > > this code will use a version compiled by llvm. > > > > Since we're not talking about something that would cause top crashes, I'm > > not sure we have to worry too much about minority compilers doing the right > > thing. > > If the UB-related issue occurs in GCC and VC++ due to missing flags, folks > using these compilers will add some annotation / cast / masking or something > like that to avoid the issues. > If it does not matter, I think it is OK. > > But now, I'm a bit optimistic than before. Fortunately, all the supported > compilers already have a option to remove UBs of signed integer overflow (I > did not know /d2UndefIntOverflow on VC++). > I saw many UB-related issues are caused by this UB, and I believe that > UB-related issues can decrease a lot if we enable this option. That's cool! I still think that if llvm had a flag to disable UB and other compilers didn't, then we wouldn't necessarily allow users of other compilers to pollute the code.
(In reply to Filip Pizlo from comment #15) > That's cool! > > I still think that if llvm had a flag to disable UB and other compilers > didn't, then we wouldn't necessarily allow users of other compilers to > pollute the code. Filed an issue enabling -fwrapv / d2UndefIntOverflow. I'll look into it at weekend as my hobby project. https://bugs.webkit.org/show_bug.cgi?id=196829 It seems that JF attempted to make signed-overflow non-UB, but the sad thing is that it is rejected by WG21 :(
(In reply to Yusuke Suzuki from comment #16) > Filed an issue enabling -fwrapv / d2UndefIntOverflow. I'll look into it at > weekend as my hobby project. > https://bugs.webkit.org/show_bug.cgi?id=196829 > > It seems that JF attempted to make signed-overflow non-UB, but the sad thing > is that it is rejected by WG21 :( Thanks Yusuke. But does -fwrapv cause ubsan to not complaining about signed overflow? I would guess not? (In reply to Brent Fulgham from comment #8) > I think that Sony's goals here are really well aligned with Apple's. WebKit > is the primary source of system exploits on our systems, too, so anything we > can do to deny attackers an avenue for exploit should be taken. > > I support the idea of getting WebKit to be UBSAN clean, just like we want to > be ASAN clean, Guard Malloc clean, etc. We should clean all the SANs! > > Currently we are prioritizing things with probably paths to exploit, but I > am in favor of making code changes to silence ASAN/TSAN/UBSAN warnings if it > reduces noise so we can identify real problems or new regressions. Just want to +1 Brent's comment here. Experience shows that WebKit is the easiest engine for Project Zero to exploit right now, by a lot, using a *publicly-available fuzzer* [1] that we just never bothered setting up infrastructure to run. We shouldn't be relying on security researchers to find vulnerabilities for us that we could easily find ourselves using the same tools. So it's great that Sony wants to help set up some security CI infrastructure. WebKit should be running as many static and dynamic analysis bots as possible. Right now, without bots, we are at a security disadvantage. Then, once we have bots, we can see how much effort will be required to fix the issues found by the bots, including false positives. It might be annoying to fix or suppress false positive warnings indicated by a tool that you don't personally use -- I've never run WebKit under ubsan myself -- but these tools are so valuable for proactively finding security issues that it makes sense for WebKit to change to placate them. Besides GuardMalloc and the LLVM sanitizers, we should also pay more attention to the Coverity scan results Red Hat provides us with (bug #104114) and the fuzzinator results (bug #116980) provided by Renata. We should also consider which publicly-available fuzzers we could be running, starting with Domato. [1] https://googleprojectzero.blogspot.com/2018/10/365-days-later-finding-and-exploiting.html (In reply to Don Olmstead from comment #4) > In the long term we'd like to spin up bots running the different sanitizers > to continuously look for issues. An issue there is that I don't know that we > could lock down the logs to a bot if it were attached to build.webkit.org. > I've talked a bit about this with Brent. I think static and dynamic analysis CI should be a priority goal for the WebKit project right now due to its potential to reduce security issues in WebKit. The main questions are to what extent such bots can integrated into WebKit infrastructure, whether their results should be public, and which contributors should have access to them if they're not.
(In reply to Michael Catanzaro from comment #17) > (In reply to Yusuke Suzuki from comment #16) > > Filed an issue enabling -fwrapv / d2UndefIntOverflow. I'll look into it at > > weekend as my hobby project. > > https://bugs.webkit.org/show_bug.cgi?id=196829 > > > > It seems that JF attempted to make signed-overflow non-UB, but the sad thing > > is that it is rejected by WG21 :( > > Thanks Yusuke. But does -fwrapv cause ubsan to not complaining about signed > overflow? I would guess not? Interesting question. I don't think if matters, if we can fix all of the issues that ubsan would find by just making the compiler do the right thing in the first place. Remember: UB exists in C/C++ only because the spec committee is really really bad at programming language design. It's not a problem to make the compiler do the right thing. If we're going to be investing in custom compiler tech to *catch* these issues, we should make that tech also fix them for us. > > (In reply to Brent Fulgham from comment #8) > > I think that Sony's goals here are really well aligned with Apple's. WebKit > > is the primary source of system exploits on our systems, too, so anything we > > can do to deny attackers an avenue for exploit should be taken. > > > > I support the idea of getting WebKit to be UBSAN clean, just like we want to > > be ASAN clean, Guard Malloc clean, etc. We should clean all the SANs! > > > > Currently we are prioritizing things with probably paths to exploit, but I > > am in favor of making code changes to silence ASAN/TSAN/UBSAN warnings if it > > reduces noise so we can identify real problems or new regressions. > > Just want to +1 Brent's comment here. Experience shows that WebKit is the > easiest engine for Project Zero to exploit right now, by a lot, using a > *publicly-available fuzzer* [1] that we just never bothered setting up > infrastructure to run. Please, don't presume that just because it's not set up in the open, that someone who contributes to WK didn't set it up. If a fuzzer was finding security vulnerabilities then it wouldn't necessarily make sense to have all of those fuzzers findings be immediately broadcast to the world. > We shouldn't be relying on security researchers to > find vulnerabilities for us that we could easily find ourselves using the > same tools. I agree! I don't think we are solely relying on non-WK security researchers. That's a stretch. But it's great to hear that more WK contributors want to help make us more secure. > So it's great that Sony wants to help set up some security CI > infrastructure. It is great, but ubsan does not qualify as security CI infrastructure. Examples of things that do qualify: - asan - gmalloc - fuzzers (especially totally new ones) - any other sanitizer that actually focuses on security bugs. It's a big stretch to suggest that ubsan belongs on this list because the benefit/cost is so low and because, to my knowledge, anything it finds is actually a C spec bug that can be fixed in the compiler (sometimes it's already fixed and you just need to add a flag). > WebKit should be running as many static and dynamic analysis > bots as possible. Right now, without bots, we are at a security disadvantage. Yeah, sure - but: 1) It's not obvious that those bots have to be in the open. Don't assume that the bots you see in the open are the only ones. 2) If you're going to invest in such technology then please invest in something useful. I.e. not ubsan. > > Then, once we have bots, we can see how much effort will be required to fix > the issues found by the bots, including false positives. That's already happening using analyses that are far more powerful than ubsan. > It might be > annoying to fix or suppress false positive warnings indicated by a tool that > you don't personally use -- I've never run WebKit under ubsan myself -- but > these tools are so valuable for proactively finding security issues that it > makes sense for WebKit to change to placate them. If you're going to have code changes made to WebKit to fix security bugs, then you should make it worth something - i.e. it should be for tools that have high payoff. > > Besides GuardMalloc and the LLVM sanitizers, we should also pay more > attention to the Coverity scan results Red Hat provides us with (bug > #104114) That seems to have caught much fewer bugs than the fuzzers. It seems to find a lot of uninit field issues, but in many cases it looks like the fields would actually be initialized in any real execution. > and the fuzzinator results (bug #116980) provided by Renata. We > should also consider which publicly-available fuzzers we could be running, > starting with Domato. Those are examples of good things to do. For example, getting some of the patches on some of the open bugs under those umbrellas across the finish line is more useful than adding ubsan. > > [1] > https://googleprojectzero.blogspot.com/2018/10/365-days-later-finding-and- > exploiting.html > > (In reply to Don Olmstead from comment #4) > > In the long term we'd like to spin up bots running the different sanitizers > > to continuously look for issues. An issue there is that I don't know that we > > could lock down the logs to a bot if it were attached to build.webkit.org. > > I've talked a bit about this with Brent. > > I think static and dynamic analysis CI should be a priority goal for the > WebKit project right now due to its potential to reduce security issues in > WebKit. The main questions are to what extent such bots can integrated into > WebKit infrastructure, whether their results should be public, and which > contributors should have access to them if they're not. I'm find with y'all bringing up security CI infrastructure. My objection is to UBSan specifically, because any time spent bringing up that pile of junk is pure opportunity cost. You could have brought up something useful instead.
By googling the word 'P1236R1', it seems P1236R1 is accepted for C++2a. P1236R1: Alternative Wording for P0907R4 Signed Integers are Two's Complement http://wg21.link/p1236r1 C++ Standards Support in GCC - GNU Project - Free Software Foundation (FSF) https://gcc.gnu.org/projects/cxx-status.html JF Bastien's comment https://www.youtube.com/watch?v=JhUxIVf1qok&lc=UgzzLXpXghcsOn35ujB4AaABAg