Skip to content

Do not double free during callbacks. #338

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.

Sign up for GitHub

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

Jump to bottom
Merged
johnhaley81 merged 2 commits into from
Jan 7, 2015
Merged

Do not double free during callbacks. #338

johnhaley81 merged 2 commits into from
Jan 7, 2015

Conversation

@mcollina
Copy link
Collaborator

@mcollina mcollina commented Jan 7, 2015

Continuing from #331, as it is a different issue.

Here is the backtrace obtained via lldb:

* thread #1: tid = 0xdd2c0, 0x00007fff973f219f libsystem_malloc.dylib`malloc_error_break, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00007fff973f219f libsystem_malloc.dylib`malloc_error_break
    frame #1: 0x00007fff973e38c5 libsystem_malloc.dylib`free + 314
    frame #2: 0x000000010327f77b nodegit.node`GitCommit::LookupWorker::HandleOKCallback(this=0x0000000100d09790) + 859 at commit.cc:689
    frame #3: 0x000000010324d812 nodegit.node`NanAsyncWorker::WorkComplete(this=0x0000000100d09790) + 66 at nan.h:1881
    frame #4: 0x000000010324e2d1 nodegit.node`NanAsyncExecuteComplete(req=0x0000000100d09798) + 33 at nan.h:1948
    frame #5: 0x00000001004598da node`uv__work_done + 168
    frame #6: 0x0000000100450717 node`uv__async_event + 62
    frame #7: 0x000000010045088a node`uv__async_io + 136
    frame #8: 0x000000010045d256 node`uv__io_poll + 1463
    frame #9: 0x0000000100450cd1 node`uv_run + 239
    frame #10: 0x0000000100409e6a node`node::Start(int, char**) + 365
    frame #11: 0x0000000100000b74 node`start + 52

Here is the relevant part of commit.cc in which the error happen:

void GitCommit::LookupWorker::HandleOKCallback() {
  TryCatch try_catch;

  if (baton->error_code == GIT_OK) {
    Handle<Value> to;
// start convert_to_v8 block

  if (baton->commit != NULL) {
    // GitCommit baton->commit
       to = GitCommit::New((void *)baton->commit, false);
   }
  else {
    to = NanNull();
  }

 // end convert_to_v8 block
    Handle<Value> result = to;
    Handle<Value> argv[2] = {
      NanNull(),
      result
    };
    callback->Call(2, argv);
  } else {
    if (baton->error) {
      Handle<Value> argv[1] = {
        NanError(baton->error->message)
      };
      callback->Call(1, argv);
      if (baton->error->message)
        free((void *)baton->error->message);
      free((void *)baton->error);
    } else {
      callback->Call(0, NULL);
    }

    free((void*)baton->id); // here is the first free
  }

  if (try_catch.HasCaught()) {
    node::FatalException(try_catch);
  }

  if (baton->idNeedsFree) {
    free((void *)baton->id); // here happens the problem, the second free
  }

  delete baton;
}

Adding this line before the first free fixes it.

baton->idNeedsFree = false;

I did that in the templates. Let me know if it is ok, or if I should change something.

I can even add a unit test, but I might need some pointers in how catch the issue in the smallest case.

@johnhaley81
Copy link
Collaborator

johnhaley81 commented Jan 7, 2015

For completeness sake, I think we should wrap that free in an if block to check if it needs to be freed in the first place and probably even set the NeedsFree in the second block as well.

@mcollina
Copy link
Collaborator Author

mcollina commented Jan 7, 2015

Done

@mcollina
Copy link
Collaborator Author

mcollina commented Jan 7, 2015

I am getting some test failures locally, but on travis is fine.

Here are the relevant tests:


  1) Clone can clone with ssh:
     Error: timeout of 15000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  2) Clone can clone with ssh while manually loading a key:
     Error: timeout of 15000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  3) Clone can clone with git:
     Error: timeout of 15000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  4) Remote can fetch from a remote:
     Error: timeout of 2000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  5) Remote can fetch from all remotes:
     Error: timeout of 15000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  6) Remote "after all" hook:
     Error: timeout of 2000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  7) Repository "before all" hook:
     Error: timeout of 2000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  8) Revwalk "before all" hook:
     Error: timeout of 60000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  9) Tag "before all" hook:
     Error: timeout of 2000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

  10) TreeEntry "before each" hook:
     Error: timeout of 2000ms exceeded
      at null.<anonymous> (/Users/matteocollina/.nvm/v0.10.33/lib/node_modules/mocha/lib/runnable.js:158:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

@johnhaley81
Copy link
Collaborator

johnhaley81 commented Jan 7, 2015

Do you get those consistently? If they are timeouts then there might be something else going on. The PR looks good though.

@mcollina
Copy link
Collaborator Author

mcollina commented Jan 7, 2015

Now I can't run them at all. Anyway, if it looks good, then go ahead :). I can confirm that it works for me and solves the issue.

@mcollina
Copy link
Collaborator Author

mcollina commented Jan 7, 2015

Ok, they started, but the timeouts are coming out consistently.

@johnhaley81
Copy link
Collaborator

johnhaley81 commented Jan 7, 2015

They all pass locally for me so I think we're good.

Nice job man!

johnhaley81 added a commit that referenced this pull request Jan 7, 2015
@johnhaley81
Merge pull request #338 from mcollina/master
3b83ff3 
Do not double free during callbacks.
@johnhaley81 johnhaley81 merged commit 3b83ff3 into nodegit:master Jan 7, 2015
@mcollina
Copy link
Collaborator Author

mcollina commented Jan 7, 2015

Thanks for maintaining this, it's so helpful!

@johnhaley81
Copy link
Collaborator

johnhaley81 commented Jan 7, 2015

np man! Just wish I had more time :)

@mcollina
Copy link
Collaborator Author

mcollina commented Jan 12, 2015

Can we get this released? Any other PRs you want in before release that I can help you with?

@johnhaley81
Copy link
Collaborator

johnhaley81 commented Jan 12, 2015

I was hoping to get in #336 before we version bumped but that PR can't be closed until libgit2 goes to 0.21.4. We could do multiple ones though if this is holding you back.

@mcollina
Copy link
Collaborator Author

mcollina commented Jan 13, 2015

Not really, we are pinned to 0.2.3 :).

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mcollina @johnhaley81