From patchwork Fri Nov 7 08:35:31 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexandre Oliva X-Patchwork-Id: 3604 Received: (qmail 13077 invoked by alias); 7 Nov 2014 08:35:50 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 13066 invoked by uid 89); 7 Nov 2014 08:35:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com From: Alexandre Oliva To: libc-alpha@sourceware.org Subject: ctermid: return string literal, document MT-Safety pitfall Date: Fri, 07 Nov 2014 06:35:31 -0200 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 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 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 -/* 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); }