8000 Switch from strdup to memcpy to faithfully copy all bytes and prevent a crash in some cases. by tylerchurch · Pull Request #966 · nodegit/nodegit · GitHub
[go: up one dir, main page]
Skip to content

Switch from strdup to memcpy to faithfully copy all bytes and prevent a crash in some cases. #966

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
Mar 24, 2016
Merged

Switch from strdup to memcpy to faithfully copy all bytes and prevent a crash in some cases. #966

johnhaley81 merged 2 commits into from
Mar 24, 2016

Conversation

@tylerchurch
Copy link
Contributor
@tylerchurch tylerchurch commented Mar 24, 2016

This is a pull request to fix the issue I created over here: #965

To summarize over here why I'm making these changes:

  • strdup expects a C-style null-terminated string, but sometimes we're using nodejs strings to pass in strings that contain nulls in the middle, thus failing to copy everything at best, and causing crashes from reading invalid memory locations at worst.
  • Thus I changed to using malloc, memcpy and memset to prevent these issues.

To explain my changes a little bit:

  • I make the malloc'ed string one byte longer so I can add a terminating null character, since it looks like these templates are used in a number of different places, and for things like a repo path, where we do want things to end in a null character.
  • I used memset instead of the more standard array-style access because most of the usages of these templates are const char *.
  • The funky casts are because some places have void * instead of const char *, so to get the pointer arithmetic right, I had to specify the char *.

C code isn't what I normally write, so let me apologize in advance if these changes aren't quite what they should be 😄

I'm happy to make any modifications necessary.

The limited testing I've done seems to show that everything is working perfectly well with these changes.

@tylerchurch
Copy link
Contributor Author
tylerchurch commented Mar 24, 2016

Okay, not sure what I did wrong to fail the Travis CI build, this is the only error I see:

> nodegit@0.11.9 coveralls /home/travis/build/nodegit/nodegit
> cat ./test/coverage/merged.lcov | coveralls


/home/travis/build/nodegit/nodegit/node_modules/coveralls/bin/coveralls.js:18
        throw err;
              ^
Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}

Did I do that? Or is there some other error I'm missing? Or is there just some intermittent CI failure?

@johnhaley81
Copy link
Collaborator
johnhaley81 commented Mar 24, 2016

@tylerchurch it's an intermittent CI failure. I restarted the test. I was seeing the same on #958

@johnhaley81
Copy link
Collaborator
johnhaley81 commented Mar 24, 2016

Ok, that took me a moment to get my head around. Would you mind doing a summary of the comments above in the code? I know that later I'm going to look at that and go "WTF??"

@johnhaley81
Copy link
Collaborator
johnhaley81 commented Mar 24, 2016

Thanks @tylerchurch!

@johnhaley81 johnhaley81 merged commit 033e684 into nodegit:master Mar 24, 2016
@johnhaley81 johnhaley81 mentioned this pull request Mar 24, 2016
Closed
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.

3 participants

@tylerchurch @johnhaley81 @tylerc
0