Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

UnhandledPromiseRejectionWarning caused by this module not catching validation errors #121

Closed
achingbrain opened this issue Feb 13, 2020 · 3 comments · Fixed by #122
Closed
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP status/in-progress In progress

Comments

@achingbrain
Copy link
Member

(node:19077) UnhandledPromiseRejectionWarning: Error: invalid ip
    at module.exports (/Users/alex/.nvm/versions/node/v10.17.0/lib/node_modules/ipfs/node_modules/libp2p-utils/src/ip-port-to-multiaddr.js:8:11)
    at module.exports (/Users/alex/.nvm/versions/node/v10.17.0/lib/node_modules/ipfs/node_modules/libp2p-tcp/src/socket-to-conn.js:55:39)
    at Server.net.createServer (/Users/alex/.nvm/versions/node/v10.17.0/lib/node_modules/ipfs/node_modules/libp2p-tcp/src/listener.js:23:20)
    at Server.emit (events.js:198:13)
    at TCP.onconnection (net.js:1520:8)

This can cause applications to crash.

@jacobheun jacobheun added kind/bug A bug in existing code (including security flaws) exp/novice Someone with a little familiarity can pick up P0 Critical: Tackled by core team ASAP status/ready Ready to be worked labels Feb 13, 2020
@jacobheun
Copy link
Contributor

My assumption is that we're getting a DNS address causing the invalid ip error. We need to wrap the calls to the utils function. Ideally we'd just use multiaddr.fromNodeAddress, but that doesn't support non ip addresses at the moment.

@vasco-santos
Copy link
Member

@jacobheun I am not sure if this is because of getting a DNS address, since this throws on parameter validation and not on ip validation.

The stack trace is:

So, this immediately failing on the parameter validation, which makes me think that ip-port-to-multiaddr is receiving undefined. Anyway, I will PR shortly a better error support for the util

@jacobheun jacobheun added status/in-progress In progress and removed status/ready Ready to be worked labels Feb 13, 2020
@jacobheun
Copy link
Contributor

I believe the issue is related to a client disconnecting before the listener converts the connection. From the nodejs docs for remoteAddress

Value may be undefined if the socket is destroyed (for example, if the client disconnected).

This is likely what we're seeing. I am adding a general catch in the listener for converting the socket to a MultiaddrConnection. Dial errors are caught in libp2p, but there is nothing to handle listener errors, so it can't throw.

jacobheun added a commit that referenced this issue Feb 13, 2020
When upgrading sockets to MultiaddConnections, it's possible for an error to be thrown.
This can crash the application if a client disconnects prior to the listener
uprading the socket, as is likely occurring in #121. Errors will now be caught and logged
when attempting to upgrade the socket.
jacobheun added a commit that referenced this issue Feb 14, 2020
When upgrading sockets to MultiaddConnections, it's possible for an error to be thrown.
This can crash the application if a client disconnects prior to the listener
uprading the socket, as is likely occurring in #121. Errors will now be caught and logged
when attempting to upgrade the socket.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P0 Critical: Tackled by core team ASAP status/in-progress In progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants