libc: return error number on ptsname_r failure

[emaste]

Summary

When we implemented ptsname_r in 2020 in commit 3e7224dffe26 we returned
-1 on error and set errno.  POSIX has subsequently adopted ptsname_r[1]
but specifies that an error number is returned on error.

In a5ed6a815e38 I changed the man page to indicate that a nonzero value
is returned on error, and callers should not be checking for an explicit
-1.  Follow up now by returning the error number (but continue to set
errno).

Existing software uses the previous implementation via symbol
versioning.

[1] https://www.austingroupbugs.net/view.php?id=508

PR: 250062

[kib]

This is breaking change and I do not see how the versioning helps there. If you take 'old' program and recompile it on patched system, it gets linked to the new symbol.

[emaste]

If you take 'old' program and recompile it on patched system

Yes, this change means software needs to follow POSIX's idea of ptsname_r.

The only uses in the base system right now are from LLVM and are already compatible with this change

contrib/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

#if SANITIZER_INTERCEPT_PTSNAME_R
INTERCEPTOR(int, ptsname_r, int fd, char *name, SIZE_T namesize) {
  void *ctx;
  COMMON_INTERCEPTOR_ENTER(ctx, ptsname_r, fd, name, namesize);
  int res = REAL(ptsname_r)(fd, name, namesize);
  if (res == 0)
    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, name, internal_strlen(name) + 1);
  return res;
}
#define INIT_PTSNAME_R COMMON_INTERCEPT_FUNCTION(ptsname_r);

contrib/llvm-project/lldb/source/Host/common/PseudoTerminal.cpp

char buf[PATH_MAX];
buf[0] = '\0';
int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
(void)r;
assert(r == 0);
return buf;

I had a look at a sample of GitHub search results for ptsname_r and they generally check zero/nonzero, e.g.
https://github.com/FrameworkComputer/EmbeddedController/blob/f6d6b927eed71550d3475411cfc3e59abe5cef2a/util/ec_uartd.c#L99

	if (ptsname_r(fd, ptname, PATH_MAX) != 0) {
		perror("getting name of pty");
		return 3;
	}

There are some counterexamples; I just submitted a pull request for one case I found https://github.com/containers/crun/pull/1340.

[kib]

Maybe we should add something like posix_ptsname_r() instead, and #define ptsname_r() to posix_ptsname_r() under the desired POSIX_VERSION. Then we need to add new right POSIX_VERSION, but this sounds better (for me) than randomly breaking ABI.

[delphij]

In D42470#969304, @kib wrote:

Maybe we should add something like posix_ptsname_r() instead, and #define ptsname_r() to posix_ptsname_r() under the desired POSIX_VERSION. Then we need to add new right POSIX_VERSION, but this sounds better (for me) than randomly breaking ABI.

(I assume you mean "breaking API" here, as we are preserving the exact behavior via symbol versioning and therefore ABI is preserved).

Unfortunately, it seems there is no perfect way to avoid this change if we want to align with POSIX, and I believe there are compelling reasons to move toward POSIX's interface, even at cost of breaking the API slightly, and we should do it earlier than later.

The current implementation relies on errno, making it thread-safe but not truly reentrant. If we were pedantic about the _r suffix, the POSIX definition is superior as it eliminates the reliance on thread-local state, ensuring the function is async-signal-safe.

Given that we are using symbol versioning to maintain backward compatibility for existing binaries, the risk is limited to source-level compatibility, and such move would encourage the (in my opinion, see above) better and POSIX compliant interface.

[kib]

Well, API or ABI, as is it is a sudden change in the behavior for the same code compiled against new or old base. IMO the #define posix is the required workaround if we going to change the ptsname_r() this way.

And of course, the change as is already get stalled, because current symver for additions in 16-CURRENT is FBSD_1.9.