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

alloc_sockaddr: Unix domain sockets: use provided address length #987

Merged
merged 1 commit into from Dec 25, 2016

Conversation

Projects
None yet
3 participants
@aantron
Contributor

aantron commented Dec 25, 2016

macOS (apparently) does not null-terminate Unix domain socket paths returned by getsockname. A sufficiently long path would also not be terminated on Linux (see man page, section BUGS). The current code in alloc_sockaddr assumes that the path is null-terminated. The fix uses the reported length instead of assuming the terminator, and subsumes the special case when the socket is not bound. The fix is based on the BUGS section of the linked man page.

The following code reproduces the problem on my macOS system, with trunk:

let () =
  let socket_path = "foobar" in

  let rec repeat = function
    | 0 -> ()
    | n ->
      let fd = Unix.(socket PF_UNIX SOCK_STREAM 0) in
      Unix.(bind fd (ADDR_UNIX socket_path));
      let actual_path =
        match Unix.getsockname fd with
        | Unix.ADDR_UNIX path -> path
        | _ -> assert false
      in
      Unix.close fd;
      Unix.unlink socket_path;
      if actual_path <> socket_path then begin
        prerr_endline actual_path;
        String.iter (fun c -> Printf.eprintf "%02X " (Char.code c)) actual_path;
        prerr_newline ();
        exit 1
      end;
      repeat (n - 1)
  in

  repeat 2

(* ocamlfind opt -linkpkg -package unix repro.ml *)

Running it produces outputs such as

foobarP(?   
66 6F 6F 62 61 72 50 28 BA 09 01

I looked in testsuite/tests/lib-unix, but it looks that code needs quite some work to be a proper test suite for the Unix library. I decided not to add tests.

aantron added a commit to ocsigen/lwt that referenced this pull request Dec 25, 2016

Show outdated Hide outdated otherlibs/unix/socketaddr.c
{ value n;
mlsize_t path_length =
strnlen(adr->s_unix.sun_path,
adr_len - offsetof(struct sockaddr_un, sun_path));

This comment has been minimized.

@gasche

gasche Dec 25, 2016

Member

would you add a comment pointing to the relevant man page section?

@gasche

gasche Dec 25, 2016

Member

would you add a comment pointing to the relevant man page section?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 25, 2016

Member

You should add a Changes entry -- see CONTRIBUTING.md#changelog.

Member

gasche commented Dec 25, 2016

You should add a Changes entry -- see CONTRIBUTING.md#changelog.

@aantron

This comment has been minimized.

Show comment
Hide comment
@aantron

aantron Dec 25, 2016

Contributor

Will squash on LGTM.

EDIT: I suppose squash-merge is more efficient :)

Contributor

aantron commented Dec 25, 2016

Will squash on LGTM.

EDIT: I suppose squash-merge is more efficient :)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 25, 2016

Member

You put the changelog entry in the section dedicated to the next minor release, which corresponds to the 4.04 branch, not trunk. Assuming that this bugfix is not urgent (this has been wrong for a long time so it can probably wait for the next major release), I think that trunk would be the better section. Also it would be nice if the changelog could explicitly mention that this was a bug affecting MacOS specifically -- so that people experiencing MacOS-specific issues with older versions can find it.

Member

gasche commented Dec 25, 2016

You put the changelog entry in the section dedicated to the next minor release, which corresponds to the 4.04 branch, not trunk. Assuming that this bugfix is not urgent (this has been wrong for a long time so it can probably wait for the next major release), I think that trunk would be the better section. Also it would be nice if the changelog could explicitly mention that this was a bug affecting MacOS specifically -- so that people experiencing MacOS-specific issues with older versions can find it.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 25, 2016

Contributor
Contributor

shindere commented Dec 25, 2016

@aantron

This comment has been minimized.

Show comment
Hide comment
@aantron

aantron Dec 25, 2016

Contributor

@gasche Moved it. Not sure which section it belongs in: Other libraries, or Bug fixes? There seems to be a bug fix to Unix in Other libraries, but it is breaking. Since this is not a major feature or breaking, I put it in Bug fixes for now.

For thoroughness, I'm mentioning the accidents that prevented this from being a bug on Linux.

  1. It would only occur for paths of length 108 on Linux, since Linux inserts the terminator otherwise. Unix.bind only allows paths of length 107 on Linux (even though bind(2) accepts 108). So, it would require a custom implementation of bind to trigger. However,
  2. all functions that eventually call alloc_sockaddr use OCaml's union sock_addr_union as the buffer, and, when making system calls, pass in its length (112), two bytes longer than the Linux struct sockaddr_un (110; two bytes for AF_UNIX and 108 for the path). This is because another union member, a struct sockaddr_in has a uint32_t field, so the whole union is padded. As a result, even for 108-byte paths passed by a custom bind(2) call, getsockname(2) and similar functions still have enough extra space to insert the null terminator.

@shindere I looked at the current test setup and ocamltest, but I am not able to readily find an example of an existing test that causes a side effect in the filesystem (relative to just ordinary compilation of the test), nor is there documentation in HACKING.adoc about where/how this is allowed. I'm reluctant to write a test that messes with an unfamiliar test environment in this way. Also, it has to be skipped on Windows, would I just add if not Sys.win32 then ... and go on? Is there some proper procedure for skipping tests? It would help if these things were documented, or else you can point me :)

A bit of a tangent, but testing in Travis on macOS would make the hypothetical test for this PR more immediately relevant.

Contributor

aantron commented Dec 25, 2016

@gasche Moved it. Not sure which section it belongs in: Other libraries, or Bug fixes? There seems to be a bug fix to Unix in Other libraries, but it is breaking. Since this is not a major feature or breaking, I put it in Bug fixes for now.

For thoroughness, I'm mentioning the accidents that prevented this from being a bug on Linux.

  1. It would only occur for paths of length 108 on Linux, since Linux inserts the terminator otherwise. Unix.bind only allows paths of length 107 on Linux (even though bind(2) accepts 108). So, it would require a custom implementation of bind to trigger. However,
  2. all functions that eventually call alloc_sockaddr use OCaml's union sock_addr_union as the buffer, and, when making system calls, pass in its length (112), two bytes longer than the Linux struct sockaddr_un (110; two bytes for AF_UNIX and 108 for the path). This is because another union member, a struct sockaddr_in has a uint32_t field, so the whole union is padded. As a result, even for 108-byte paths passed by a custom bind(2) call, getsockname(2) and similar functions still have enough extra space to insert the null terminator.

@shindere I looked at the current test setup and ocamltest, but I am not able to readily find an example of an existing test that causes a side effect in the filesystem (relative to just ordinary compilation of the test), nor is there documentation in HACKING.adoc about where/how this is allowed. I'm reluctant to write a test that messes with an unfamiliar test environment in this way. Also, it has to be skipped on Windows, would I just add if not Sys.win32 then ... and go on? Is there some proper procedure for skipping tests? It would help if these things were documented, or else you can point me :)

A bit of a tangent, but testing in Travis on macOS would make the hypothetical test for this PR more immediately relevant.

@aantron

This comment has been minimized.

Show comment
Hide comment
@aantron

aantron Dec 25, 2016

Contributor

If you wish, I can alter Unix.bind (in a separate PR?) so that it allows fullmaximum-length (108 bytes on Linux, IIRC 104 on OS XmacOS) paths.

Contributor

aantron commented Dec 25, 2016

If you wish, I can alter Unix.bind (in a separate PR?) so that it allows fullmaximum-length (108 bytes on Linux, IIRC 104 on OS XmacOS) paths.

@gasche gasche added the approved label Dec 25, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 25, 2016

Member

As far as I am concerned, this is good to merge. I don't have a strong opinion on whether it makes sense to delay the fix until we have an ocamltest test for it, or whether we should just log the need with a github label and come back to it after ocamltest is merged upstream.

Member

gasche commented Dec 25, 2016

As far as I am concerned, this is good to merge. I don't have a strong opinion on whether it makes sense to delay the fix until we have an ocamltest test for it, or whether we should just log the need with a github label and come back to it after ocamltest is merged upstream.

alloc_sockaddr: don't assume a null terminator
Some systems, macOS in particular, do not null-terminate Unix domain
socket paths returned by getsockname and other system calls.
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 25, 2016

Contributor
Contributor

shindere commented Dec 25, 2016

@aantron

This comment has been minimized.

Show comment
Hide comment
@aantron

aantron Dec 25, 2016

Contributor

Squashed.

Contributor

aantron commented Dec 25, 2016

Squashed.

@gasche gasche merged commit 886cccb into ocaml:trunk Dec 25, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 25, 2016

Member

Merged in trunk, thanks. I created a label ocamltest-related to track the relation. @shindere, is that enough for you, or should I also create a mantis issue to make sure we do not forget about it?

Member

gasche commented Dec 25, 2016

Merged in trunk, thanks. I created a label ocamltest-related to track the relation. @shindere, is that enough for you, or should I also create a mantis issue to make sure we do not forget about it?

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 26, 2016

Contributor
Contributor

shindere commented Dec 26, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #987 from aantron/getsockname
alloc_sockaddr: Unix domain sockets: use provided address length
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment