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.

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

Sockets.get(peer|sock)name on closed TCPSocket segfaults #40993

Closed
fredrikekre opened this issue May 28, 2021 · 3 comments
Closed

Sockets.get(peer|sock)name on closed TCPSocket segfaults #40993

fredrikekre opened this issue May 28, 2021 · 3 comments
Labels
good first issue Indicates a good issue for first-time contributors to Julia

Comments

@fredrikekre
Copy link
Member

Could probably be an error instead?

sock.handle is a null pointer here

julia/stdlib/Sockets/src/Sockets.jl

Line 817 in 5dbf45a

sock.handle, rport, raddress, rfamily)
and here

julia/stdlib/Sockets/src/Sockets.jl

Line 821 in 5dbf45a

sock.handle, rport, raddress, rfamily)

Would peer = isopen(sock) ? getpeername(sock) : nothing be a safe alternative to avoid this segfault (or error if this issue is fixed)?
@vtjnash
Copy link
Member

vtjnash commented May 28, 2021

Normally we'd call check_open(sock) somewhere near the top of the function, and let that throw an exception if the state is invalid for access

@fredrikekre fredrikekre added the good first issue Indicates a good issue for first-time contributors to Julia label May 28, 2021
@cljord
Copy link
Contributor

cljord commented May 29, 2021

Hi @vtjnash @fredrikekre

I'm pretty new to contributing to open-source and Julia, but it sounds like the solution is really just adding the check_open(sock) check to the top of the function (I took a look at check_open and it throws an error itself). If I understood that correctly, I'd like to take this issue.

cljord added a commit to cljord/julia that referenced this issue May 29, 2021
@cljord
Added check_open(sock) to _sockname, which is used in getpeername. No…
161b11a 
…t checking could lead to a segfault on closed TCPSockets, see JuliaLang#40993
@cljord cljord mentioned this issue May 29, 2021
Merged
fredrikekre added a commit to JuliaWeb/HTTP.jl that referenced this issue May 29, 2021
@fredrikekre
Store remote IP adress in connection struct
67efd91 
The remote IP is used in access logging but if the connection is closed
at the time of creating the log message it is not possible to fetch it.
(In fact, it currently segfaults, but will error in future Julia versions,
see JuliaLang/julia#40993). This should not cost anything since the call
to Sockets.getpeername(...) is already there for storing the remote port.
fredrikekre added a commit to JuliaWeb/HTTP.jl that referenced this issue May 29, 2021
@fredrikekre
Store remote IP adress in connection struct
572c903 
The remote IP is used in access logging but if the connection is closed
at the time of creating the log message it is not possible to fetch it.
(In fact, it currently segfaults, but will error in future Julia versions,
see JuliaLang/julia#40993). This should not cost anything since the call
to Sockets.getpeername(...) is already there for storing the remote port.
@fredrikekre fredrikekre mentioned this issue May 29, 2021
Merged
fredrikekre added a commit to JuliaWeb/HTTP.jl that referenced this issue May 29, 2021
@fredrikekre
Store remote IP adress in connection struct
9a4a44f 
The remote IP is used in access logging but if the connection is closed
at the time of creating the log message it is not possible to fetch it.
(In fact, it currently segfaults, but will error in future Julia versions,
see JuliaLang/julia#40993). This should not cost anything since the call
to Sockets.getpeername(...) is already there for storing the remote port.
fredrikekre added a commit to JuliaWeb/HTTP.jl that referenced this issue May 29, 2021
@fredrikekre
Store remote IP adress in connection struct
99ada16 
The remote IP is used in access logging but if the connection is closed
at the time of creating the log message it is not possible to fetch it.
(In fact, it currently segfaults, but will error in future Julia versions,
see JuliaLang/julia#40993). This should not cost anything since the call
to Sockets.getpeername(...) is already there for storing the remote port.
fredrikekre added a commit to JuliaWeb/HTTP.jl that referenced this issue May 30, 2021
@fredrikekre
Store remote IP adress in connection struct
bf020ab 
The remote IP is used in access logging but if the connection is closed
at the time of creating the log message it is not possible to fetch it.
(In fact, it currently segfaults, but will error in future Julia versions,
see JuliaLang/julia#40993). With this patch the remote IP is stored in
the Connection struct. This should not cost anything extras since the call
to Sockets.getpeername(...) is already there for storing the remote port.
fredrikekre added a commit to JuliaWeb/HTTP.jl that referenced this issue May 31, 2021
@fredrikekre
Store remote IP adress in connection struct (#718)
7e4420d 
The remote IP is used in access logging but if the connection is closed
at the time of creating the log message it is not possible to fetch it.
(In fact, it currently segfaults, but will error in future Julia versions,
see JuliaLang/julia#40993). With this patch the remote IP is stored in
the Connection struct. This should not cost anything extras since the call
to Sockets.getpeername(...) is already there for storing the remote port.
@cljord
Copy link
Contributor

cljord commented May 31, 2021

Hi again,
so I added the check_open(sock) to the beginning of the function, but now the tests fail. I've poked around a little and the buildbot always has a:

nested task error: ArgumentError: TCPSocket(RawFD(19) init, 0 bytes waiting) is not initialized

from runtests.jl, starting at line 93.

Sorry for probably adding more work than I solved, but I'm in a bit over my head with this large a codebase and I can't figure it out. My pull request is #41000 for a closer look at the buildbot.

shirodkara pushed a commit to shirodkara/julia that referenced this issue Jun 9, 2021
@cljord
Add check_open(sock) to _sockname (JuliaLang#41000)
df16c4c 
Not checking could lead to a segfault on closed TCPSockets in getpeername

Fixes JuliaLang#40993
johanmon pushed a commit to johanmon/julia that referenced this issue Jul 5, 2021
@cljord @johanmon
Add check_open(sock) to _sockname (JuliaLang#41000)
889ce9b 
Not checking could lead to a segfault on closed TCPSockets in getpeername

Fixes JuliaLang#40993
@fredrikekre fredrikekre mentioned this issue Dec 7, 2021
Closed
@ghost ghost mentioned this issue Dec 8, 2021
Closed
KristofferC pushed a commit that referenced this issue Jan 10, 2022
@cljord @KristofferC
Add check_open(sock) to _sockname (#41000)
a3bfdbe 
Not checking could lead to a segfault on closed TCPSockets in getpeername

Fixes #40993

(cherry picked from commit 28e30a3)
staticfloat pushed a commit that referenced this issue Dec 23, 2022
@cljord @staticfloat
Add check_open(sock) to _sockname (#41000)
a2471c1 
Not checking could lead to a segfault on closed TCPSockets in getpeername

Fixes #40993

(cherry picked from commit 28e30a3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Indicates a good issue for first-time contributors to Julia
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@vtjnash @fredrikekre @cljord