Skip to content

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 3, 2017

WriteWrap instances may contain extra storage space.
self_size() returns the size of the entire struct, member fields as
well as storage space, so it is not an accurate measure for the
storage space available.

Add a method ExtraSize() (like the existing Extra() for accessing
the storage memory) that yields the wanted value, and use it
in the HTTP2 impl to fix a crash.

Ref: #16669

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included (they crashe without this)
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@nodejs/http2 @jasnell Probably want to fast-track this :)

`WriteWrap` instances may contain extra storage space.
`self_size()` returns the size of the *entire* struct, member fields as
well as storage space, so it is not an accurate measure for the
storage space available.

Add a method `ExtraSize()` (like the existing `Extra()` for accessing
the storage memory) that yields the wanted value, and use it
in the HTTP2 impl to fix a crash.

Ref: nodejs#16669
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. labels Nov 3, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 3, 2017
@addaleax
Copy link
Member Author

addaleax commented Nov 3, 2017

@jasnell
Copy link
Member

jasnell commented Nov 3, 2017

Lgtm... Thanks for catching

Copy link
Contributor

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Should land soon to unbreak the CI

@jasnell
Copy link
Member

jasnell commented Nov 3, 2017

Looks like the fix is good, just waiting on CI to finish then will get it landed (CI is a bit bound up by a couple prior tests, taking longer than it should)

@jasnell
Copy link
Member

jasnell commented Nov 3, 2017

Ci is green, landing

jasnell pushed a commit that referenced this pull request Nov 3, 2017
`WriteWrap` instances may contain extra storage space.
`self_size()` returns the size of the *entire* struct, member fields as
well as storage space, so it is not an accurate measure for the
storage space available.

Add a method `ExtraSize()` (like the existing `Extra()` for accessing
the storage memory) that yields the wanted value, and use it
in the HTTP2 impl to fix a crash.

PR-URL: #16727
Refs: #16669
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 3, 2017

Landed in a5f3b3a.
thank you @addaleax :-D

@jasnell jasnell closed this Nov 3, 2017
@addaleax addaleax deleted the http2-fix branch November 3, 2017 22:10
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
`WriteWrap` instances may contain extra storage space.
`self_size()` returns the size of the *entire* struct, member fields as
well as storage space, so it is not an accurate measure for the
storage space available.

Add a method `ExtraSize()` (like the existing `Extra()` for accessing
the storage memory) that yields the wanted value, and use it
in the HTTP2 impl to fix a crash.

PR-URL: nodejs#16727
Refs: nodejs#16669
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
@gibfahn
Copy link
Member

gibfahn commented Nov 14, 2017

@addaleax conflicts for v8.x, mind raising a backport?

diff --cc src/node_http2_core-inl.h
index fa365fee05,c78e5d673f..0000000000
--- a/src/node_http2_core-inl.h
+++ b/src/node_http2_core-inl.h
@@@ -501,6 -501,11 +501,20 @@@ inline void Nghttp2Session::SendPending
  
    // While srcLength is greater than zero
    while ((srcLength = nghttp2_session_mem_send(session_, &src)) > 0) {
++<<<<<<< HEAD
++||||||| parent of a5f3b3a6da... src: add method to compute storage in WriteWrap
++    if (req == nullptr) {
++      req = AllocateSend();
++      destRemaining = req->self_size();
++      dest = req->Extra();
++    }
++=======
+     if (req == nullptr) {
+       req = AllocateSend();
+       destRemaining = req->ExtraSize();
+       dest = req->Extra();
+     }
++>>>>>>> a5f3b3a6da... src: add method to compute storage in WriteWrap
      DEBUG_HTTP2("Nghttp2Session %s: nghttp2 has %d bytes to send\n",
                  TypeName(), srcLength);
      size_t srcRemaining = srcLength;
@@@ -519,7 -524,9 +533,17 @@@
        destLength = 0;
        srcRemaining -= destRemaining;
        srcOffset += destRemaining;
++<<<<<<< HEAD
 +      destRemaining = dest.len;
++||||||| parent of a5f3b3a6da... src: add method to compute storage in WriteWrap
++      req = AllocateSend();
++      destRemaining = req->self_size();
++      dest = req->Extra();
++=======
+       req = AllocateSend();
+       destRemaining = req->ExtraSize();
+       dest = req->Extra();
++>>>>>>> a5f3b3a6da... src: add method to compute storage in WriteWrap
      }
  
      if (srcRemaining > 0) {

@addaleax
Copy link
Member Author

@gibfahn I think that might just be because #16669 hasn’t landed there yet, right?

MylesBorins pushed a commit that referenced this pull request Dec 7, 2017
`WriteWrap` instances may contain extra storage space.
`self_size()` returns the size of the *entire* struct, member fields as
well as storage space, so it is not an accurate measure for the
storage space available.

Add a method `ExtraSize()` (like the existing `Extra()` for accessing
the storage memory) that yields the wanted value, and use it
in the HTTP2 impl to fix a crash.

PR-URL: #16727
Refs: #16669
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 7, 2017
MylesBorins pushed a commit that referenced this pull request Dec 8, 2017
`WriteWrap` instances may contain extra storage space.
`self_size()` returns the size of the *entire* struct, member fields as
well as storage space, so it is not an accurate measure for the
storage space available.

Add a method `ExtraSize()` (like the existing `Extra()` for accessing
the storage memory) that yields the wanted value, and use it
in the HTTP2 impl to fix a crash.

PR-URL: #16727
Refs: #16669
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants