| Message ID | AM5PR0701MB2657DE3C14A57C8A18FD8158E4300@AM5PR0701MB2657.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve checks in c_strlen (PR 87053) | expand |
On 08/22/2018 08:41 AM, Bernd Edlinger wrote: > Hi! > > > This patch adds some more checks to c_getstr to fix PR middle-end/87053 > wrong code bug. > > Unfortunately this patch alone is not sufficient to fix the problem, > but also the patch for PR 86714 that hardens c_getstr is necessary > to prevent the wrong folding. > > > Bootstrapped and reg-tested on top of my PR 86711/86714 patch. > Is it OK for trunk? This case is also the subject of the patch I submitted back in July for 86711/86714 and 86552. With it, GCC avoid folding the strlen call early and warns for the missing nul: warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=] if (__builtin_strlen (u.z) != 7) ^~~~~~~~~~~~~~~~ The patch doesn't doesn't prevent all such strings from being folded and it eventually lets fold_builtin_strlen() do its thing: /* To avoid warning multiple times about unterminated arrays only warn if its length has been determined and is being folded to a constant. */ if (nonstr) warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr); return fold_convert_loc (loc, type, len); Handling this case is a matter of avoiding the folding here as well and moving the warning later. Since my patch is still in the review queue and does much more than just prevent folding of non-nul terminated arrays it should be reviewed first. Martin
On 08/22/18 18:28, Martin Sebor wrote: > On 08/22/2018 08:41 AM, Bernd Edlinger wrote: >> Hi! >> >> >> This patch adds some more checks to c_getstr to fix PR middle-end/87053 >> wrong code bug. >> >> Unfortunately this patch alone is not sufficient to fix the problem, >> but also the patch for PR 86714 that hardens c_getstr is necessary >> to prevent the wrong folding. >> >> >> Bootstrapped and reg-tested on top of my PR 86711/86714 patch. >> Is it OK for trunk? > > This case is also the subject of the patch I submitted back in > July for 86711/86714 and 86552. With it, GCC avoid folding > the strlen call early and warns for the missing nul: > > warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=] > if (__builtin_strlen (u.z) != 7) > ^~~~~~~~~~~~~~~~ > > The patch doesn't doesn't prevent all such strings from being > folded and it eventually lets fold_builtin_strlen() do its thing: > > /* To avoid warning multiple times about unterminated > arrays only warn if its length has been determined > and is being folded to a constant. */ > if (nonstr) > warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr); > > return fold_convert_loc (loc, type, len); > > Handling this case is a matter of avoiding the folding here as > well and moving the warning later. > > Since my patch is still in the review queue and does much more > than just prevent folding of non-nul terminated arrays it should > be reviewed first. > Hmmm, now you made me curious. So I tried to install your patch (I did this on r263508 since it does not apply to trunk, one thing I noted is that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c I did not check if they are identical or not). So I tried the test case from this PR on the compiler built with your patch: $ cat cat pr87053.c /* PR middle-end/87053 */ const union { struct { char x[4]; char y[4]; }; struct { char z[8]; }; } u = {{"1234", "567"}}; int main () { if (__builtin_strlen (u.z) != 7) __builtin_abort (); } $ gcc -S pr87053.c pr87053.c: In function 'main': pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=] 15 | if (__builtin_strlen (u.z) != 7) | ^~~~~~~~~~~~~~~~ pr87053.c:11:3: note: referenced argument declared here 11 | } u = {{"1234", "567"}}; | ^ $ cat pr87053.s .file "pr87053.c" .text .globl u .section .rodata .align 8 .type u, @object .size u, 8 u: .ascii "1234" .string "567" .text .globl main .type main, @function main: .LFB0: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset 6, -16 movq %rsp, %rbp .cfi_def_cfa_register 6 call abort .cfi_endproc .LFE0: .size main, .-main .ident "GCC: (GNU) 9.0.0 20180813 (experimental)" .section .note.GNU-stack,"",@progbits So we get a warning, and still wrong code. That is the reason why I think this patch of yours adds confusion by trying to fix everything in one step. And I would like you to think of ways how to solve a problem step by step. And at this time, sorry, we should restore correctness issues. And fix wrong-code issues. If possible without breaking existing warnings, yes. But no new warnings, sorry again. Bernd.
On 08/22/2018 10:28 AM, Martin Sebor wrote: > On 08/22/2018 08:41 AM, Bernd Edlinger wrote: >> Hi! >> >> >> This patch adds some more checks to c_getstr to fix PR middle-end/87053 >> wrong code bug. >> >> Unfortunately this patch alone is not sufficient to fix the problem, >> but also the patch for PR 86714 that hardens c_getstr is necessary >> to prevent the wrong folding. >> >> >> Bootstrapped and reg-tested on top of my PR 86711/86714 patch. >> Is it OK for trunk? > > This case is also the subject of the patch I submitted back in > July for 86711/86714 and 86552. With it, GCC avoid folding > the strlen call early and warns for the missing nul: > > warning: ‘__builtin_strlen’ argument missing terminating nul > [-Wstringop-overflow=] > if (__builtin_strlen (u.z) != 7) > ^~~~~~~~~~~~~~~~ > > The patch doesn't doesn't prevent all such strings from being > folded and it eventually lets fold_builtin_strlen() do its thing: > > /* To avoid warning multiple times about unterminated > arrays only warn if its length has been determined > and is being folded to a constant. */ > if (nonstr) > warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr); > > return fold_convert_loc (loc, type, len); > > Handling this case is a matter of avoiding the folding here as > well and moving the warning later. > > Since my patch is still in the review queue and does much more > than just prevent folding of non-nul terminated arrays it should > be reviewed first. I think we need to address 86711/86714 first. However, neither approach to 86711/86714 (Bernd's or yours) is sufficient alone to fix this bug. This patch can be layered on top of either approach to 86711/86714 to fix 87053 (I've actually tested that). So let's table this, hopefully for just a day or so. It's getting late and I've got more tests to run on the 86714/86711 patches for comparison purposes and some loose ends I want to look at in Martin's patch. I'd hoped to be finished already, but wasn't able to move things as fast as I hoped. jeff
On 08/23/2018 03:27 AM, Bernd Edlinger wrote: > On 08/22/18 18:28, Martin Sebor wrote: >> On 08/22/2018 08:41 AM, Bernd Edlinger wrote: >>> Hi! >>> >>> >>> This patch adds some more checks to c_getstr to fix PR middle-end/87053 >>> wrong code bug. >>> >>> Unfortunately this patch alone is not sufficient to fix the problem, >>> but also the patch for PR 86714 that hardens c_getstr is necessary >>> to prevent the wrong folding. >>> >>> >>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch. >>> Is it OK for trunk? >> >> This case is also the subject of the patch I submitted back in >> July for 86711/86714 and 86552. With it, GCC avoid folding >> the strlen call early and warns for the missing nul: >> >> warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=] >> if (__builtin_strlen (u.z) != 7) >> ^~~~~~~~~~~~~~~~ >> >> The patch doesn't doesn't prevent all such strings from being >> folded and it eventually lets fold_builtin_strlen() do its thing: >> >> /* To avoid warning multiple times about unterminated >> arrays only warn if its length has been determined >> and is being folded to a constant. */ >> if (nonstr) >> warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr); >> >> return fold_convert_loc (loc, type, len); >> >> Handling this case is a matter of avoiding the folding here as >> well and moving the warning later. >> >> Since my patch is still in the review queue and does much more >> than just prevent folding of non-nul terminated arrays it should >> be reviewed first. >> > > Hmmm, now you made me curious. > > So I tried to install your patch (I did this on r263508 > since it does not apply to trunk, one thing I noted is > that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c > I did not check if they are identical or not). > > So I tried the test case from this PR on the compiler built with your patch: > > $ cat cat pr87053.c > /* PR middle-end/87053 */ > > const union > { struct { > char x[4]; > char y[4]; > }; > struct { > char z[8]; > }; > } u = {{"1234", "567"}}; > > int main () > { > if (__builtin_strlen (u.z) != 7) > __builtin_abort (); > } > $ gcc -S pr87053.c > pr87053.c: In function 'main': > pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=] > 15 | if (__builtin_strlen (u.z) != 7) > | ^~~~~~~~~~~~~~~~ > pr87053.c:11:3: note: referenced argument declared here > 11 | } u = {{"1234", "567"}}; > | ^ > $ cat pr87053.s > .file "pr87053.c" > .text > .globl u > .section .rodata > .align 8 > .type u, @object > .size u, 8 > u: > .ascii "1234" > .string "567" > .text > .globl main > .type main, @function > main: > .LFB0: > .cfi_startproc > pushq %rbp > .cfi_def_cfa_offset 16 > .cfi_offset 6, -16 > movq %rsp, %rbp > .cfi_def_cfa_register 6 > call abort > .cfi_endproc > .LFE0: > .size main, .-main > .ident "GCC: (GNU) 9.0.0 20180813 (experimental)" > .section .note.GNU-stack,"",@progbits > > > So we get a warning, and still wrong code. > > That is the reason why I think this patch of yours adds > confusion by trying to fix everything in one step. > > And I would like you to think of ways how to solve > a problem step by step. > > And at this time, sorry, we should restore correctness issues. > And fix wrong-code issues. > If possible without breaking existing warnings, yes. > But no new warnings, sorry again. Just a note, Martin's most fix for 86711/86714 fixes codegen issues without breaking existing warnings or adding new warnings. The new warnings were broken out into follow-up patches. jeff > Bernd. >
On 08/24/18 07:58, Jeff Law wrote: > On 08/23/2018 03:27 AM, Bernd Edlinger wrote: >> On 08/22/18 18:28, Martin Sebor wrote: >>> On 08/22/2018 08:41 AM, Bernd Edlinger wrote: >>>> Hi! >>>> >>>> >>>> This patch adds some more checks to c_getstr to fix PR middle-end/87053 >>>> wrong code bug. >>>> >>>> Unfortunately this patch alone is not sufficient to fix the problem, >>>> but also the patch for PR 86714 that hardens c_getstr is necessary >>>> to prevent the wrong folding. >>>> >>>> >>>> Bootstrapped and reg-tested on top of my PR 86711/86714 patch. >>>> Is it OK for trunk? >>> >>> This case is also the subject of the patch I submitted back in >>> July for 86711/86714 and 86552. With it, GCC avoid folding >>> the strlen call early and warns for the missing nul: >>> >>> warning: ‘__builtin_strlen’ argument missing terminating nul [-Wstringop-overflow=] >>> if (__builtin_strlen (u.z) != 7) >>> ^~~~~~~~~~~~~~~~ >>> >>> The patch doesn't doesn't prevent all such strings from being >>> folded and it eventually lets fold_builtin_strlen() do its thing: >>> >>> /* To avoid warning multiple times about unterminated >>> arrays only warn if its length has been determined >>> and is being folded to a constant. */ >>> if (nonstr) >>> warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr); >>> >>> return fold_convert_loc (loc, type, len); >>> >>> Handling this case is a matter of avoiding the folding here as >>> well and moving the warning later. >>> >>> Since my patch is still in the review queue and does much more >>> than just prevent folding of non-nul terminated arrays it should >>> be reviewed first. >>> >> >> Hmmm, now you made me curious. >> >> So I tried to install your patch (I did this on r263508 >> since it does not apply to trunk, one thing I noted is >> that part 4 and part 3 seem to create gcc/testsuite/gcc.dg/warn-strcpy-no-nul.c >> I did not check if they are identical or not). >> >> So I tried the test case from this PR on the compiler built with your patch: >> >> $ cat cat pr87053.c >> /* PR middle-end/87053 */ >> >> const union >> { struct { >> char x[4]; >> char y[4]; >> }; >> struct { >> char z[8]; >> }; >> } u = {{"1234", "567"}}; >> >> int main () >> { >> if (__builtin_strlen (u.z) != 7) >> __builtin_abort (); >> } >> $ gcc -S pr87053.c >> pr87053.c: In function 'main': >> pr87053.c:15:7: warning: '__builtin_strlen' argument missing terminating nul [-Wstringop-overflow=] >> 15 | if (__builtin_strlen (u.z) != 7) >> | ^~~~~~~~~~~~~~~~ >> pr87053.c:11:3: note: referenced argument declared here >> 11 | } u = {{"1234", "567"}}; >> | ^ >> $ cat pr87053.s >> .file "pr87053.c" >> .text >> .globl u >> .section .rodata >> .align 8 >> .type u, @object >> .size u, 8 >> u: >> .ascii "1234" >> .string "567" >> .text >> .globl main >> .type main, @function >> main: >> .LFB0: >> .cfi_startproc >> pushq %rbp >> .cfi_def_cfa_offset 16 >> .cfi_offset 6, -16 >> movq %rsp, %rbp >> .cfi_def_cfa_register 6 >> call abort >> .cfi_endproc >> .LFE0: >> .size main, .-main >> .ident "GCC: (GNU) 9.0.0 20180813 (experimental)" >> .section .note.GNU-stack,"",@progbits >> >> >> So we get a warning, and still wrong code. >> >> That is the reason why I think this patch of yours adds >> confusion by trying to fix everything in one step. >> >> And I would like you to think of ways how to solve >> a problem step by step. >> >> And at this time, sorry, we should restore correctness issues. >> And fix wrong-code issues. >> If possible without breaking existing warnings, yes. >> But no new warnings, sorry again. > Just a note, Martin's most fix for 86711/86714 fixes codegen issues > without breaking existing warnings or adding new warnings. The new > warnings were broken out into follow-up patches. > BTW: the warning about u.z not null terminated is bogus. There are middle-end consistency and correctness issues all over. They have IMO precedence even over wrong-code issues. Bernd.
On 08/22/2018 08:41 AM, Bernd Edlinger wrote: > Hi! > > > This patch adds some more checks to c_getstr to fix PR middle-end/87053 > wrong code bug. > > Unfortunately this patch alone is not sufficient to fix the problem, > but also the patch for PR 86714 that hardens c_getstr is necessary > to prevent the wrong folding. > > > Bootstrapped and reg-tested on top of my PR 86711/86714 patch. > Is it OK for trunk? > > > Thanks > Bernd. > > > > patch-pr87053.diff > > > gcc: > 2018-08-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR middle-end/87053 > * builtins.c (c_strlen): Improve range checks. > > testsuite: > 2018-08-22 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR middle-end/87053 > * gcc.c-torture/execute/pr87053.c: New test. This is OK and I'm going to go ahead and commit it momentarily. >> @@ -700,6 +700,10 @@ c_strlen (tree src, int only_value, unsi > unsigned len = string_length (ptr + eltoff * eltsize, eltsize, > strelts - eltoff); > > + /* Don't know what to return if there was no zero termination. */ > + if (len > maxelts - eltoff) > + return NULL_TREE; > + I'm going to add a comment to the code above indicating that it'd be nice if this could be an assert in the future. We're not there yet. Jeff
gcc: 2018-08-22 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/87053 * builtins.c (c_strlen): Improve range checks. testsuite: 2018-08-22 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/87053 * gcc.c-torture/execute/pr87053.c: New test. diff -Npur gcc/builtins.c gcc/builtins.c --- gcc/builtins.c 2018-08-17 05:02:16.000000000 +0200 +++ gcc/builtins.c 2018-08-22 08:51:21.287960030 +0200 @@ -576,7 +576,7 @@ string_length (const void *ptr, unsigned tree c_strlen (tree src, int only_value, unsigned eltsize) { - gcc_assert (eltsize == 1 || eltsize == 2 || eltsize == 4); + gcc_checking_assert (eltsize == 1 || eltsize == 2 || eltsize == 4); STRIP_NOPS (src); if (TREE_CODE (src) == COND_EXPR && (only_value || !TREE_SIDE_EFFECTS (TREE_OPERAND (src, 0)))) @@ -665,10 +665,10 @@ c_strlen (tree src, int only_value, unsi a null character if we can represent it as a single HOST_WIDE_INT. */ if (byteoff == 0) eltoff = 0; - else if (! tree_fits_shwi_p (byteoff)) + else if (! tree_fits_uhwi_p (byteoff) || tree_to_uhwi (byteoff) % eltsize) eltoff = -1; else - eltoff = tree_to_shwi (byteoff) / eltsize; + eltoff = tree_to_uhwi (byteoff) / eltsize; /* If the offset is known to be out of bounds, warn, and call strlen at runtime. */ @@ -700,6 +700,10 @@ c_strlen (tree src, int only_value, unsi unsigned len = string_length (ptr + eltoff * eltsize, eltsize, strelts - eltoff); + /* Don't know what to return if there was no zero termination. */ + if (len > maxelts - eltoff) + return NULL_TREE; + return ssize_int (len); } diff -Npur gcc/testsuite/gcc.c-torture/execute/pr87053.c gcc/testsuite/gcc.c-torture/execute/pr87053.c --- gcc/testsuite/gcc.c-torture/execute/pr87053.c 1970-01-01 01:00:00.000000000 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr87053.c 2018-08-22 12:54:00.801019240 +0200 @@ -0,0 +1,17 @@ +/* PR middle-end/87053 */ + +const union +{ struct { + char x[4]; + char y[4]; + }; + struct { + char z[8]; + }; +} u = {{"1234", "567"}}; + +int main () +{ + if (__builtin_strlen (u.z) != 7) + __builtin_abort (); +}