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_client: ensure empty socket on error #1103

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 9, 2015

Read all pending data out of the socket on error event and ensure that
no data/end handlers will be invoked on socket.destroy().
Otherwise following assertion happens:

AssertionError: null == true
    at TLSSocket.socketOnData (_http_client.js:308:3)
    at TLSSocket.emit (events.js:107:17)
    at TLSSocket.Readable.read (_stream_readable.js:373:10)
    at TLSSocket.socketCloseListener (_http_client.js:229:10)
    at TLSSocket.emit (events.js:129:20)
    at TCP.close (net.js:476:12)

Fix: nodejs/node-v0.x-archive#9348
See: npm/npm#7349

cc @iojs/collaborators @bnoordhuis @othiym23

Read all pending data out of the socket on `error` event and ensure that
no `data`/`end` handlers will be invoked on `socket.destroy()`.
Otherwise following assertion happens:

    AssertionError: null == true
        at TLSSocket.socketOnData (_http_client.js:308:3)
        at TLSSocket.emit (events.js:107:17)
        at TLSSocket.Readable.read (_stream_readable.js:373:10)
        at TLSSocket.socketCloseListener (_http_client.js:229:10)
        at TLSSocket.emit (events.js:129:20)
        at TCP.close (net.js:476:12)

Fix: nodejs/node-v0.x-archive#9348
@rvagg
Copy link
Member

rvagg commented Mar 9, 2015

:shipit: I'll hold up 1.5.1 for this

@rvagg
Copy link
Member

rvagg commented Mar 9, 2015

oh, that was a lgtm from me

@rvagg
Copy link
Member

rvagg commented Mar 9, 2015

@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@rvagg thanks! This seems to be pretty sensitive piece of code, would anyone from @iojs/collaborators like to take a look at it too and give it additional LGTY?

@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@rvagg erm, language is my enemy :) I didn't meant to offend you in any way, just would like to have two LGTYs on this.

@rvagg
Copy link
Member

rvagg commented Mar 9, 2015

heh, no offense taken at all! I'm keen for more eyes too, mine aren't tuned to this section of code

@micnic
Copy link
Contributor

micnic commented Mar 9, 2015

LGTM! the only thing I would like to mention is to leave the declaration of the parser variable at the line 250.

@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@micnic : why? It might disappear after socket.read()

indutny added a commit that referenced this pull request Mar 9, 2015
Read all pending data out of the socket on `error` event and ensure that
no `data`/`end` handlers will be invoked on `socket.destroy()`.
Otherwise following assertion happens:

    AssertionError: null == true
        at TLSSocket.socketOnData (_http_client.js:308:3)
        at TLSSocket.emit (events.js:107:17)
        at TLSSocket.Readable.read (_stream_readable.js:373:10)
        at TLSSocket.socketCloseListener (_http_client.js:229:10)
        at TLSSocket.emit (events.js:129:20)
        at TCP.close (net.js:476:12)

Fix: nodejs/node-v0.x-archive#9348
PR-URL: #1103
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

Landed in 1a3ca82, thank you!

@indutny indutny closed this Mar 9, 2015
@indutny indutny deleted the fix/gh-node-9348 branch March 9, 2015 13:27
@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@rvagg : guess we are good to go for 1.5.1 now. There is a remaining TLS leak that I'll investigate soon, but do not let it block the release for a significant time.

@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@rvagg : I mean - we already fixed most of it, only minor problem is left.

@micnic
Copy link
Contributor

micnic commented Mar 9, 2015

@indutny, I mentioned that for the sake of "variable declaration first in function body" code style. Could you show me, please, where exactly may the parser change in socket.read()? I didn't know about this, thought it is something more like a trivial stream.read() implementation.

@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@micnic this is not about stream.read() itself, it might invoke socketOnData or socketOnEnd which might call freeParser. In such case socket.parser will be null after stream.read() and the code below it will double-free the parser.

@micnic
Copy link
Contributor

micnic commented Mar 9, 2015

Thanks, I was a bit confused, I'll investigate more to understand better how the things work under the hood.

@rvagg
Copy link
Member

rvagg commented Mar 9, 2015

is this an 0.12 & io.js thing only or does it also go to 0.10?

rvagg added a commit that referenced this pull request Mar 9, 2015
Notable changes:

* tls: The reported TLS memory leak has been at least partially
  resolved via various commits in this release. Current testing indicated
  that there may still be some leak problems. Progress being tracked at:
  #1075
* http: Fixed an error reported at
  nodejs/node-v0.x-archive#9348 and
  npm/npm#7349
  Pending data was not being fully read upon an 'error' event leading to
  an assertion failure on socket.destroy().
  (Fedor Indutny) #1103
@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@rvagg 0.10 doesn't read from the socket on close event. This change ( f153d6d ) wasn't backported there.

jomo added a commit to crafatar/crafatar that referenced this pull request Mar 14, 2015
misterdjules pushed a commit to misterdjules/node that referenced this pull request Mar 18, 2015
Read all pending data out of the socket on `error` event and ensure that
no `data`/`end` handlers will be invoked on `socket.destroy()`.
Otherwise following assertion happens:

    AssertionError: null == true
        at TLSSocket.socketOnData (_http_client.js:308:3)
        at TLSSocket.emit (events.js:107:17)
        at TLSSocket.Readable.read (_stream_readable.js:373:10)
        at TLSSocket.socketCloseListener (_http_client.js:229:10)
        at TLSSocket.emit (events.js:129:20)
        at TCP.close (net.js:476:12)

Fix: nodejs#9348
PR-URL: nodejs/node#1103
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Nicu Micleușanu <[email protected]>
misterdjules pushed a commit to misterdjules/node that referenced this pull request Mar 25, 2015
This change is a backport of 1a3ca82
from io.js.

Original commit message:

  Read all pending data out of the socket on `error` event and ensure that
  no `data`/`end` handlers will be invoked on `socket.destroy()`.
  Otherwise following assertion happens:

      AssertionError: null == true
          at TLSSocket.socketOnData (_http_client.js:308:3)
          at TLSSocket.emit (events.js:107:17)
          at TLSSocket.Readable.read (_stream_readable.js:373:10)
          at TLSSocket.socketCloseListener (_http_client.js:229:10)
          at TLSSocket.emit (events.js:129:20)
          at TCP.close (net.js:476:12)

  Fix: nodejs#9348
  PR-URL: nodejs/node#1103
  Reviewed-By: Rod Vagg <[email protected]>
  Reviewed-By: Nicu Micleușanu <[email protected]>

Fixes nodejs#9348.
misterdjules pushed a commit to misterdjules/node that referenced this pull request Mar 25, 2015
This change is a backport of 1a3ca82
from io.js.

Original commit message:

  Read all pending data out of the socket on `error` event and ensure that
  no `data`/`end` handlers will be invoked on `socket.destroy()`.
  Otherwise following assertion happens:

      AssertionError: null == true
          at TLSSocket.socketOnData (_http_client.js:308:3)
          at TLSSocket.emit (events.js:107:17)
          at TLSSocket.Readable.read (_stream_readable.js:373:10)
          at TLSSocket.socketCloseListener (_http_client.js:229:10)
          at TLSSocket.emit (events.js:129:20)
          at TCP.close (net.js:476:12)

  Fix: nodejs#9348
  PR-URL: nodejs/node#1103
  Reviewed-By: Rod Vagg <[email protected]>
  Reviewed-By: Nicu Micleușanu <[email protected]>

Fixes nodejs#9348.
indutny added a commit to nodejs/node-v0.x-archive that referenced this pull request Mar 28, 2015
This change is a backport of 1a3ca82
from io.js.

Original commit message:

  Read all pending data out of the socket on `error` event and ensure that
  no `data`/`end` handlers will be invoked on `socket.destroy()`.
  Otherwise following assertion happens:

      AssertionError: null == true
          at TLSSocket.socketOnData (_http_client.js:308:3)
          at TLSSocket.emit (events.js:107:17)
          at TLSSocket.Readable.read (_stream_readable.js:373:10)
          at TLSSocket.socketCloseListener (_http_client.js:229:10)
          at TLSSocket.emit (events.js:129:20)
          at TCP.close (net.js:476:12)

  Fix: #9348
  PR-URL: nodejs/node#1103
  Reviewed-By: Rod Vagg <[email protected]>
  Reviewed-By: Nicu Micleușanu <[email protected]>

Fixes #9348.

Reviewed-By: Julien Gilli <[email protected]>
PR-URL: #14087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants