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

fix bug in abstract sockets #3908

Merged
merged 1 commit into from Mar 20, 2023

Conversation

walid-git
Copy link
Member

@walid-git walid-git commented Mar 16, 2023

Current behavior:

bin/varnishd/varnishd -a @varnish -f /etc/varnish/default.vcl -n /tmp/varnishd`
ss -lnxp | grep varnish
u_str LISTEN 0      1024   @varnish@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ 2741935            * 0    users:(("cache-main",pid=894543,fd=3),("varnishd",pid=894529,fd=3))  

As you can see, the whole 108 bytes of sun_path are used as the socket name/path.
From man UNIX(7):

The socket's address in this namespace is given by the additional bytes in sun_path that are covered by the specified length of the address structure. (Null bytes in the name have no special significance.) The name has no connection with filesystem pathnames. When the address of an abstract socket is returned, the returned addrlen is greater than sizeof(sa_family_t) (i.e., greater than 2), and the name of the socket is contained in the first (addrlen - sizeof(sa_family_t)) bytes of sun_path.

Which means that socklen is used to determine the length of name and must be set to the length of the struct sockaddr_un up to the last character of name, otherwise the 108 characters of sun_path will be treated as the name of the socket, including NULL bytes.

@nigoroll
Copy link
Member

It's interesting how I understand exactly that paragraph from UNIX(7) in the opposite way.

@walid-git
Copy link
Member Author

I agree that it can look confusing, but for me Null bytes in the name have no special significance. means that NULL bytes do not act as string delimiters, which means that the only other way is to use the length argument.

@nigoroll
Copy link
Member

So this means we are incompatible with other implementations? Have you actually checked/tested?

@walid-git
Copy link
Member Author

Yes I tried to connect as a client with socat - ABSTRACT-CLIENT:varnish, it is unable to connect without this patch

@nigoroll
Copy link
Member

someone(tm) should have done that earlier. Sorry and thank you!

lib/libvarnish/vus.c Outdated Show resolved Hide resolved
with uds abstract sockets, sun_path should start with a NULL character followed
by the socket's name. The name is not considered to be NULL terminated and can
contain NULL bytes which have no special meaning. socklen is used to determine
the length of name and must be set to the length of the struct sockaddr_un up to
the last character of name, otherwise the 108 characters of sun_path will be
treated as the name of the socket, including NULL bytes.
@nigoroll nigoroll merged commit 50e03b0 into varnishcache:master Mar 20, 2023
@nigoroll
Copy link
Member

Thank you very much!

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

Successfully merging this pull request may close these issues.

None yet

2 participants