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

http_parser: do not dealloc during kOnExecute #2956

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Sep 18, 2015

freeParser deallocates Parser instances early if they do not fit
into the free list. This does not play well with recent socket
consumption change, because it will try to deallocate the parser while
executing on its stack.

Fix: #2928

cc @nodejs/collaborators @trevnorris @bnoordhuis

@indutny indutny added http Issues or PRs related to the http subsystem. land-on-v4.x labels Sep 18, 2015
@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

Test is a bit complicated, because the crash happens only on the access to the uninitialized memory.

@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

@bnoordhuis
Copy link
Member

What commit or commits introduced the regression?

class ScopedRetainParser {
public:
ScopedRetainParser(Parser* p) : p_(p) {
p_->refcount_++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider doing CHECK(p_->refcount_ > 0) first in this function.

@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

The regression happened at: 1bc4468

@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

@bnoordhuis all fixed.

@bnoordhuis
Copy link
Member

Test is a bit complicated, because the crash happens only on the access to the uninitialized memory.

Create 1,000 or 10,000 parser objects and close them from within .execute(). One of them is bound to trigger the crash.

The regression happened at: 1bc4468

Can you include that in the commit log? Maybe also reference the first line of its commit log, that saves the log spelunker an extra git show.

@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

@bnoordhuis included. Will work on the test, but it won't be reliable unless started under valgrind.

@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

Thank you!

@@ -668,7 +689,10 @@ class Parser : public BaseObject {
char* current_buffer_data_;
StreamResource::Callback<StreamResource::AllocCb> prev_alloc_cb_;
StreamResource::Callback<StreamResource::ReadCb> prev_read_cb_;
int refcount_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, you can initialize it to 1 here.

@trevnorris
Copy link
Contributor

@indutny this is a bit nitty on my part, but mind putting the regression sha in this format: 3b78b85

Find it's easier to parse when looking through history. No worries if you don't though. :)

@trevnorris
Copy link
Contributor

Outside of including a test, don't see anything I would change. LGTM.

@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

Force pushed, PTAL

@indutny
Copy link
Member Author

indutny commented Sep 18, 2015

@@ -504,6 +506,22 @@ class Parser : public BaseObject {
}

protected:
class ScopedRetainParser {
public:
explicit ScopedRetainParser(Parser* p) : p_(p) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to have a const here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that delete may be called on const pointer... Am I wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny I just tried this

# include <cstdio>

class Test {
  public:
    Test(const char * str) : my_str(str) {
    }

    ~Test() {
      fprintf(stderr, "Cleaning up");
      delete my_str;
    }

  private:
    const char * my_str;
};

int main() {
  Test test(new char[50]);
}

with

➜  Desktop  g++ Test.cpp && ./a.out
Cleaning up
➜  Desktop

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Wall?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Hmmm, I tried that now. No warnings.

➜  Desktop  g++ -Wall Test.cpp && ./a.out
Cleaning up
➜  Desktop  

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think you're right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I'm changing the refcount of the parser, so it won't work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Ah, right. Sorry for the fuss.

`freeParser` deallocates `Parser` instances early if they do not fit
into the free list. This does not play well with recent socket
consumption change, because it will try to deallocate the parser while
executing on its stack.

Regression was introduced in: 1bc4468

Fix: nodejs#2928
@indutny
Copy link
Member Author

indutny commented Sep 19, 2015

CI is green. Shall I land it?

@trevnorris
Copy link
Contributor

LGTM

@indutny
Copy link
Member Author

indutny commented Sep 19, 2015

Landed in 229a03f, thank you!

@indutny indutny closed this Sep 19, 2015
indutny added a commit that referenced this pull request Sep 19, 2015
`freeParser` deallocates `Parser` instances early if they do not fit
into the free list. This does not play well with recent socket
consumption change, because it will try to deallocate the parser while
executing on its stack.

Regression was introduced in: 1bc4468

Fix: #2928
PR-URL: #2956
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@indutny indutny deleted the fix/gh-2928 branch September 19, 2015 07:33
@stephank
Copy link

Awesome! Thanks guys! 💪

indutny added a commit that referenced this pull request Sep 20, 2015
`freeParser` deallocates `Parser` instances early if they do not fit
into the free list. This does not play well with recent socket
consumption change, because it will try to deallocate the parser while
executing on its stack.

Regression was introduced in: 1bc4468

Fix: #2928
PR-URL: #2956
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@rvagg rvagg mentioned this pull request Sep 22, 2015
@MylesBorins
Copy link
Contributor

landed in lts-v4.x-staging as bc9f629

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants