Date: Tue, 31 Jan 2012 23:06:53 GMT From: Matthew Story <matthewstory@gmail.com> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/164674: vsprintf/vswprintf return error (EOF) on success if __SERR flag is already set on file Message-ID: <201201312306.q0VN6roO091429@red.freebsd.org> Resent-Message-ID: <201201312310.q0VNA949037216@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 164674
>Category: kern
>Synopsis: vsprintf/vswprintf return error (EOF) on success if __SERR flag is already set on file
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Tue Jan 31 23:10:09 UTC 2012
>Closed-Date:
>Last-Modified:
>Originator: Matthew Story
>Release: 9.0-Release
>Organization:
>Environment:
FreeBSD matt9fromouterspace 9.0-RELEASE FreeBSD 9.0-RELEASE #0: Tue Jan 3 07:15:25 UTC 2012 root@obrian.cse.buffalo.edu:/usr/obj/usr/src/sys/GENERIC i386
>Description:
The printf family of functions behaves unpredictably if the file passed to the function has the __SERR flag set in _flags. The one consistency across all of the cases detailed below is that regardless of the number of bytes written to the file, and the success/failure of that operation, the printf functions (printf, fprintf, wprintf, fwprintf) will always return -1 (EOF).
* For the case of an unbuffered file, the operation currently fails transmitting no bytes, and returns -1.
* For the case of a buffered file, the operation transmits all bytes and the function returns -1.
The problem is that the behavior here is inconsistent, and should not be. The question is wether all should be made to fail consistently and transmit no bytes if __SERR is set on _flags, or if failure should be determined based on the success of byte transmission, discounting file state.
Per the ISO/IEC 9899:201x (C11) Section 7.21.6.1, 14:
The fprintf function returns the number of characters transmitted, or a negative value if an output or encoding error occurred.
My reading of this specification is that success should be determined based on byte-transmission, and should not factor-in file state. In addition to the ISO standard, the glibc implementation will reliably succeed with any error flag set if bytes are successfully transmitted (although it will transmit partial messages prior to successful conversion, which is unfortunate).
The attached patch makes the operation on buffered and unbuffered files consistent across the affected printf/wprintf functions, determines success/failure based on successful byte-transmission alone, and preserves _flags state for __SERR as passed in.
I originally discovered this behavior through black-box testing ``%<n>'' behavior against data-types through programmatic exhaustion, although a more likely cause of file error state is reading from a file open in write mode, prior to writing to that file (which is the example used in the repeat steps below).
>How-To-Repeat:
/* this simple program demonstrates behavior on buffered (stdout) and unbuffered (stderr) files */
#include <stdio.h>
#define HELLO "Hello, world\n"
int
main() {
/* make sure everything is nice and clean */
clearerr(stdout);
/* read from write-only file, should cause an error */
fgetc(stdout);
printf("stdout is buffered, and %s an error\n",
ferror(stdout) ? "has":"doesn't have");
printf("bytes transmitted without replace: %d\n",
printf(HELLO));
printf("bytes transmitted with replace: %d\n",
printf("%s", HELLO));
clearerr(stderr);
/* read from write-only file, should cause an error */
fgetc(stderr);
/*
* print diagnostics to stdout, because some will not
* print to stderr correctly if stderr has an error
*/
printf("stderr is unbuffered, and %s an error\n",
ferror(stderr) ? "has":"doesn't have");
printf("bytes transmitted without replace: %d\n",
fprintf(stderr, HELLO));
printf("bytes transmitted with replace: %d\n",
fprintf(stderr, "%s", HELLO));
return 0;
}
>Fix:
See patch, my fix is to store and then unset the error flag inside of the exclusive lock, and then to reset the error flag to the original (or new) state after the vs(w)printf prior to releasing the exclusive lock.
this change maintains the ferror state of the file as passed in or as set by __vfprintf, but allows for vs(w)printf to determine success of the print operation independent of the state of the file as passed in. there may be other ways to handle this more gracefully, but this change seems to me to be the safest and most minimally impacting solution.
The patch applies cleanly on both 8.2/9.0, and includes an additional regression test for stdio pertaining to correctness of return codes.
Patch attached with submission follows:
diff -urN old/lib/libc/stdio/vfprintf.c new/lib/libc/stdio/vfprintf.c
--- old/lib/libc/stdio/vfprintf.c 2012-01-31 17:35:31.025336246 -0500
+++ new/lib/libc/stdio/vfprintf.c 2012-01-31 17:44:50.078652303 -0500
@@ -265,14 +265,24 @@
{
int ret;
+ short orig_err;
FLOCKFILE(fp);
+ /*
+ * store flags, unset errors, success of printf is
+ * independent of previous errors on the fd
+ */
+ orig_err = fp->_flags & __SERR;
+ fp->_flags &= ~__SERR;
+
/* optimise fprintf(stderr) (and other unbuffered Unix files) */
if ((fp->_flags & (__SNBF|__SWR|__SRW)) == (__SNBF|__SWR) &&
fp->_file >= 0)
ret = __sbprintf(fp, fmt0, ap);
else
ret = __vfprintf(fp, fmt0, ap);
+
+ fp->_flags |= orig_err & __SERR;
FUNLOCKFILE(fp);
return (ret);
}
diff -urN old/lib/libc/stdio/vfwprintf.c new/lib/libc/stdio/vfwprintf.c
--- old/lib/libc/stdio/vfwprintf.c 2012-01-31 17:35:31.021336063 -0500
+++ new/lib/libc/stdio/vfwprintf.c 2012-01-31 17:46:28.106128024 -0500
@@ -347,14 +347,24 @@
{
int ret;
+ short orig_err;
FLOCKFILE(fp);
+ /*
+ * store flags, unset errors, success of wprintf is
+ * independent of previous errors on the fd
+ */
+ orig_err = fp->_flags & __SERR;
+ fp->_flags &= ~__SERR;
+
/* optimise fprintf(stderr) (and other unbuffered Unix files) */
if ((fp->_flags & (__SNBF|__SWR|__SRW)) == (__SNBF|__SWR) &&
fp->_file >= 0)
ret = __sbprintf(fp, fmt0, ap);
else
ret = __vfwprintf(fp, fmt0, ap);
+
+ fp->_flags |= orig_err & __SERR;
FUNLOCKFILE(fp);
return (ret);
}
diff -urN old/tools/regression/lib/libc/stdio/Makefile new/tools/regression/lib/libc/stdio/Makefile
--- old/tools/regression/lib/libc/stdio/Makefile 2012-01-31 17:36:04.247820192 -0500
+++ new/tools/regression/lib/libc/stdio/Makefile 2012-01-31 17:41:30.201758177 -0500
@@ -1,6 +1,6 @@
# $FreeBSD: stable/9/tools/regression/lib/libc/stdio/Makefile 189142 2009-02-28 06:39:39Z das $
-TESTS= test-getdelim test-perror test-print-positional test-printbasic test-printfloat test-scanfloat
+TESTS= test-getdelim test-perror test-print-positional test-printbasic test-printfloat test-scanfloat test-printreturn
CFLAGS+= -lm
.PHONY: tests
diff -urN old/tools/regression/lib/libc/stdio/test-printreturn.c new/tools/regression/lib/libc/stdio/test-printreturn.c
--- old/tools/regression/lib/libc/stdio/test-printreturn.c 1969-12-31 19:00:00.000000000 -0500
+++ new/tools/regression/lib/libc/stdio/test-printreturn.c 2012-01-31 17:41:30.200756664 -0500
@@ -0,0 +1,148 @@
+/*-
+ * Copyright (c) 2012 Matthew Story <matthewstory@gmail.com>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/*
+ * Tests for correct return codes from f(w)printf
+ */
+
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <wchar.h>
+#include <errno.h>
+#include <string.h>
+
+#define BATTERY_OK "ok %d - printreturn\n"
+
+#define run_battery(void) \
+ _run_battery(__LINE__)
+
+void smash_stack(void);
+void _run_battery(int line);
+int battery = 1;
+
+int
+main() {
+ /*
+ * All tests occur on stdout, this makes the tests quite noisy.
+ * but keeps them contained -- e.g. no reliance on external
+ * files, or on manipulating file struct internals.
+ */
+
+ printf("1..12\n");
+ /*
+ * without errors
+ */
+ clearerr(stdout);
+
+ /* line-buffered */
+ setvbuf(stdout, (char *)NULL, _IOLBF, 0);
+ run_battery();
+
+ /* unbuffered */
+ setvbuf(stdout, (char *)NULL, _IONBF, 0);
+ run_battery();
+
+ /* fully-buffered */
+ setvbuf(stdout, (char *)NULL, _IOFBF, 0);
+ run_battery();
+
+ /*
+ * success with errors, would rather not manipulate _flags * directly, but
+ * can't find another direct way to set error, without an actual error.
+ */
+ stdout->_flags |= __SERR;
+
+ /* line-buffered */
+ setvbuf(stdout, (char *)NULL, _IOLBF, 0);
+ run_battery();
+
+ /* unbuffered */
+ setvbuf(stdout, (char *)NULL, _IONBF, 0);
+ run_battery();
+
+ /* fully-buffered */
+ setvbuf(stdout, (char *)NULL, _IOFBF, 0);
+ run_battery();
+
+ return 0;
+}
+
+/* taken from test-printbasic.c */
+void
+smash_stack(void)
+{
+ static uint32_t junk = 0xdeadbeef;
+ uint32_t buf[512];
+ int i;
+
+ for (i = 0; i < sizeof(buf) / sizeof(buf[0]); i++)
+ buf[i] = junk;
+}
+
+void
+_run_battery(int line) {
+#define BUF 100
+ extern int battery;
+ wchar_t ws[BUF], wfmt[BUF];
+ char s[BUF];
+
+ /* regular width tests, no replacement, and replacement */
+ smash_stack();
+ sprintf(s, BATTERY_OK, battery);
+ if (0 > fprintf(stdout, s)) {
+ fprintf(stderr,
+ "%d: failure in no replace printf (%d)\n",
+ line, errno);
+ abort();
+ }
+ ++battery;
+ smash_stack();
+ if (0 > fprintf(stdout, BATTERY_OK, battery)) {
+ fprintf(stderr, "%d: failure in replace printf (%d)\n", line, errno);
+ abort();
+ }
+ ++battery;
+
+ /* wide tests, no replacement, and replacement */
+ smash_stack();
+ mbstowcs(wfmt, BATTERY_OK, BUF - 1);
+ swprintf(ws, sizeof(ws) / sizeof(ws[0]), wfmt, battery);
+ if (0 > fwprintf(stdout, ws)) {
+ fprintf(stderr,
+ "%d: failure in no replace wprintf (%d)\n",
+ line, errno);
+ abort();
+ }
+ ++battery;
+ smash_stack();
+ if (0 > fwprintf(stdout, wfmt, battery)) {
+ fprintf(stderr, "%d: failure in replace wprintf (%d)\n", line, errno);
+ abort();
+ }
+ ++battery;
+}
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201312306.q0VN6roO091429>