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

Segfault using socket.fdopen() + connect #6

Closed
daurnimator opened this issue Oct 20, 2014 · 7 comments
Closed

Segfault using socket.fdopen() + connect #6

daurnimator opened this issue Oct 20, 2014 · 7 comments

Comments

@daurnimator
Copy link
Collaborator

On my computer, the following program consistently segfaults:

local socket = require "socket" -- luasocket

local cqueues = require "cqueues"
local cqueues_socket = require "cqueues.socket"


-- Lets use luasocket to get a raw file descriptor
local rawfd do
    local s = socket.tcp()
    s:settimeout(0)
    s:connect("127.0.0.1", 80)

    rawfd = s:getfd()
    s:setfd(-1) -- Invalidate luasocket object; so luasocket doesn't close/clean it up
end
print("Got raw (unconnected) file descriptor: ", rawfd)

local cq = cqueues.new()
cq:wrap(function()
    local sock = cqueues_socket.fdopen(rawfd)
    -- Lets wait for the socket to connect with cqueues
    assert(sock:connect()) -- SEGFAULT!
end)
assert(cq:loop())
@wahern
Copy link
Owner

wahern commented Oct 20, 2014

Using undocumented APIs? ;) It would have crashed when calling :connect on a socket pair, too, though.

Calling :connect doesn't make sense in either case because the object doesn't know what the host address is. The patch I committed will return an error when :connect is called and there's no host information available.

By the way, socket.fdopen duplicates the descriptor, so it doesn't need to be explicitly discarded. The above code leaks rawfd. I'll accept a patch that adds an option to tell .fdopen to take ownership of the existing descriptor number. But because stale descriptors are perhaps the biggest source of bugs in asynchronous networking software, I prefer that the default behavior is to duplicate the descriptor.

@daurnimator
Copy link
Collaborator Author

Calling :connect doesn't make sense in either case because the object doesn't know what the host address is.

Doesn't it? IIRC, at the syscall level, you're meant to call connect() until it doesn't return EINPROGRESS
How can I do this within the confines of cqueues?

patch I committed will return an error when :connect is called and there's no host information available

Can I tell it somehow? Or could it just do without it?

By the way, socket.fdopen duplicates the descriptor, so it doesn't need to be explicitly discarded. The above code leaks rawfd.

Yeah I thought so; but when filing a bug it's better to never clean up at all than to clean up too early :)

@daurnimator
Copy link
Collaborator Author

Btw, with the fix from bb0181e I just see socket:connect: Unknown error -1935895349.
At least it's not a segfault, but an error message would be nice :)

@wahern
Copy link
Owner

wahern commented Oct 20, 2014

connect(2) requires passing the connect address, which is unknown when using socket.fdopen. There's no way at the moment to wrap an existing socket descriptor and then call :connect. The way .fdopen is meant to work, the socket should already be in the connected state. If it's already in the connected state there's no need to call :connect--:read and :write should just work.

What's your use case? You might be able to fake waiting for the connection to finish by doing cqueues.poll{ .pollfd = sock:pollfd(), .events = "w" }. The .fdopen API could be changed, or I could test out one of several ugly hacks that come to mind, but it seems like a really weird case: if you know the address, just call socket.connect("127.0.0.1", 80); if you already have a descriptor where connect has already been called, then why can't the connect be finished?

(Maybe originally you just wanted to wrap a descriptor and then have cqueues start and finish the connect?)

FWIW, I recently added a feature which short-circuited some of the DNS resolver code when an IP address was specified. Previously a connect would fail for "127.0.0.1" if, e.g., /etc/resolv.conf was missing. Now address literals should work even when a host name couldn't be resolved.

The "Unknown error" is a bug in my makefile. It's not rebuilding errno.o when lib/socket.h changes. Try make clean. I'll commit a fix to the makefile.

@daurnimator
Copy link
Collaborator Author

connect(2) requires passing the connect address, which is unknown when using socket.fdopen
I have them, just tell me where to put them :)

What's your use case?

I'm trying to rewrite the wrapclient from prosody to use cqueues. It takes a luasocket object that has had connect() called on it once.
Sadly, I cannot change the API, and am stuck with trying to work with this half-connected lua socket object.

You might be able to fake waiting for the connection to finish by doing cqueues.poll{ .pollfd = sock:pollfd(), .events = "w" }

That's what I've been doing in the mean time :) If it's correct, maybe socket:connect could do that when it doesn't know the host/port?

The "Unknown error" is a bug in my makefile. It's not rebuilding errno.o when lib/socket.h changes. Try make clean. I'll commit a fix to the makefile.

Ah ha! thanks.

@wahern wahern closed this as completed Oct 21, 2014
@daurnimator
Copy link
Collaborator Author

wahern closed this 2 minutes ago

Is that just marking the segfault as fixed?
or also implying calling socket:connect when cqueues didn't create the socket itself is a "WONTFIX"?

@wahern
Copy link
Owner

wahern commented Oct 22, 2014

I was just marking the segfault as fixed.

Making socket:connect work after .fdopen would be much more involved. If you really need it please open a new ticket.

I'm not sure, yet, how to address it. The pattern doesn't fit well with the low-level socket library abstraction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants