exec: avoid possible undefined behavior in count()
Subsystems (2)
RFC PATCH 1/1 exec: avoid possible undefined behavior in count()
The tricky problem is this check:
if (i++ >= max)
icc (mis)optimizes this check as:
if (++i > max)
The check now becomes a no-op since max is MAX_ARG_STRINGS (0x7FFFFFFF).
This is "allowed" by the C standard, assuming i++ never overflows, because signed integer overflow is undefined behavior. This optimization effectively reverts the previous commit 362e6663ef ("exec.c, compat.c:
fix count(), compat_count() bounds checking") that tries to fix the check.
This patch simply moves ++ after the check.
---
Not sure how many people are using Intel's icc to compiled the kernel. Some projects like LinuxDNA did.
The kernel uses gcc's -fno-strict-overflow to disable this optimization. icc probably doesn't recognize the option.
To illustrate the problem, try this simple program:
int count(int i, int max)
{
if (i++ >= max) {
__builtin_trap();
return -1;
}
return i;
}#include <stdio.h>
#include <stdlib.h>
int main(int argc, char **argv)
{
int x = atoi(argv[1]);
int max = atoi(argv[2]);
printf("%d %d %d\n", x, max, count(x, max));
}$ gcc -O2 t.c $ ./a.out 2147483647 2147483647 Illegal instruction (core dumped)
$ icc -O2 t.c $ ./a.out 2147483647 2147483647 2147483647 2147483647 -2147483648
There's no difference whether we add -fno-strict-overflow or not.
--- fs/exec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/exec.c +2 -1
@@ -434,8 +434,9 @@ static int count(struct user_arg_ptr argv, int max)if (IS_ERR (p))return -EFAULT ; -if (i++ >= max) +if (i >= max)return -E2BIG ; + ++i;if (fatal_signal_pending (current))return -ERESTARTNOHAND ;
Signature · 1 line
1.7.10.4
Quoted 22 lines · On Sun, 6 Jan 2013 00:29:05 -0500 Xi Wang <[email protected]> wrote:
The tricky problem is this check:
if (i++ >= max)icc (mis)optimizes this check as:
if (++i > max)The check now becomes a no-op since max is
MAX_ARG_STRINGS(0x7FFFFFFF).This is "allowed" by the C standard, assuming i++ never overflows, because signed integer overflow is undefined behavior. This optimization effectively reverts the previous commit
362e6663ef("exec.c,compat.c:
fixcount(),compat_count()bounds checking") that tries to fix the check.This patch simply moves ++ after the check.
...
fs/exec.c static int count(struct user_arg_ptr argv, int max) · line 442
Quoted 10 lines
fs/exec.c +2
-1@@ -434,8 +434,9 @@ static int count(struct user_arg_ptr argv, int max)if (IS_ERR (p))return -EFAULT ; -if (i++ >= max) +if (i >= max)return -E2BIG ; + ++i;if (fatal_signal_pending (current))return -ERESTARTNOHAND ;
I have no problem working around a compiler bug when the workaround is so small and simple. For clarity and accuracy I renamed the patch to "fs/exec.c: work around icc miscompilation".
However I'd also like to be able to add "this bug has been reported to the icc developers and will be fixed in version X.Y"?
Quoted 3 lines · Andrew Morton
I have no problem working around a compiler bug when the workaround is so small and simple. For clarity and accuracy I renamed the patch to "
fs/exec.c: work around icc miscompilation".
Thanks!
Quoted 2 lines
However I'd also like to be able to add "this bug has been reported to the icc developers and will be fixed in version X.Y"?
The icc developers have confirmed this bug and filed a defect ticket. I'll let you know if there's any further update.
- xi