>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 [email protected]:/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 <[email protected]>
+ * 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:
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "[email protected]"
