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

UB in OperandStack #583

Closed
chfast opened this issue Oct 6, 2020 · 3 comments · Fixed by #630
Closed

UB in OperandStack #583

chfast opened this issue Oct 6, 2020 · 3 comments · Fixed by #630
Labels
bug Something isn't working

Comments

@chfast
Copy link
Collaborator

chfast commented Oct 6, 2020

There is potential undefined behavior in OperandStack when no locals. Then the stack top pointer is set to m_bottom - 1. This is undefined behavior because you are allowed to move pointer within the array or to the element after the array (not before).

Possible solutions

  1. Make the top pointer to always point to the first empty stack slot (init to m_bottom). This may negatively affect unary op, because m_top - 1 must be used instead of m_top being used currently.
  2. Grow the stack in reverse order. m_top = m_bottom + max_stack_size (i.e. the "end"). Push is *m_top-- = value`. This will work, but pointer arithmetic will be confusing.
  3. Always add unused stack item (aka stack red zone). So m_bottom[0] is always allocated but should never be used. Init m_top = m_bottom. This works good, but requires more memory.
  4. Conditionally add unused stack item when number of locals is zero. The rest works like 3. Requires more memory only in rare case of not having any locals (including params). Requires additional branch in initialization, but can be marked with [[unlikely]].

3 and 4 have additional property that you can assign execution result value unconditionally because you can always safely access stack.top(), although in may contain garbage value.

@chfast chfast added the bug Something isn't working label Oct 6, 2020
@axic
Copy link
Member

axic commented Oct 6, 2020

I'd try to avoid (2) as that makes it worse for people writing host functions. I'd also try to avoid branching, so would vouch for (3).

@chfast
Copy link
Collaborator Author

chfast commented Oct 7, 2020

I'd also try to avoid branching

What is the rationale for that?

@axic
Copy link
Member

axic commented Oct 9, 2020

I'd also try to avoid branching

What is the rationale for that?

Wait a second, this is only done once in the constructor? If so, fine with branching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants