Bug 78433 - [7 Regression] glibc posix/execvpe.c gets miscompiled with -O3
Summary: [7 Regression] glibc posix/execvpe.c gets miscompiled with -O3
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: 7.0
Assignee: Not yet assigned to anyone
URL: https://sourceware.org/bugzilla/show_...
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2016-11-20 11:38 UTC by Markus Trippelsdorf
Modified: 2016-11-21 11:07 UTC (History)
3 users (show)

See Also:
Host:
Target: x86_64-*-*, ppc64le-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-11-20 00:00:00


Attachments
unreduced testcase (38.63 KB, text/plain)
2016-11-20 11:38 UTC, Markus Trippelsdorf
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Trippelsdorf 2016-11-20 11:38:15 UTC
Created attachment 40088 [details]
unreduced testcase

glibc's posix/execvpe.c gets miscompiled with -O3:

markus@x4 posix % ./tst-vfork3
script 1
script 1
script 1
script 1
script 1
/bin/sh: : No such file or directory
script2.sh failed with status 32512

12912 execve("/tmp/tst-vfork3.Ox2RZ9/script2.sh", ["script2.sh", "2"], [/* 53 vars */]) = -1 ENOEXEC (Exec format error)                                                           
12912 execve("/bin/sh", ["/bin/sh", "\1", "2"], [/* 53 vars */] <unfinished ...> 

Notice the bogus "\1" in the second execve call.

 36 /* The file is accessible but it is not an executable file.  Invoke                                                                                                            
 37    the shell to interpret it as a script.  */                                                                                                                                  
 38 static void                                                                                                                                                                    
 39 maybe_script_execute (const char *file, char *const argv[], char *const envp[])                                                                                                
 40 {                                                                                                                                                                              
 41   ptrdiff_t argc = 0;                                                                                                                                                          
 42   while (argv[argc++] != NULL)                                                                                                                                                 
 43     {                                                                                                                                                                          
 44       if (argc == INT_MAX - 1)                                                                                                                                                 
 45         {                                                                                                                                                                      
 46           errno = E2BIG;                                                                                                                                                       
 47           return;                                                                                                                                                              
 48         }                                                                                                                                                                      
 49     }                                                                                                                                                                          
 50                                                                                                                                                                                
 51   /* Construct an argument list for the shell.  */                                                                                                                             
 52   char *new_argv[argc + 1];                                                                                                                                                    
 53   new_argv[0] = (char *) _PATH_BSHELL;                                                                                                                                         
 54   new_argv[1] = (char *) file;                                                                                                                                                 
 55   if (argc > 1)                                                                                                                                                                
 56     memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));                                                                                                                    
 57   else                                                                                                                                                                         
 58     new_argv[2] = NULL;                                                                                                                                                        
 59                                                                                                                                                                                
 60   /* Execute the shell.  */                                                                                                                                                    
 61   __execve (new_argv[0], new_argv, envp);                                                                                                                                      
 62 }                                                                                                                                                                              
 63                                                                                                                                                                                
 64                                                                                                                                                                                
 65 /* Execute FILE, searching in the `PATH' environment variable if it contains                                                                                                   
 66    no slashes, with arguments ARGV and environment from ENVP.  */                                                                                                              
 67 int                                                                                                                                                                            
 68 __execvpe (const char *file, char *const argv[], char *const envp[])                                                                                                           
 69 {                                                                                                                                                                              
 70   /* We check the simple case first. */                                                                                                                                        
 71   if (*file == '\0')                                                                                                                                                           
 72     {                                                                                                                                                                          
 73       __set_errno (ENOENT);                                                                                                                                                    
 74       return -1;                                                                                                                                                               
 75     }                                                                                                                                                                          
 76                                                                                                                                                                                
 77   /* Don't search when it contains a slash.  */                                                                                                                                
 78   if (strchr (file, '/') != NULL)                                                                                                                                              
 79     {                                                                                                                                                                          
 80       __execve (file, argv, envp);                                                                                                                                             
 81                                                                                                                                                                                
 82       if (errno == ENOEXEC)                                                                                                                                                    
 83         maybe_script_execute (file, argv, envp);                                                                                                                               
 84                                                                                                                                                                                
 85       return -1;                                                                                                                                                               
 86     }                                                                                                                                                                          
 87       

I haven't looked deeper yet.
Comment 1 Markus Trippelsdorf 2016-11-20 12:40:34 UTC
Started with r242590:

commit 35d8be37ee5f4bc13bb3ee30b55c0c379e9a89a2
Author: krebbel <krebbel@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Fri Nov 18 14:44:54 2016 +0000

    Re-apply: Drop excess size used for run time allocated stack variables.
    
    The patch got reverted after hitting PR77359 which turned out to be a
    rs6000 backend problem.  Reapplying after the PR got fixed.
Comment 2 Markus Trippelsdorf 2016-11-20 12:45:26 UTC
On pcc64le this is the assembly diff:

--- good        2016-11-20 12:43:40.318860969 +0000
+++ bad 2016-11-20 12:42:57.277928999 +0000
@@ -178,7 +178,7 @@
        sldi 9,9,3
        ld 10,0(1)
        mr 26,1
-       addi 9,9,38
+       addi 9,9,31
        addis 25,2,.LC2@toc@ha
        rldicr 9,9,0,59
        addi 25,25,.LC2@toc@l
@@ -286,7 +286,7 @@
        sldi 9,9,3
        ld 10,0(1)
        mr 23,1
-       addi 9,9,38
+       addi 9,9,31
        cmpdi 7,5,1
        rldicr 9,9,0,59
        neg 9,9
Comment 3 Markus Trippelsdorf 2016-11-20 17:58:54 UTC
This happens on all targets.
Comment 4 Markus Trippelsdorf 2016-11-20 18:15:38 UTC
On X86_64:

--- good        2016-11-20 19:12:27.353412333 +0100
+++ bad 2016-11-20 19:13:37.738531151 +0100
@@ -892,7 +892,7 @@
        leaq    1(%rax), %rdx
        cmpq    $0, -8(%r12,%rdx,8)
        jne     .L115
-       leaq    38(,%rax,8), %rax
+       leaq    31(,%rax,8), %rax
        movq    %rsp, %r14
        andq    $-16, %rax
        subq    %rax, %rsp
@@ -1009,7 +1009,7 @@
        leaq    1(%rdx), %rax
        cmpq    $0, -8(%r12,%rax,8)
        jne     .L132
-       leaq    38(,%rdx,8), %rdx
+       leaq    31(,%rdx,8), %rdx
        movq    %rsp, -112(%rbp)
        movq    -56(%rbp), %rcx
        andq    $-16, %rdx
Comment 5 Dominik Vogt 2016-11-21 09:35:47 UTC
Is that with any specific version of Glibc?
Comment 6 Markus Trippelsdorf 2016-11-21 09:53:48 UTC
(In reply to Dominik Vogt from comment #5)
> Is that with any specific version of Glibc?

I was using trunk.
Comment 7 Markus Trippelsdorf 2016-11-21 09:57:40 UTC
To reproduce build glibc with -O3 and then run "make check".

Or directly:
 ~ % ~/glibc_build/elf/ld.so --library-path /home/trippels/glibc_build/ ~/glibc_build/posix/tst-vfork3
Comment 8 Dominik Vogt 2016-11-21 10:11:04 UTC
This code from maybe_script_execute() writes past the allocated array bounds:

  /* Construct an argument list for the shell. */
  char *new_argv[argc + 1];
                 ^^^^^^^^
  new_argv[0] = (char *) _PATH_BSHELL;
  new_argv[1] = (char *) file;
  if (argc > 1)
    memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
                    ^^^^^          ^^^^^^
  else
    new_argv[2] = NULL;
            ^^^

Before the patch, Gcc often allocated several bytes more than necessary so that many off-by-one bugs went unnoticed.
Comment 9 Dominik Vogt 2016-11-21 10:51:13 UTC
... and I think the buffer allocated in __execvpe() is also one byte too small:

  char buffer[path_len + file_len + 1];
  ...
  char *pend = mempcpy (buffer, p, subp - p);      <-- path_len
  *pend = '/';                                     <-- path_len + 1
  memcpy (pend + (p < subp), file, file_len + 1);  <-- path_len + file_len + 2
Comment 10 Markus Trippelsdorf 2016-11-21 11:07:03 UTC
Thanks Dominik.
Andreas has reopened the glibc bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20847