[Bug c/98404] New: Compiler emits unexpected function call that may cause security problems

[Bug c/98404] New: Compiler emits unexpected function call that may cause security problems

[msharov at users dot sourceforge.net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98404

            Bug ID: 98404
           Summary: Compiler emits unexpected function call that may cause
                    security problems
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: msharov at users dot sourceforge.net
  Target Milestone: ---

int rotate_argv (const char** argv, int first, int mid, int end)
{
    const char** p = argv+first;
    int n1 = mid-first;
    int n2 = end-mid;
    int nm = n1+n2-1;
    for (int j = 0; j < n2; ++j) {
        const char* v = p[nm];
        for (int i = 0; i < nm; ++i)
            p[nm-i] = p[nm-i-1];
        p[0] = v;
    }
    return n1;
}

This bit of code unexpectedly emits a call to memmove to replace the inner copy
loop. Such behavior is highly inappropriate, breaking the
"what-you-see-is-what-you-get" spirit of C. Sure, the loop is equivalent to a
memmove call, but if I wanted to call memmove, I would have called memmove.
Doing it behind my back brings in code paths that may cause problems impossible
to understand by looking at the code. Worse yet, the compiler only does this in
the optimized build (-Os, -O2, and -O3, but not -O1 or -O0), making debugging
of the resulting problem a beat-your-head-on-the-desk frustrating exercise.

The bug in my code was causing memory corruption in argv to happen in that
inner loop, but looking at the code above will not reveal the problem, no
matter how much you scream at the debugger. The bug was in my memmove
implementation returning the wrong value, which the compiler then helpfully
reloaded into p. Naturally, it's a good thing that I fixed the bug; having
never used the return value of memmove myself I doubt I would have discovered
it anytime soon. But this illustrates how a malicious exploit could be
introduced into that loop without anybody being able to figure it out. Let's
remember that we still have that LD_PRELOAD abomination.

On a more mundane note, replacing the loop with memmove causes the compiled
code to grow from 107 bytes to 166. This is using the -Os, switch, of course. I
have complained many times about how gcc doesn't care about size optimization
and doesn't inline stuff because it can't understand that inserting a function
call into code that currently has none has great costs of register saving and
all that. I have by now resigned to having to #define inline inline
__attribute__((always_inline)) everywhere, but will you perhaps someday
reconsider your position that size optimization does not matter? If 55% code
bloat in this example doesn't convince you, what will?

Finally, calling memmove will make the code slower, not faster, due to its much
higher startup overhead that is justifiable for copying large blocks, but not
for copying one or two elements, which is what the code above is made for. The
conceit of the compiler, in thinking it knows better, thus results in worse
outcome all around; in size, speed, and security.

[Bug c/98404] Compiler emits unexpected function call that may cause security problems

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98404

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
NOT A BUG.  GCC implements non-freestanding implementation of the C langauge
which means it can emit memcpy/memmove and other functions that part of the C
language.  If your libc has a bug in it, it is not a GCC bug as you found out. 
The security bug happens only if your libc has a bug.

[Bug c/98404] Compiler emits unexpected function call that may cause security problems

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98404

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
You can always -fno-tree-loop-distribute-patterns.

And as for -Os, especially in the earlier optimizations, GCC can't really know
if a particular optimization will for a particular code result in larger or
smaller code, it needs to use heuristics, which is tuned for improving code
size in general, but can always misfire on some particular testcases.  For
inlining, GCC has -Os heuristics and estimation if function will grow or
decrease overall size.

[Bug c/98404] ldist might punt on too large expressions for detected patterns with -Os

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98404

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Compiler emits unexpected   |ldist might punt on too
                   |function call that may      |large expressions for
                   |cause security problems     |detected patterns with -Os
     Ever confirmed|0                           |1
                 CC|                            |rguenth at gcc dot gnu.org
   Last reconfirmed|                            |2020-12-21
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Well, for -Os I guess we should reopen this for the case where the expressions
needed for the detected patterns look too big.
ldist in this case turns the loop into:
  _34 = _3 + -1;
  _33 = (unsigned int) _34;
  _32 = (sizetype) _33;
  _35 = _32 * 8;
  _36 = _3 > 0 ? _35 : 0;
  _37 = (long unsigned int) nm_25;
  _38 = _37 * 8;
  _39 = (sizetype) _33;
  _40 = 1 - _39;
  _41 = _40 * 8;
  _42 = _3 > 0 ? _41 : 8;
  _43 = _38 + _42;
  _44 = p_20 + _43;
  _45 = (long unsigned int) nm_25;
  _46 = _45 * 8;
  _47 = (unsigned int) _34;
  _48 = (sizetype) _47;
  _49 = 1 - _48;
  _50 = _49 * 8;
  _51 = _3 > 0 ? _50 : 8;
  _52 = _46 + _51;
  _53 = _52 + 18446744073709551608;
  _54 = p_20 + _53;
  __builtin_memmove (_44, _54, _36);
and while some of it is optimized later, we still have:
  _4 = (long unsigned int) nm_25;
  _5 = _4 * 8;
  _6 = p_20 + _5;
  _33 = (unsigned int) nm_25;
  _32 = (sizetype) _33;
  _35 = _32 * 8;
  _36 = _3 > 0 ? _35 : 0;
  _40 = 1 - _32;
  _41 = _40 * 8;
  _42 = _3 > 0 ? _41 : 8;
  _43 = _5 + _42;
  _44 = p_20 + _43;
  _9 = _5 + 18446744073709551608;
  _53 = _9 + _42;
  _54 = p_20 + _53;
in *.optimized.