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

Undefined behaviour in the usage of incrSeqV3 #10568

Closed
zielmicha opened this issue Feb 5, 2019 · 1 comment

Comments

Projects
None yet
2 participants
@zielmicha
Copy link
Contributor

commented Feb 5, 2019

Code like this:

proc xxx() =
  var a: seq[int]
  a.add 5

xxx()

generates the following C code (both on latest devel and on 0.18.1):


N_LIB_PRIVATE N_NIMCALL(void, xxx_yIubWxUz8g5WVxMecH6HtQ)(void) {
	tySequence_qwqHTkRvwhrRyENtudHQ7g* a;
	NI T1_;
	nimfr_("xxx", "a.nim");
	a = (tySequence_qwqHTkRvwhrRyENtudHQ7g*)0;
	nimln_(4, "a.nim");
	a = (tySequence_qwqHTkRvwhrRyENtudHQ7g*) incrSeqV3(&(a)->Sup, (&NTI_qwqHTkRvwhrRyENtudHQ7g_));
	T1_ = a->Sup.len++;
	a->data[T1_] = ((NI) 5);
	popFrame();
}

Note that we dereference a null pointer in (a)->Sup. This is an undefined behaviour and it's actually possible (but not trivial) to get Clang undefined behaviour sanitizer to report it. I fear this might produce miscompiled programs now or in the future (and it actually makes it harder to use Clang sanitizers).

Suggested fix would be to change &(a)->Sup to (PGenericSeq*)a. That would probably not violate aliasing rules (https://stackoverflow.com/a/50383349).

@Araq

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

I don't know if we dereference, looks like an address computation to me. But your suggestion makes sense.

@LemonBoy LemonBoy closed this in 580f622 Feb 6, 2019

LemonBoy added a commit that referenced this issue Feb 6, 2019

Merge pull request #10577 from zielmicha/fix-10568
Fixes #10568: Fix null pointer dereference in address computation for incrSeqV3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.