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

unix_socket: add support for abstract unix domain socket #1197

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@frenche
Contributor

frenche commented Jan 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 8, 2017

@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.

mention-bot commented Jan 8, 2017

@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.

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 8, 2017

Contributor

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

Contributor

frenche commented Jan 8, 2017

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

Show outdated Hide outdated 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);

This comment has been minimized.

@jay

jay Jan 9, 2017

Member

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);
}
@jay

jay Jan 9, 2017

Member

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);
}

This comment has been minimized.

@jay

jay Jan 9, 2017

Member

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);
  }
}
@jay

jay Jan 9, 2017

Member

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);
  }
}

This comment has been minimized.

@frenche

frenche Jan 9, 2017

Contributor

Thanks, I'll fix this.

@frenche

frenche Jan 9, 2017

Contributor

Thanks, I'll fix this.

Show outdated Hide outdated 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;

This comment has been minimized.

@jay

jay Jan 9, 2017

Member

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

@jay

jay Jan 9, 2017

Member

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

This comment has been minimized.

@frenche

frenche Jan 9, 2017

Contributor

Ok.

@frenche

frenche Jan 9, 2017

Contributor

Ok.

@Lekensteyn

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Jan 9, 2017

Member

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.)

Member

Lekensteyn commented Jan 9, 2017

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.)

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 9, 2017

Contributor

@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).

Contributor

frenche commented Jan 9, 2017

@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).

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 9, 2017

Contributor

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

Contributor

frenche commented Jan 9, 2017

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

@bagder

bagder approved these changes Jan 10, 2017

👍

@Lekensteyn

Looks good, only some doc nitpicks below :-)

Show outdated Hide outdated docs/libcurl/opts/CURLOPT_ABSTRACT_UNIX_SOCKET.3
.\" * | (__| |_| | _ <| |___
.\" * \___|\___/|_| \_\_____|
.\" *
.\" * Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

We're already in 2017 :-)

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

We're already in 2017 :-)

Show outdated Hide outdated 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"

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

2017

@Lekensteyn
Show outdated Hide outdated 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",

This comment has been minimized.

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

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

@Lekensteyn

Lekensteyn Jan 12, 2017

Member

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

This comment has been minimized.

@frenche

frenche Jan 12, 2017

Contributor

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

@frenche

frenche Jan 12, 2017

Contributor

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

unix_socket: add support for abstract unix domain socket
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)
@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 12, 2017

Contributor

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

Contributor

frenche commented Jan 12, 2017

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

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 12, 2017

Member

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

Member

jay commented Jan 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 12, 2017

Member

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

Member

jay commented Jan 12, 2017

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

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 12, 2017

Contributor

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.

Contributor

frenche commented Jan 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 12, 2017

Member

Ok, nevermind then

Member

jay commented Jan 12, 2017

Ok, nevermind then

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 12, 2017

Contributor

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.

Contributor

frenche commented Jan 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Jan 12, 2017

Member

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.

Member

jay commented Jan 12, 2017

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.

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 12, 2017

Contributor

The man page terminology argument was about both :)

Contributor

frenche commented Jan 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Jan 13, 2017

Member

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
Member

Lekensteyn commented Jan 13, 2017

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
@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Jan 13, 2017

Contributor

Great, thanks all!

Contributor

frenche commented Jan 13, 2017

Great, thanks all!

@frenche frenche deleted the frenche:abstract_unix_domain_socket branch Mar 28, 2017

@curl curl locked and limited conversation to collaborators May 30, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.