ctermid: return string literal, document MT-Safety pitfall

[Alexandre Oliva <aoliva at redhat dot com>]

The ctermid implementation, like cuserid, uses a static buffer.  I
noticed this one, but I reasoned that, since the buffer was initialized
with the same short string in every thread that called the function
without passing it a buffer, the value would remain unchanged, and so no
harmful effects would be caused by what is technically a data race.

This was based on an interpretation that strcpy (and memcpy, and
compiler-inlined versions thereof) could not write garbage in the
destination before writing the intended values, because this would be a
deviation from the specification, and it could be observed by an
asynchronous signal handler.

Whether or not this reading of POSIX is correct is not so important:
ctermid can be implemented so as to return a pre-initialized static
buffer, instead of initializing it every time.  Callers are not allowed
by POSIX to modify this buffer, so we can even make it read-only.  This
patch does this, to sidestep the debate.  It might even be the case that
it makes ctermid more efficient, since it avoids reinitializing a static
buffer every time.  GCC is still smart enough to notice that, when a
buffer is passed in, the string copied to it is a known constant, so it
optimizes the strcpy to the same sequence of stores used before this
patch.

As for the MT-Safety documentation, I update the comments next to the
annotations to reflect this change in the implementation, add a note
indicating we diverge from POSIX in the static buffer case (MT-Safety is
not required), and suggest that, when we drop the note that indicates
this is preliminary documentation about the current implementation,
rather than a commitment to remain within these safety boundaries in the
future, we may want to add a note indicating the possibility of a race
condition.

Ok to install?


From: Alexandre Oliva <aoliva@redhat.com>

for  ChangeLog

	* sysdeps/posix/ctermid.c (ctermid): Return a pointer to a
	string literal if not passed a buffer.
	* manual/job.texi (ctermid): Update reasoning, note deviation
	from posix, suggest mtasurace when not passed a buffer, for
	future non-preliminary safety notes.
---
 manual/job.texi         |    8 +++++---
 sysdeps/posix/ctermid.c |   13 +++++++------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/manual/job.texi b/manual/job.texi
index 4f9bd81..095c26d 100644
--- a/manual/job.texi
+++ b/manual/job.texi
@@ -1039,10 +1039,12 @@ The function @code{ctermid} is declared in the header file
 @comment stdio.h
 @comment POSIX.1
 @deftypefun {char *} ctermid (char *@var{string})
-@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
+@safety{@prelim{}@mtsafe{@mtsposix{/!string}}@assafe{}@acsafe{}}
 @c This function is a stub by default; the actual implementation, for
-@c posix systems, returns an internal buffer if passed a NULL string,
-@c but the internal buffer is always set to /dev/tty.
+@c posix systems, returns a pointer to a string literal if passed a NULL
+@c string.  It's not clear we want to commit to being MT-Safe in the
+@c !string case, so maybe add mtasurace{:ctermid/!string} when we take
+@c prelim out, to make room for using a static buffer in the future.
 The @code{ctermid} function returns a string containing the file name of
 the controlling terminal for the current process.  If @var{string} is
 not a null pointer, it should be an array that can hold at least
diff --git a/sysdeps/posix/ctermid.c b/sysdeps/posix/ctermid.c
index 0ef9a3f..ca81d42 100644
--- a/sysdeps/posix/ctermid.c
+++ b/sysdeps/posix/ctermid.c
@@ -19,17 +19,18 @@
 #include <string.h>
 
 
-/* Return the name of the controlling terminal.
-   If S is not NULL, the name is copied into it (it should be at
-   least L_ctermid bytes long), otherwise a static buffer is used.  */
+/* Return the name of the controlling terminal.  If S is not NULL, the
+   name is copied into it (it should be at least L_ctermid bytes
+   long), otherwise we return a pointer to a non-const but read-only
+   string literal, that POSIX states the caller must not modify.  */
 char *
 ctermid (s)
      char *s;
 {
-  static char name[L_ctermid];
+  char *name = (char /*drop const*/ *) "/dev/tty";
 
   if (s == NULL)
-    s = name;
+    return name;
 
-  return strcpy (s, "/dev/tty");
+  return strcpy (s, name);
 }


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer