Storing difference of pointers in I32

[hvds]

As mentioned in perlmonks, we have this concept repeatedly in inline.h (eg line 2951 in blead at dab4006):

    PL_curstackinfo->si_cxsubix = cx - PL_curstackinfo->si_cxstack;

PL_curstackinfo->si_cxsubix is declared as I32 (in cop.h as part of struct stackinfo), so at first glance this appears to be unsafe when we have 64-bit pointers. I don't know if this just needs a cast (possibly with overflow checks), or whether si_cxsubix and its old_cxsubix derivatives need to be a different type such as ptrdiff_t.

The perlmonks report complains that this results in many warnings when building with MSVC on an appropriate platform; it isn't clear to me why gcc doesn't complain.

[iabyn]

On Tue, Feb 21, 2023 at 03:40:13PM -0800, Hugo van der Sanden wrote: As mentioned in [perlmonks](https://perlmonks.org/index.pl?node_id=11150515), we have this in `inline.h` (in blead at dab4006): ``` PL_curstackinfo->si_cxsubix = cx - PL_curstackinfo->si_cxstack; ``` `PL_curstackinfo->si_cxsubix` is declared as `I32` (in `cop.h` as part of `struct stackinfo`), so at first glance this appears to be unsafe when we have 64-bit pointers. I don't know if this just needs a cast (possibly with overflow checks), or whether `si_cxsubix` and its `old_cxsubix` derivatives need to be a different type such as `ptrdiff_t`. The perlmonks report complains that this results in many warnings when building with MSVC on an appropriate platform; it isn't clear to me why gcc doesn't complain.

This was my code. Using a 32-bit offset seems reasonable, unless we are ever going to have more than 2 billion nested scopes; in which case all the other I32 stack stuff will have blown up first. What I don't understand is why that compiler hasn't complained about all the other I32 stack stuff we do. For example we have a plenty of code like const I32 markoff = MARK - PL_stack_base; But, judging from this in pp.h: #define dORIGMARK const I32 origmark = (I32)(mark - PL_stack_base) an (I32) cast seems to be the way to go.

…

-- You live and learn (although usually you just live).

[tonycoz]

What I don't understand is why that compiler hasn't complained about all the other I32 stack stuff we do. For example we have a plenty of code like const I32 markoff = MARK - PL_stack_base;

MSVC does warn, for example for PL_curstackinfo->si_cxsubix = cx - PL_curstackinfo->si_cxstack;:

D:\a\perl5\perl5\inline.h(2815): warning C4244: '=': conversion from '__int64' to 'I32', possible loss of data

We can enable extra conversion warnings for gcc with -Wconversion which isn't included in -Wall nor in -Wextra:

inline.h: In function ‘Perl_cx_pushsub’:
inline.h:2815:35: warning: conversion from ‘long int’ to ‘I32’ {aka ‘int’} may change value [-Wconversion]
 2815 |     PL_curstackinfo->si_cxsubix = cx - PL_curstackinfo->si_cxstack;
      |                                   ^~

MSVC produces a lot of conversion warnings for perl.

and clang:

In file included from av.c:20:
In file included from ./perl.h:7781:
./inline.h:2815:38: warning: implicit conversion loses integer precision: 'long' to 'I32' (aka 'int') [-Wshorten-64-to-32]
    PL_curstackinfo->si_cxsubix = cx - PL_curstackinfo->si_cxstack;
                                ~ ~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

both gcc and clang -Wconversion also warns on signed changes:

In file included from av.c:20:
In file included from ./perl.h:7781:
./inline.h:213:19: warning: implicit conversion changes signedness: 'ssize_t' (a
ka 'long') to 'unsigned long' [-Wsign-conversion]
        Zero(ary, size, SV*);
        ~~~~~~~~~~^~~~~~~~~~

There's a MSVC warning we can enable for that too.

[mubinskt]

Regardless of why the warning does or doesn't show up, using the wrong variable size for the pointer can lead to incorrect functionality as and address truncation isn't the same as a value truncation where it's impact is only if you exceed the max value for the smaller variable

[demerphq]

@mubinskt I think you have neglected to read @iabyn's comment on this. These aren't arbitrary pointers that are being subtracted here, there are pointers into an array that is used as a stack. The computation at hand is computing how far apart two items in an array (used as a stack) are from each other. For that to overflow an I32 the stack would have to be huge.

[khwilliamson]

On 3/7/23 08:17, Yves Orton wrote: @mubinskt <https://github.com/mubinskt> I think you have neglected to read @iabyn <https://github.com/iabyn>'s comment on this. These aren't arbitrary pointers that are being subtracted here, there are pointers into an array that is used as a stack. The computation at hand is computing how far apart two items in an array (used as a stack) are from each other. For that to overflow an I32 the stack would have to be huge.

It is undefined behavior to subtract pointers not to the same data structure. The proper type for subtracting any pointers is technically ptrdiff_t, but IMO the Standards are defective here, because that type need not be big enough to hold every possible outcome from same-structure subtraction. But I can't imagine the stack legitimately exceeding an I32 size.

…

— Reply to this email directly, view it on GitHub <#20841 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA2DH6KQBNCNVTBIQKK66TW25GQJANCNFSM6AAAAAAVDUR3XU>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[tonycoz]

But I can't imagine the stack legitimately exceeding an I32 size.

The stack being discussed here is the context stack, which has PERL_CONTEXT as entries, each of which is >80 bytes in size on 64-bit platforms, so I think that yes, that's unlikely to exceed $2^{31}$ entries.

I could see the value stack (which is often just referred to as "the stack") exceeding the limit of an I32 pretty easily:

$ ./perl -e 'my @x; $x[0x8000_0000] = 1; sub x { @x } my @y = x(); printf "%x\n", scalar(@y)'
Segmentation fault

Oops.

The stack here has over $2^{31}$ entries, but the entry in the mark stack has been truncated:

Breakpoint 1, Perl_pp_aassign () at pp_hot.c:2316
2316        dSP;
(gdb) p/x PL_stack_sp - PL_stack_base
$1 = 0x80000002
(gdb) p PL_markstack
$1 = (I32 *) 0x555555a4d560
(gdb) p PL_markstack_ptr
$2 = (I32 *) 0x555555a4d568
(gdb) p PL_markstack_ptr[0]
$3 = -2147483647
(gdb) p PL_markstack_ptr[-1]
$4 = 0

• the machine runs out of memory with a slightly smaller array creating entries in @y with 64GB of RAM. A user may have a machine with more memory.

[demerphq]

@tonycoz will you follow-up with a patch for the second case to fix the segfault?

[tonycoz]

I'll take a look at it, I've opened #20917 to track that specifically.

It's kind of part of this issue, but a lot of this issue isn't related to marks.