FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Do not double free during callbacks. by mcollina · Pull Request #338 · nodegit/nodegit · GitHub

  1. nodegit
  2. nodegit

Repository navigation

Do not double free during callbacks. - #338

Merged
johnhaley81 merged 2 commits into
nodegit:masterfrom
mcollina:master
Jan 7, 2015
ConversationCommits (2)ChecksFiles changed
Merged

Do not double free during callbacks.#338
johnhaley81 merged 2 commits into
nodegit:masterfrom
mcollina:master

Conversation

mcollina commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator

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 commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator

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 commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator Author

Done

mcollina commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator Author

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 commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator

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

mcollina commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator Author

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 commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator Author

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

johnhaley81 commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator

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
Merge pull request #338 from mcollina/master
3b83ff3 
Do not double free during callbacks.
johnhaley81 merged commit 3b83ff3 into nodegit:master Jan 7, 2015

mcollina commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator Author

Thanks for maintaining this, it's so helpful!

johnhaley81 commented Jan 7, 2015

Copy link
Copy Markdown
Collaborator

np man! Just wish I had more time :)

mcollina commented Jan 12, 2015

Copy link
Copy Markdown
Collaborator Author

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

johnhaley81 commented Jan 12, 2015

Copy link
Copy Markdown
Collaborator

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 commented Jan 13, 2015

Copy link
Copy Markdown
Collaborator Author

Not really, we are pinned to 0.2.3 :).

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL