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

double free bug with arc, Result type, object refs, and iterators #13102

Closed
disruptek opened this issue Jan 10, 2020 · 2 comments
Closed

double free bug with arc, Result type, object refs, and iterators #13102

disruptek opened this issue Jan 10, 2020 · 2 comments
Assignees

Comments

@disruptek
Copy link
Contributor

@disruptek disruptek commented Jan 10, 2020

This is my current repro:

type
  D = ref object
  R = object
    case o: bool
    of true:
      d: D
    of false:
      discard

proc get(r: R): D =
  result = r.d

iterator things(): R =
  while true:
    yield R(o: true, d: D())

for n in things():
  when defined(crashme):
    discard n.get
  once:
    continue
  break

The crashme branch is on the left:

  //      yield R.ok D(x: i)
  					eqsink___Vxyc5ibyvJXAnSDCKVo0QQ((&n__T1vjwqUZuuW0aWoZZsYHsQ), T5_);
  //  discard n.unsafeGet.x
  					(void)((*n__T1vjwqUZuuW0aWoZZsYHsQ.v).x);
- //    discard n.get.x
- //      yield R.ok D(x: i)
- //    discard n.get.x
- //    discard n.get.x
- 					T7_ = (tyObject_DcolonObjectType___6TuBWP49bfEosJ8P1C7T4bw*)0;
- 					T7_ = get__dyNOqu4hmuyhepcGsIdpBwresults(n__T1vjwqUZuuW0aWoZZsYHsQ);
- //      yield R.ok D(x: i)
- 					eqsink___ZjaqRTgzdDygyGf9cxywHtA(&colontmpD__2, T7_);
- 					(void)((*colontmpD__2).x);
  //        inc(res)
  					res += ((NI) 1);
  				} LA4: ;
  			}
  		}
  	}
- //      yield R.ok D(x: i)
- //      yield R.ok D(x: i)
- 	eqdestroy___xadGWVQE8F8qfCUpBt9cTkg(&colontmpD__2);

So it seems like we're sinking the same ref twice, which makes me think that
maybe we're missing a move, right?

-d:traceArc is super helpful.

This could be a separate issue; probably not a problem if the first issue is fixed.

type
  D = ref object
  R = object
    case o: bool
    of true:
      d: D
    of false:
      discard

iterator things(): R =
  when defined(crashme):
    var
      x = D()
  while true:
    when not defined(crashme):
      var
        x = D()
    yield R(o: true, d: x)

for n in things():
  when defined(crashme):
    discard n.d
  once:
    continue
  break
@napalu

This comment has been minimized.

Copy link
Contributor

@napalu napalu commented Jan 14, 2020

@Araq Please reopen: I made a mistake when checking the linked issues - this issue is still not fixed by d568488. Example 1 using branch d568488 still SIGSEGVs.

Tested on

Nim Compiler Version 1.1.1 [Linux: amd64]
Compiled at 2020-01-14
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: 2a8eba99757cae211f04533cd949cb6dcabaada0
active boot switches: -d:release

Compiling with the tracing patch with flags with nim c -d:debug --gc:arc -d:crashme -d:traceArc yields:

[Allocated] 25 on 0x7f3004f5c048
[Allocated] 25 on 0x7f3004f5c078
[Freed] 25 on 0x7f3004f5c048
[WARN] attempt to double-free 0x7f3004f5c048
[WARN] gone 1
[WARN] allocated 0
[Freed] 25 on 0x7f3004f5c078

Example 2 also SIGSEGVs. Output with the tracing patch with flags nim c -d:debug --gc:arc -d:crashme -d:traceArc is:

[Allocated] 25 on 0x7f7bbd0a5048
[Freed] 25 on 0x7f7bbd0a5048
[WARN] attempt to double-free 0x7f7bbd0a5048
[WARN] gone 1
[WARN] allocated 0
@disruptek

This comment has been minimized.

Copy link
Contributor Author

@disruptek disruptek commented Jan 14, 2020

The sink decs the dest twice:

N_LIB_PRIVATE N_NIMCALL(void, eqsink___sP7qTDFBoQf6OcbX1LEeWQ)(tyObject_R__7OQxUuBKDK7hE0YBjDJ80Q* dest, tyObject_R__7OQxUuBKDK7hE0YBjDJ80Q src) {
	switch ((*dest).o) {
	case NIM_TRUE:
	{
//      yield R(o: true, field: unit)
		{
			NIM_BOOL T4_;
//      yield R(o: true, field: unit)
//      yield R(o: true, field: unit)
			T4_ = (NIM_BOOL)0;
			T4_ = nimDecRefIsLast((*dest).field);
			if (!T4_) goto LA5_;
//      yield R(o: true, field: unit)
//      yield R(o: true, field: unit)
			eqdestroy___9cW128TVJ3ZjSgGWuRjBEEg((*dest).field);
//      yield R(o: true, field: unit)
//      yield R(o: true, field: unit)
			nimRawDispose((*dest).field);
//      yield R(o: true, field: unit)
			(*dest).field = NIM_NIL;
		}
		LA5_: ;
	}
	break;
	case NIM_FALSE:
	{
	}
	break;
	default:
	{
	}
	break;
	}
//      yield R(o: true, field: unit)
	(*dest).o = src.o;
//      yield R(o: true, field: unit)
	switch ((*dest).o) {
	case NIM_TRUE:
	{
//      yield R(o: true, field: unit)
		{
			NIM_BOOL T12_;
//      yield R(o: true, field: unit)
//      yield R(o: true, field: unit)
			T12_ = (NIM_BOOL)0;
			T12_ = nimDecRefIsLast((*dest).field);
			if (!T12_) goto LA13_;
//      yield R(o: true, field: unit)
//      yield R(o: true, field: unit)
			eqdestroy___9cW128TVJ3ZjSgGWuRjBEEg((*dest).field);
//      yield R(o: true, field: unit)
//      yield R(o: true, field: unit)
			nimRawDispose((*dest).field);
		}
		LA13_: ;
//      yield R(o: true, field: unit)
		(*dest).field = src.field;
	}
	break;
	case NIM_FALSE:
	{
	}
	break;
	default:
	{
	}
	break;
	}
}
Araq added a commit that referenced this issue Jan 14, 2020
Araq added a commit that referenced this issue Jan 15, 2020
@Araq Araq closed this in a5e6707 Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.