[Bug sanitizer/112727] [11/12/13 Regression] UBSAN creates GIMPLE path with uninitialized variable
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112727 Richard Biener changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED Assignee|unassigned at gcc dot gnu.org |jakub at gcc dot gnu.org --- Comment #13 from Richard Biener --- Fixed I guess.
[Bug sanitizer/112727] [11/12/13 Regression] UBSAN creates GIMPLE path with uninitialized variable
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112727 --- Comment #12 from GCC Commits --- The releases/gcc-11 branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:97d6683f6aeda4916a89889bde4c051e55aac984 commit r11-11156-g97d6683f6aeda4916a89889bde4c051e55aac984 Author: Jakub Jelinek Date: Fri Dec 8 20:56:48 2023 +0100 c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727] The following testcase is miscompiled because two ubsan instrumentations run into each other. The first one is the shift instrumentation. Before the C++ FE calls it, it wraps the 2 shift arguments with cp_save_expr, so that side-effects in them aren't evaluated multiple times. And, ubsan_instrument_shift itself uses unshare_expr on any uses of the operands to make sure further modifications in them don't affect other copies of them (the only not unshared ones are the one the caller then uses for the actual operation after the instrumentation, which means there is no tree sharing). Now, if there are side-effects in the first operand like say function call, cp_save_expr wraps it into a SAVE_EXPR, and ubsan_instrument_shift in this mode emits something like if (..., SAVE_EXPR , SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR , ...); and caller adds SAVE_EXPR << SAVE_EXPR after it in a COMPOUND_EXPR. So far so good. If there are no side-effects and cp_save_expr doesn't create SAVE_EXPR, everything is ok as well because of the unshare_expr. We have if (..., SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., ptr->something[i], ...); and ptr->something[i] << SAVE_EXPR where ptr->something[i] is unshared. In the testcase below, the !x->s[j] ? 1 : 0 expression is wrapped initially into a SAVE_EXPR though, and unshare_expr doesn't unshare SAVE_EXPRs nor anything used in them for obvious reasons, so we end up with: if (..., SAVE_EXPR (x)->s[j] ? 1 : 0>, SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR (x)->s[j] ? 1 : 0>, ...); and SAVE_EXPR (x)->s[j] ? 1 : 0> << SAVE_EXPR So far good as well. But later during cp_fold of the SAVE_EXPR we find out that VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1 is actually invariant (has TREE_READONLY set) and so cp_fold simplifies the above to if (..., SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., (bool) VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1, ...); and ((bool) VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1) << SAVE_EXPR with the s[j] ARRAY_REFs and other expressions shared in between the two uses (and obviously the expression optimized away from the COMPOUND_EXPR in the if condition. Then comes another ubsan instrumentation at genericization time, this time to instrument the ARRAY_REFs with strict bounds checking, and replaces the s[j] in there with s[.UBSAN_BOUNDS (0B, SAVE_EXPR, 8), SAVE_EXPR] As the trees are shared, it does that just once though. And as the if body is gimplified first, the SAVE_EXPR is evaluated inside of the if body and when it is used again after the if, it uses a potentially uninitialized value of j.1 (always uninitialized if the shift count isn't out of bounds). The following patch fixes that by unshare_expr unsharing the folded argument of a SAVE_EXPR if we've folded the SAVE_EXPR into an invariant and it is used more than once. 2023-12-08 Jakub Jelinek PR sanitizer/112727 * cp-gimplify.c (cp_fold): If SAVE_EXPR has been previously folded, unshare_expr what is returned. * c-c++-common/ubsan/pr112727.c: New test. (cherry picked from commit 6ddaf06e375e1c15dcda338697ab6ea457e6f497)
[Bug sanitizer/112727] [11/12/13 Regression] UBSAN creates GIMPLE path with uninitialized variable
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112727 --- Comment #11 from GCC Commits --- The releases/gcc-12 branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:2b8f9636c1b19fff7723995f2d58d41f3d30c46d commit r12-10056-g2b8f9636c1b19fff7723995f2d58d41f3d30c46d Author: Jakub Jelinek Date: Fri Dec 8 20:56:48 2023 +0100 c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727] The following testcase is miscompiled because two ubsan instrumentations run into each other. The first one is the shift instrumentation. Before the C++ FE calls it, it wraps the 2 shift arguments with cp_save_expr, so that side-effects in them aren't evaluated multiple times. And, ubsan_instrument_shift itself uses unshare_expr on any uses of the operands to make sure further modifications in them don't affect other copies of them (the only not unshared ones are the one the caller then uses for the actual operation after the instrumentation, which means there is no tree sharing). Now, if there are side-effects in the first operand like say function call, cp_save_expr wraps it into a SAVE_EXPR, and ubsan_instrument_shift in this mode emits something like if (..., SAVE_EXPR , SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR , ...); and caller adds SAVE_EXPR << SAVE_EXPR after it in a COMPOUND_EXPR. So far so good. If there are no side-effects and cp_save_expr doesn't create SAVE_EXPR, everything is ok as well because of the unshare_expr. We have if (..., SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., ptr->something[i], ...); and ptr->something[i] << SAVE_EXPR where ptr->something[i] is unshared. In the testcase below, the !x->s[j] ? 1 : 0 expression is wrapped initially into a SAVE_EXPR though, and unshare_expr doesn't unshare SAVE_EXPRs nor anything used in them for obvious reasons, so we end up with: if (..., SAVE_EXPR (x)->s[j] ? 1 : 0>, SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR (x)->s[j] ? 1 : 0>, ...); and SAVE_EXPR (x)->s[j] ? 1 : 0> << SAVE_EXPR So far good as well. But later during cp_fold of the SAVE_EXPR we find out that VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1 is actually invariant (has TREE_READONLY set) and so cp_fold simplifies the above to if (..., SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., (bool) VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1, ...); and ((bool) VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1) << SAVE_EXPR with the s[j] ARRAY_REFs and other expressions shared in between the two uses (and obviously the expression optimized away from the COMPOUND_EXPR in the if condition. Then comes another ubsan instrumentation at genericization time, this time to instrument the ARRAY_REFs with strict bounds checking, and replaces the s[j] in there with s[.UBSAN_BOUNDS (0B, SAVE_EXPR, 8), SAVE_EXPR] As the trees are shared, it does that just once though. And as the if body is gimplified first, the SAVE_EXPR is evaluated inside of the if body and when it is used again after the if, it uses a potentially uninitialized value of j.1 (always uninitialized if the shift count isn't out of bounds). The following patch fixes that by unshare_expr unsharing the folded argument of a SAVE_EXPR if we've folded the SAVE_EXPR into an invariant and it is used more than once. 2023-12-08 Jakub Jelinek PR sanitizer/112727 * cp-gimplify.cc (cp_fold): If SAVE_EXPR has been previously folded, unshare_expr what is returned. * c-c++-common/ubsan/pr112727.c: New test. (cherry picked from commit 6ddaf06e375e1c15dcda338697ab6ea457e6f497)
[Bug sanitizer/112727] [11/12/13 Regression] UBSAN creates GIMPLE path with uninitialized variable
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112727 --- Comment #10 from GCC Commits --- The releases/gcc-13 branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:a982b9cb690a163434f1ac5a0901548b594205f2 commit r13-8160-ga982b9cb690a163434f1ac5a0901548b594205f2 Author: Jakub Jelinek Date: Fri Dec 8 20:56:48 2023 +0100 c++: Unshare folded SAVE_EXPR arguments during cp_fold [PR112727] The following testcase is miscompiled because two ubsan instrumentations run into each other. The first one is the shift instrumentation. Before the C++ FE calls it, it wraps the 2 shift arguments with cp_save_expr, so that side-effects in them aren't evaluated multiple times. And, ubsan_instrument_shift itself uses unshare_expr on any uses of the operands to make sure further modifications in them don't affect other copies of them (the only not unshared ones are the one the caller then uses for the actual operation after the instrumentation, which means there is no tree sharing). Now, if there are side-effects in the first operand like say function call, cp_save_expr wraps it into a SAVE_EXPR, and ubsan_instrument_shift in this mode emits something like if (..., SAVE_EXPR , SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR , ...); and caller adds SAVE_EXPR << SAVE_EXPR after it in a COMPOUND_EXPR. So far so good. If there are no side-effects and cp_save_expr doesn't create SAVE_EXPR, everything is ok as well because of the unshare_expr. We have if (..., SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., ptr->something[i], ...); and ptr->something[i] << SAVE_EXPR where ptr->something[i] is unshared. In the testcase below, the !x->s[j] ? 1 : 0 expression is wrapped initially into a SAVE_EXPR though, and unshare_expr doesn't unshare SAVE_EXPRs nor anything used in them for obvious reasons, so we end up with: if (..., SAVE_EXPR (x)->s[j] ? 1 : 0>, SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., SAVE_EXPR (x)->s[j] ? 1 : 0>, ...); and SAVE_EXPR (x)->s[j] ? 1 : 0> << SAVE_EXPR So far good as well. But later during cp_fold of the SAVE_EXPR we find out that VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1 is actually invariant (has TREE_READONLY set) and so cp_fold simplifies the above to if (..., SAVE_EXPR > const) __ubsan_handle_shift_out_of_bounds (..., (bool) VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1, ...); and ((bool) VIEW_CONVERT_EXPR(x)->s[j] ? 0 : 1) << SAVE_EXPR with the s[j] ARRAY_REFs and other expressions shared in between the two uses (and obviously the expression optimized away from the COMPOUND_EXPR in the if condition. Then comes another ubsan instrumentation at genericization time, this time to instrument the ARRAY_REFs with strict bounds checking, and replaces the s[j] in there with s[.UBSAN_BOUNDS (0B, SAVE_EXPR, 8), SAVE_EXPR] As the trees are shared, it does that just once though. And as the if body is gimplified first, the SAVE_EXPR is evaluated inside of the if body and when it is used again after the if, it uses a potentially uninitialized value of j.1 (always uninitialized if the shift count isn't out of bounds). The following patch fixes that by unshare_expr unsharing the folded argument of a SAVE_EXPR if we've folded the SAVE_EXPR into an invariant and it is used more than once. 2023-12-08 Jakub Jelinek PR sanitizer/112727 * cp-gimplify.cc (cp_fold): If SAVE_EXPR has been previously folded, unshare_expr what is returned. * c-c++-common/ubsan/pr112727.c: New test. (cherry picked from commit 6ddaf06e375e1c15dcda338697ab6ea457e6f497)
[Bug sanitizer/112727] [11/12/13 Regression] UBSAN creates GIMPLE path with uninitialized variable
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112727 Jakub Jelinek changed: What|Removed |Added Summary|[11/12/13/14 Regression]|[11/12/13 Regression] UBSAN |UBSAN creates GIMPLE path |creates GIMPLE path with |with uninitialized variable |uninitialized variable --- Comment #9 from Jakub Jelinek --- Fixed on the trunk so far.
