Bug 2160682

(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++ ?

Oh wait, I missed the io++ there, never mind.  Let me take a look at the compiler dumps.

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?

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.

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.

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.