Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid codegen / dangling pointer for openArray escaping from block #24261

Open
arnetheduck opened this issue Oct 8, 2024 · 2 comments
Open

Comments

@arnetheduck
Copy link
Contributor

arnetheduck commented Oct 8, 2024

Description

The following snippet results in a dangling pointer for the array access:

type ArrayBuf = object
  buf: array[32, byte]
  n: int

template data*(bParam: ArrayBuf): openArray =
  block:
    let b = bParam
    b.buf.toOpenArray(0, b.n - 1)

proc leaky() =
  var a: ArrayBuf

  echo a.data()

leaky()
N_LIB_PRIVATE N_NIMCALL(void, _ZN6testit5leakyE)(void) {
  tyObject_ArrayBuf__3ViybJCcjsNgggY4gNLhwQ a;
  tyArray__nHXaesL0DJZHyVS07ARPRA T1_;
  tyOpenArray__UMVJID9bgFAzHOc9bt5jE4PA T2_;
  nimZeroMem((void *)(&a), sizeof(tyObject_ArrayBuf__3ViybJCcjsNgggY4gNLhwQ));
  nimZeroMem((void *)T1_, sizeof(tyArray__nHXaesL0DJZHyVS07ARPRA));
  {
    tyObject_ArrayBuf__3ViybJCcjsNgggY4gNLhwQ bX60gensym0_;
    NI TM__8qVWOq10n9bbKER8oGDqhuw_2;
    nimZeroMem((void *)(&bX60gensym0_),
               sizeof(tyObject_ArrayBuf__3ViybJCcjsNgggY4gNLhwQ));
    bX60gensym0_ = a;
    if (nimSubInt(bX60gensym0_.n, ((NI)1), &TM__8qVWOq10n9bbKER8oGDqhuw_2)) {
      raiseOverflow();
    };
    if ((NI)(TM__8qVWOq10n9bbKER8oGDqhuw_2) - ((NI)0) != -1 &&
        ((NI)(TM__8qVWOq10n9bbKER8oGDqhuw_2) - ((NI)0) < -1 || ((NI)0) < 0 ||
         ((NI)0) > 31 || (NI)(TM__8qVWOq10n9bbKER8oGDqhuw_2) < 0 ||
         (NI)(TM__8qVWOq10n9bbKER8oGDqhuw_2) > 31)) {
      raiseIndexError();
    }
    T2_.Field0 = (NU8 *)((bX60gensym0_.buf) + (((NI)0)));
    T2_.Field1 = ((NI)(TM__8qVWOq10n9bbKER8oGDqhuw_2)) - (((NI)0)) + 1;
  }
  T1_[0] = _ZN7dollars7dollar_E9openArrayI5uInt8E(T2_.Field0, T2_.Field1);
  echoBinSafe(T1_, 1);
}

Here, tyObject_ArrayBuf__3ViybJCcjsNgggY4gNLhwQ bX60gensym0_; is generated inside a {} block and the pointer T2_.Field0 is allowed to escape from out of that block.

Because bX60gensym0_ has gone out of scope, this allows the C compiler to reuse the stack space for other purposes which results in data corruption when T2_ is accessed.

Nim Version

2.0.8

Current Output

No response

Expected Output

No response

Known Workarounds

No response

Additional Information

No response

@metagn
Copy link
Collaborator

metagn commented Oct 8, 2024

let b = bParam probably copies if bParam is used again. This means the result of data which derives from b.buf outlives b. The compiler could detect and disallow this but the only real alternative is let b = addr bParam (or a template which acts on the addr) which may not be universally available and might violate func rules with --experimental:strictFuncs. Maybe the "proper" alternative is let b: lent = bParam but this would need --experimental:views.

@arnetheduck
Copy link
Contributor Author

let b = bParam probably copies if bParam is used again.

A copy here is fine / expected. ie bParam could be a temporary, such as the return value of a function.

The compiler could detect

this is the bug. semantically, openArray is supposed to be "safe" and not allow dangling references, ever - that's really the only advantage it has really over a pointer, otherwise it's just a complexity-introducing construct with an ugly name.

The compiler can do two things: extend the lifetime of b (this is what C++ does) or disallow it (the easier option).

Of course, actually making view types work would be nice, though something tells me this is more work - ie the problem is the same: view types have to have correct lifetime analysis or they run into the same problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants