fixed SYS___pthread_chdir value for FreeBSD when making call to syscall

[jasonkafer]

Fix for FreeBSD LibC chdir syscal

[brettwooldridge]

Thanks for the pull request, but please remove unrelated whitespace changes.

[jasonkafer]

Whitespace has been removed, sorry about that.

[bturner]

If we don't put the chdir constant back here, should this line be removed?

[bturner]

It doesn't seem like this should be mutable. This is clearly intended to be a constant.

Rather than initializing this here and changing it in the static block, which requires this to be mutable, leave it uninitialized here and update the static block to set the "right" value:

public static final int SYS___pthread_chdir;

static {
   // O_NONBLOCK initialization as now

   SYS___pthread_chdir = System.getProperty("os.name").toLowerCase().contains("freebsd") ? 12 : 348;

   // Register Native
}

If this constant is going to move to LibC, should it be further down, with the others (like O_NONBLOCK)? Alternatively, perhaps the constant should just stay where it was, paired with a new static block inside SyscallLibrary to initialize it?

[skissane]

I just wanted to make the point that SYS__pthread_chdir and SYS__chdir do really different things – the first sets the per-thread working directory, the second sets the per-process working directory. FreeBSD does not support per-thread working directory. Very few platforms do. In fact, the only platforms I know of that do are Linux and XNU (but Linux's implementation is very different from, and incompatible with, XNU's).

Setting SYS__pthread_chdir = 12 on FreeBSD is going to really confuse people because the constant name implies "per-thread working directory" but you are setting it to a value which means "per-process working directory". It is also going to introduce thread safety issues when replacing a thread-safe API with a non-thread-safe one.

It is possible that FreeBSD developers might some day add support for pthread_chdir_np/pthread_fchdir_np. On the one hand, they'll likely argue that it is the wrong approach, and openat/etc are the right approach. Especially given FreeBSD supports that approach in scenarios many other platforms don't (e.g. bindat/connectat for Unix domain sockets). OTOH, making it easier to port software from macOS/XNU might convince them.

But, in the meantime, I think the whole approach here is wrong – since NuProcesses' macOS process startup code is based on SYS__pthread_chdir, it is not a good base for a FreeBSD implementation.

A better approach could be something like this:

• Refactor OsxProcess into an abstract base class BsdProcess, with two concrete subclasses – OsxProcess and FreeBsdProcess
• OsxProcessFactory will decide at runtime whether to create an OsxProcess or a FreeBsdProcess depending on what the OS is (possibly should rename it to BsdProcessFactory, but that might break backward compatibility – one option might be to do the rename, but keep OsxProcessFactory as a dummy subclass)
• Make the spawnWithCwd method of BsdProcess be protected abstract, so the concrete subclasses have to implement their own spawnWithCwd
• For the FreeBsdProcess implementation of spawnWithCwd, use a completely different strategy. For example, you could spawn an external helper binary written in C, which does a chdir() to its first argument, then execs the remaining arguments.
• An alternative to relying on an external helper binary: LinuxProcess does this (using the jspawnhelper binary shipped with the JDK) – I'm not sure if that code works on FreeBSD, but if it does you could base FreeBsdProcess more off LinuxProcess than OsxProcess.
• You could even possibly use /bin/sh as in this: https://unix.stackexchange.com/a/480779
• As that link notes, there are heaps of third party utilities which can do this (cd and exec), but while some of them may be in FreeBSD ports, you can't rely on them being there on a random FreeBSD system. Although, that might be some argument for a system property/etc to let you specify an arbitrary utility.

[bturner]

Thanks for the detailed feedback, @skissane! I'll readily admit, I lack the depth of knowledge you have around FreeBSD and how some of these internals work there. Looking purely at end behavior, though, trying to use the per-process work directory to control working directories for processes we spawn, and can actively be spawning on multiple threads concurrently, seems like an obvious no-no. (I no longer work on Bitbucket Server, but thinking about this in terms of Bitbucket Server's usage of the library it'd be pretty disastrous to, for example, have 2 threads that are trying to operate on different repositories end up racily changing the process-wide working directory and one thread's update stomps the other, resulting in the forked git processes both running on the same repository. The support case to try and find that issue would be brutal.)

Based on that potential issue, and reinforced given that I asked for a variety of rework on this pull request over 3 years ago and it hasn't been actioned, I think the right approach is to decline this change and consider other options separately. One thing I'll add to that, though, is that I don't a) use FreeBSD or b) have any sort of FreeBSD setup, so I personally am unlikely to make any inroads on a solution to this issue.