unix_socket: add support for abstract unix domain socket #1197

iboukris · 2017-01-08T23:28:43Z

In addition to unix domain sockets, Linux also supports an
abstract namespace which is independent of the filesystem.

In order to support it, add new CURLOPT_ABSTRACT_UNIX_SOCKET
option which uses the same storage as CURLOPT_UNIX_SOCKET_PATH
internally, along with a flag to specify abstract socket.

On non-supporting platforms, the abstract address will be
interpreted as an empty string and fail gracefully.

Also add new --abstract-unix-socket tool parameter.

Reported-by: Chungtsun Li (typeless)
Signed-off-by: Isaac Boukris iboukris@gmail.com

Fixes #1061

mention-bot · 2017-01-08T23:28:49Z

@Frenche, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @Lekensteyn and @yangtse to be potential reviewers.

iboukris · 2017-01-08T23:31:18Z

Note, the man pages are missing, I'll work on it asap.
Just thought maybe someone could review meanwhile, thanks.

jay · 2017-01-09T02:42:18Z

src/tool_operate.c

-                        config->unix_socket_path);
+          my_setopt_str(curl, config->abstract_unix_socket ?
+                        CURLOPT_ABSTRACT_UNIX_SOCKET :
+                        CURLOPT_UNIX_SOCKET_PATH, config->unix_socket_path);


fix for style, use column alignment. suggest parentheses and braces to make it easier to understand

if(config->unix_socket_path) { my_setopt_str(curl, (config->abstract_unix_socket ? CURLOPT_ABSTRACT_UNIX_SOCKET : CURLOPT_UNIX_SOCKET_PATH), config->unix_socket_path); }

Also it occurs to me that CURLoption parameter is stringized by the function-like macro so I'm pretty sure this is not going to work, run with --libcurl to confirm. Assuming that's the case do this instead

if(config->unix_socket_path) { if(config->abstract_unix_socket) { my_setopt_str(curl, CURLOPT_ABSTRACT_UNIX_SOCKET, config->unix_socket_path); } else { my_setopt_str(curl, CURLOPT_UNIX_SOCKET_PATH, config->unix_socket_path); } }

Thanks, I'll fix this.

jay · 2017-01-09T02:54:19Z

lib/curl_addrinfo.c

  /* sun_path must be able to store the NUL-terminated path */
-  path_len = strlen(path);
-  if(path_len >= sizeof(sa_un->sun_path)) {
+  path_len = strlen(path) +1;


I would change all the +1 to + 1 and the -1 to - 1. I don't think we have a policy on it but foo +1 looks weird and I don't recall it elsewhere in the codebase. Occasionally I've seen foo+1 but foo + 1 I believe is more common

Lekensteyn · 2017-01-09T14:29:06Z

Can you modify this to use url-escaped paths (and make this encoding requirement very clear in the docs)? That will correctly handle paths containing NUL bytes.

(As for alternatives: curl_easy_setopt only accepts one parameter, so the length cannot be given. Requiring the user to pass a fixed-length buffer is error prone as well. I also considered an option to provide the length for an abstract socket path, but this is too complicated to get right.)

iboukris · 2017-01-09T19:24:22Z

@Lekensteyn that was my initial intention (see comments in #1061), however it got complicated in matter of design, usage and implementation.
And frankly, I think using such path embedding zeros is a good recipe for troubles and should be discouraged. Also all the tools and services I've seen using abstract socket had proper normal abstract path (with only null prefix).

iboukris · 2017-01-09T22:49:19Z

I've addressed Jay's comments and added the missing doc, please review - thanks!

bagder

👍

Lekensteyn

Looks good, only some doc nitpicks below :-)

Lekensteyn · 2017-01-12T09:22:56Z

docs/libcurl/opts/CURLOPT_ABSTRACT_UNIX_SOCKET.3

+.\" *                            | (__| |_| |  _ <| |___
+.\" *                             \___|\___/|_| \_\_____|
+.\" *
+.\" * Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.


We're already in 2017 :-)

Lekensteyn · 2017-01-12T09:23:20Z

docs/libcurl/opts/CURLOPT_ABSTRACT_UNIX_SOCKET.3

+.\" *
+.\" **************************************************************************
+.\"
+.TH CURLOPT_ABSTRACT_UNIX_SOCKET 3 "08 Jan 2016" "libcurl 7.53.0" "curl_easy_setopt options"


Lekensteyn · 2017-01-12T09:26:54Z

src/tool_help.c

@@ -272,6 +272,7 @@ static const char *const helptext[] = {
  "     --tlspassword STRING  TLS password",
  "     --tlsauthtype STRING  TLS authentication type (default: SRP)",
  "     --unix-socket FILE    Connect through this Unix domain socket",
+  "     --abstract-unix-socket PATH Connect to an abstract Unix domain socket",


For consistency with --unix-socket, maybe change FILE to PATH (or the other way round?)

It isn't a file in case of abstract, but path would be fine for both, I'll fix it.

In addition to unix domain sockets, Linux also supports an abstract namespace which is independent of the filesystem. In order to support it, add new CURLOPT_ABSTRACT_UNIX_SOCKET option which uses the same storage as CURLOPT_UNIX_SOCKET_PATH internally, along with a flag to specify abstract socket. On non-supporting platforms, the abstract address will be interpreted as an empty string and fail gracefully. Also add new --abstract-unix-socket tool parameter. Signed-off-by: Isaac Boukris <iboukris@gmail.com> Reported-by: Chungtsun Li (typeless)

iboukris · 2017-01-12T18:54:21Z

Thanks for the reviews, I've addressed @Lekensteyn's comments.

jay · 2017-01-12T19:00:17Z

It's better as FILE because that's what we use in the other options , see curl --help | grep FILE
Otherwise this looks ready to go. Before you send it upstream make sure to modify the commit message to point back to this thread with a Ref or Closes etc

jay · 2017-01-12T19:01:54Z

I may have spoken too soon maybe it's not better as FILE, if it's not a file.

iboukris · 2017-01-12T19:15:12Z

Yea, it is not a file, just a name (could be just 'foo', without slash).
In the unix(7) man page it is mostly referred to as path.

jay · 2017-01-12T19:33:24Z

Ok, nevermind then

iboukris · 2017-01-12T19:55:54Z

Actually, a regular unix socket which is a file, can also be set to just 'foo' as a relative path, but abstract is really not a file on the file system.

jay · 2017-01-12T20:07:05Z

Ok, thanks for explaining that. I think it was fine the way you had it then, FILE for unix domain and PATH for abstract unix domain.

iboukris · 2017-01-12T20:23:39Z

The man page terminology argument was about both :)

Lekensteyn

LGTM. I'll try to test the new functionality after the weekend and merge then.

(Hey, if you want to get your hands wet, maybe you can create another patch that extends the test suite with support for abstract domain sockets :-))

Lekensteyn · 2017-01-13T15:32:18Z

Decided to apply it now anyway. Briefly tested with an out-of-tree autotools build. Tested things like:

socat ABSTRACT-LISTEN:meh,fork TCP-CONNECT:lekensteyn.nl:443
src/curl -vL --abstract-unix-socket meh https://lekensteyn.nl/files

iboukris · 2017-01-13T15:42:34Z

Great, thanks all!

iboukris mentioned this pull request Jan 8, 2017

Unable to connect to unix socket of Linux's abstract namespace #1061

Closed

jay reviewed Jan 9, 2017

View reviewed changes

iboukris force-pushed the abstract_unix_domain_socket branch 2 times, most recently from 4e932d7 to f9fb05d Compare January 9, 2017 22:46

iboukris force-pushed the abstract_unix_domain_socket branch from f9fb05d to d03029d Compare January 9, 2017 23:58

bagder approved these changes Jan 10, 2017

View reviewed changes

Lekensteyn reviewed Jan 12, 2017

View reviewed changes

iboukris force-pushed the abstract_unix_domain_socket branch from d03029d to 57fcd60 Compare January 12, 2017 18:52

bagder assigned Lekensteyn Jan 13, 2017

Lekensteyn approved these changes Jan 13, 2017

View reviewed changes

Lekensteyn closed this in 1d786fa Jan 13, 2017

iboukris deleted the abstract_unix_domain_socket branch March 28, 2017 21:16

curl locked and limited conversation to collaborators May 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix_socket: add support for abstract unix domain socket #1197

unix_socket: add support for abstract unix domain socket #1197

iboukris commented Jan 8, 2017

mention-bot commented Jan 8, 2017

iboukris commented Jan 8, 2017

jay Jan 9, 2017

jay Jan 9, 2017

iboukris Jan 9, 2017

jay Jan 9, 2017

iboukris Jan 9, 2017

Lekensteyn commented Jan 9, 2017

iboukris commented Jan 9, 2017

iboukris commented Jan 9, 2017

bagder left a comment

Lekensteyn left a comment

Lekensteyn Jan 12, 2017

Lekensteyn Jan 12, 2017

Lekensteyn Jan 12, 2017

iboukris Jan 12, 2017

iboukris commented Jan 12, 2017

jay commented Jan 12, 2017

jay commented Jan 12, 2017

iboukris commented Jan 12, 2017

jay commented Jan 12, 2017

iboukris commented Jan 12, 2017 •

edited

jay commented Jan 12, 2017

iboukris commented Jan 12, 2017

Lekensteyn left a comment

Lekensteyn commented Jan 13, 2017

iboukris commented Jan 13, 2017

unix_socket: add support for abstract unix domain socket #1197

unix_socket: add support for abstract unix domain socket #1197

Conversation

iboukris commented Jan 8, 2017

mention-bot commented Jan 8, 2017

iboukris commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lekensteyn commented Jan 9, 2017

iboukris commented Jan 9, 2017

iboukris commented Jan 9, 2017

bagder left a comment

Choose a reason for hiding this comment

Lekensteyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iboukris commented Jan 12, 2017

jay commented Jan 12, 2017

jay commented Jan 12, 2017

iboukris commented Jan 12, 2017

jay commented Jan 12, 2017

iboukris commented Jan 12, 2017 • edited

jay commented Jan 12, 2017

iboukris commented Jan 12, 2017

Lekensteyn left a comment

Choose a reason for hiding this comment

Lekensteyn commented Jan 13, 2017

iboukris commented Jan 13, 2017

iboukris commented Jan 12, 2017 •

edited