Make make_absolute thread-local in chdir.c

[CPunisher]

make_absolute previously used a process-wide static buffer to assemble absolute paths for relative path resolution. In _REENTRANT builds, concurrent calls could overwrite or reallocate that shared buffer while another thread was still using it, leading to races and potentially invalid pointers.

Related #474

[alexcrichton]

Thanks for the PR! I think though this might be something that's more appropriate to use synchronization for rather than thread-local storage. These are malloc'd buffers which means that if it were thread-local then a thread exiting would leak the buffer information. As a static that doesn't matter too much as the leaked memory is bounded, but adding threads into the mix could make it much worse. Could this use a mutex intead?

[CPunisher]

I've updated the code to use synchronization instead of thread-local storage.

I ended up protecting the whole __wasilibc_find_relpath_alloc with a mutex, because the lifetime of make_absolute_buf extends until strcpy(*relative_buf, rel).

[alexcrichton]

I think that the locking needs to be in the callers of __wasilibc_find_relpath_alloc because the function itself doesn't use any statics and it's the callers that are using statics and using the values beyond the return of __wasilibc_find_relpath_alloc

[CPunisher]

because the function itself doesn't use any statics

__wasilibc_find_relpath_alloc does rely on statics here, as make_absolute() returns a static pointer static char *make_absolute_buf = NULL.

The mutex in __wasilibc_find_relpath_alloc is intended to protect the returnt const char *abspath = make_absolute(path). In particular, __wasilibc_find_abspath() consumes that pointer, and the resulting rel pointer may still refer into the same static buffer until it is copied into relative_buf.

it's the callers that are using statics and using the values beyond the return of __wasilibc_find_relpath_alloc

If you mean the caller-managed char **relative_buf that may be reallocated in __wasilibc_find_relpath_alloc by char *tmp = realloc(*relative_buf, rel_len + 1);, then as far as I can tell, the only caller that currently has a data race there is chdir(), since it keeps relative_buf statically. The other internal callers already use static __thread for their reusable buffers.

The two reasonable options for chdir() are:

1. add synchronization around chdir()'s shared static relative_buf, or
2. make chdir() use TLS like the other callers do.

[alexcrichton]

Ah right yeah good points, I misread! Ok in that case I'm going to go ahead and merge this as it handles __wasilibc_find_relpath_alloc regardless in terms of its responsibilities.

Would you be interested in making PRs to fix the other callers? Personally I think they should all switch to synchronization instead of thread-local buffers for the same reason that I don't think thread-local buffers are appropriate here. Or, alternatively, if the thread-local buffers are cleaned up at some point then I think it's reasonable to use thread-local buffers.

[CPunisher]

Yeah thanks for your trust. I'm willing to continue, and I'll also consider the best approach.