Bug 2160682 - Bogus memmove() abort with _FORTIFY_SOURCE=3
Summary: Bogus memmove() abort with _FORTIFY_SOURCE=3
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: gcc
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jakub Jelinek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 2160077
TreeView+ depends on / blocked
 
Reported: 2023-01-13 10:14 UTC by Petr Pisar
Modified: 2023-01-16 11:09 UTC (History)
18 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-01-13 20:26:47 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
GNU Compiler Collection 108398 0 P3 UNCONFIRMED tree-object-size trips up with pointer arithmetic if an intermediate result is an invalid pointer 2023-01-13 17:44:36 UTC

Description Petr Pisar 2023-01-13 10:14:42 UTC
After Fedora 38 enabled increased _FORTIFY_SOURCE to 3, a perl-Prima-1.67-1.fc38 test started to abort on a memmove() call (bug #2160077, <https://github.com/dk/Prima/issues/78>). Here is a minimized reproducer:

#include <stdio.h>
#include <string.h>

typedef struct {
    int cmd;
    int p;
} ImgOp;

typedef struct {
    int n_steps;
    ImgOp steps[2];
} ImgOpPipeline;

int main(void) {
    ImgOpPipeline iop = {
        .n_steps = 2,
        .steps = {
            {.cmd = 0, .p = 0},
            {.cmd = 0, .p = 0},
        }
    };
    ImgOp *io;

    printf("sizeof(oip.steps)=%zu\n", sizeof(iop.steps));
    printf("Initial iop.steps=%p past(iop.steps)=%p iop.n_steps=%d\n",
            iop.steps, (char*)iop.steps + sizeof(iop.steps), iop.n_steps);

    for (io = iop.steps; 0 < iop.n_steps; io++) {
        printf("New iteration iop.n_steps=%d, io=%p, io->cmd=%d\n", iop.n_steps, io, io->cmd);
        switch (io->cmd) {
        case 0:
            printf("Calling memmove(io=%p, io+1=%p, sizeof(ImgOp)=%zu)\n",
                    io, io + 1, sizeof(ImgOp));
            memmove(io, io + 1, sizeof(ImgOp));
            printf("memmove() finished\n");
            printf("Decreasing iop.n_steps, io\n");
            iop.n_steps--;
            io--;
            break;
        }
    }

    return 0;
}

When compiled like this, it aborts:

$ gcc -Wp,-D_FORTIFY_SOURCE=3 -g -O1 /tmp/test.c -o /tmp/a.out; /tmp/a.out
sizeof(oip.steps)=16
Initial iop.steps=0x7ffce792a754 past(iop.steps)=0x7ffce792a764 iop.n_steps=2
New iteration iop.n_steps=2, io=0x7ffce792a754, io->cmd=0
Calling memmove(io=0x7ffce792a754, io+1=0x7ffce792a75c, sizeof(ImgOp)=8)
memmove() finished
Decreasing iop.n_steps, io
New iteration iop.n_steps=1, io=0x7ffce792a754, io->cmd=0
Calling memmove(io=0x7ffce792a754, io+1=0x7ffce792a75c, sizeof(ImgOp)=8)
*** buffer overflow detected ***: terminated
Aborted (core dumped)

I believe that memmove() does not read or write out of the allocated object.

The abort disappears If I remove the switch condition, or change it into an if condition. It also disappears if I move "io--;" decrement past the switch block, or if I remove "p" element from ImgOp structure.

It seems there is a misoptimization related to the io pointer.

I hazily recall that C specification forbids a pointer arithmetic out of boundary of a pointed object. But I always thought that it only applies to dereferencing the pointer. Here in this code the io pointer is invariant when dereferenced.

I use glibc-2.36.9000-19.fc38.x86_64 and gcc-12.2.1-4.implicits.4.fc38.x86_64. My gcc is from a Florian's instrumented fork, but it happens also with a stock Fedora 38 gcc as you can see in Koschei <https://koschei.fedoraproject.org/package/perl-Prima>.

Comment 1 Siddhesh Poyarekar 2023-01-13 16:21:14 UTC
(In reply to Petr Pisar from comment #0)
> After Fedora 38 enabled increased _FORTIFY_SOURCE to 3, a
> perl-Prima-1.67-1.fc38 test started to abort on a memmove() call (bug
> #2160077, <https://github.com/dk/Prima/issues/78>). Here is a minimized
> reproducer:
> 
> #include <stdio.h>
> #include <string.h>
> 
> typedef struct {
>     int cmd;
>     int p;
> } ImgOp;
> 
> typedef struct {
>     int n_steps;
>     ImgOp steps[2];
> } ImgOpPipeline;
> 
> int main(void) {
>     ImgOpPipeline iop = {
>         .n_steps = 2,
>         .steps = {
>             {.cmd = 0, .p = 0},
>             {.cmd = 0, .p = 0},
>         }
>     };
>     ImgOp *io;
> 
>     printf("sizeof(oip.steps)=%zu\n", sizeof(iop.steps));
>     printf("Initial iop.steps=%p past(iop.steps)=%p iop.n_steps=%d\n",
>             iop.steps, (char*)iop.steps + sizeof(iop.steps), iop.n_steps);
> 
>     for (io = iop.steps; 0 < iop.n_steps; io++) {
>         printf("New iteration iop.n_steps=%d, io=%p, io->cmd=%d\n",
> iop.n_steps, io, io->cmd);
>         switch (io->cmd) {
>         case 0:
>             printf("Calling memmove(io=%p, io+1=%p, sizeof(ImgOp)=%zu)\n",
>                     io, io + 1, sizeof(ImgOp));
>             memmove(io, io + 1, sizeof(ImgOp));
>             printf("memmove() finished\n");
>             printf("Decreasing iop.n_steps, io\n");
>             iop.n_steps--;
>             io--;

Wouldn't that underflow?  Maybe that was supposed to be io++ ?

Comment 2 Siddhesh Poyarekar 2023-01-13 16:26:42 UTC
Oh wait, I missed the io++ there, never mind.  Let me take a look at the compiler dumps.

Comment 3 Siddhesh Poyarekar 2023-01-13 16:41:20 UTC
OK, moving to gcc; this looks like the same problem as https://github.com/Perl/perl5/issues/20678 where undefined behaviour is invoked very briefly, causing the object size computation to go berserk.  Basically in this loop:

    for (ImgOp *io = iop.steps; 0 < iop.n_steps; io++) {
        printf("New iteration io=%p, iop=%p, iop.steps=%p\n", io, &iop, iop.steps);
        switch (io->cmd) {
	case 0:
            memmove(io, io + 1, sizeof(ImgOp));
            iop.n_steps--;
	    io--;
	    break;
        }
    }

The io-- in the first iteration sends the pointer outside the object before the io++ brings it back.  This messes with the object size computation at the boundary because the first iteration ends up with an object size of zero thanks to the underflow.  The strictly correct version of the loop would be:

    for (ImgOp *io = iop.steps; 0 < iop.n_steps;) {
        printf("New iteration io=%p, iop=%p, iop.steps=%p\n", io, &iop, iop.steps);
        switch (io->cmd) {
	case 0:
            memmove(io, io + 1, sizeof(ImgOp));
            iop.n_steps--;
	    break;
	default:
	    io++;
        }

where the pointer always stays within the target object.

On the one hand I want to congratulate myself for finding a new (seemingly not that uncommon) undefined behaviour that seems to be in common use but on the other hand, I feel like this should fall under the same category as the pointer use after realloc case:

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

clang seems to do the right thing in this case, so maybe there's a case for gcc to try and support this too?

Comment 4 Siddhesh Poyarekar 2023-01-13 17:09:06 UTC
Actually no, this is probably a bug in __bdos; it doesn't even do the right thing if the condition is never taken, i.e. cmd != 0.  I'll file it upstream and work on fixing it.

Comment 5 Siddhesh Poyarekar 2023-01-13 20:26:47 UTC
For more context, see the upstream gcc bug.  Basically more than just I think that this is undefined behaviour, so I've posted a PR to Prima for a fix instead.

Comment 6 Petr Pisar 2023-01-16 11:09:03 UTC
Thanks. So at the end a pointer arithmetic resulting into a pointer value out of an array (except one past the last element) is an undefined behavior (paragraph 8, section 6.5.6 of C17 standard). Dereference is not required.


Note You need to log in before you can comment on or make changes to this bug.