[v2] ptsname_r: don't leak uninitialized memory
Commit Message
If the fd refers to a terminal device, but not a pty master, the
TIOCGPTN ioctl returns with ENOTTY. This error is not caught, and the
possibly undefined buffer passed to ptsname_r is sent directly to the
stat64 syscall.
Fix this by using a fallback to the old method only if the TIOCGPTN
ioctl fails with EINVAL. This also fix the return value in that specific
case (it return ENOENT without this patch).
Also add tests to the ptsname_r function (and ptsname at the same time).
Note: this is Debian bug#741482, reported by Jakub Wilk <jwilk@debian.org>
---
ChangeLog | 8 +++
login/Makefile | 2 +-
login/tst-ptsname.c | 138 ++++++++++++++++++++++++++++++++++++++
sysdeps/unix/sysv/linux/ptsname.c | 4 +-
4 files changed, 150 insertions(+), 2 deletions(-)
create mode 100644 login/tst-ptsname.c
v1 -> v2: add tests.
Comments
Any user-visible bug needs a bugzilla report filed and BZ# tag in log entries.
> + * sysdeps/unix/sysv/linux/ptsname.c (__ptsname_internal): return
> + errno if the TIOCGPTN ioctl fails with an error different than
> + EINVAL.
Capitalize the sentence.
> --- /dev/null
> +++ b/login/tst-ptsname.c
> @@ -0,0 +1,138 @@
A new file needs the standard header at the top: a descriptive one-line
comment, and the copyright header.
> +static int
> +test_ok (int fd)
It looks simple enough to consolidate all these into a single function that
takes (int fd, char *buf, size_t buflen, int expected_error).
> + printf ("ptsname_r(): expected: return = %d\n", 0);
"()" is not part of a function's name.
Don't use it in messages (or comments).
> + int ret, err;
> + char *buf = NULL;
> +
> + ret = ptsname_r (fd, buf, 1);
> + err = errno;
Don't pre-declare. Always use C99 style and define at first assignment.
> + const char file[] = "./ptsname-einval";
Just use a macro for the string constant, not a local variable (which does
a useless copy instead of just passing the rodata string constant).
This function too can use the common function above (twice) for most of its
guts.
> + /* Then try with a terminal device which is not a master. */
> + ret = ptsname_r (0, buf, sizeof (buf));
Don't assume stdin is a terminal. (Incidentally, when using stdin is
appropriate, using STDIN_FILENO from <unistd.h> is good implicit
documentation of intent.) You can open /dev/tty, but the test should
not report failure (perhaps report untestable if we have that now)
if the open fails.
> +int
> +main (void)
Use test-skeleton.c.
> + close(fd);
Space before paren.
Thanks,
Roland
@@ -1,3 +1,11 @@
+2014-05-06 Aurelien Jarno <aurelien@aurel32.net>
+
+ * sysdeps/unix/sysv/linux/ptsname.c (__ptsname_internal): return
+ errno if the TIOCGPTN ioctl fails with an error different than
+ EINVAL.
+ * login/tst-ptsname.c: New file.
+ * login/Makefile (tests): Add tst-ptsname.
+
2014-05-05 Aurelien Jarno <aurelien@aurel32.net>
* po/fr.po: Fix French translation of inappropriate.
@@ -43,7 +43,7 @@ endif
subdir-dirs = programs
vpath %.c programs
-tests := tst-utmp tst-utmpx tst-grantpt
+tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname
# Build the -lutil library with these extra functions.
extra-libs := libutil
new file mode 100644
@@ -0,0 +1,138 @@
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+static int
+test_ok (int fd)
+{
+ int ret, err;
+ char buf[512];
+
+ ret = ptsname_r (fd, buf, sizeof (buf));
+ err = errno;
+
+ if (ret != 0)
+ {
+ printf ("ptsname_r(): expected: return = %d\n", 0);
+ printf (" got: return = %d, errno = %d (%s)\n",
+ ret, err, strerror (err));
+ return 1;
+ }
+
+ return 0;
+}
+
+static int
+test_einval (int fd)
+{
+ int ret, err;
+ char *buf = NULL;
+
+ ret = ptsname_r (fd, buf, 1);
+ err = errno;
+
+ if (ret == 0 || errno != EINVAL)
+ {
+ printf ("ptsname_r(): expected: return = %d, errno = %d\n", -1, EINVAL);
+ printf (" got: return = %d, errno = %d (%s)\n",
+ ret, err, strerror (err));
+ return 1;
+ }
+
+ return 0;
+}
+
+static int
+test_erange (int fd)
+{
+ int ret, err;
+ char buf[512];
+
+ /* We assume that the PTS name will always be more than one character. */
+ ret = ptsname_r (fd, buf, 1);
+ err = errno;
+
+ if (ret == 0 || errno != ERANGE)
+ {
+ printf ("ptsname_r(): expected: return = %d, errno = %d\n", -1, ERANGE);
+ printf (" got: return = %d, errno = %d (%s)\n",
+ ret, err, strerror (err));
+ return 1;
+ }
+
+ return 0;
+}
+
+static int
+test_enotty (void)
+{
+ int fd, ret, err;
+ const char file[] = "./ptsname-einval";
+ char buf[512];
+
+ /* First try with a normal file. */
+ fd = open (file, O_RDWR | O_CREAT, 0600);
+ if (fd == -1)
+ {
+ printf ("open(\"%s\", O_RDWR) failed\nerrno %d (%s)\n",
+ file, errno, strerror (errno));
+ return 0;
+ }
+ unlink (file);
+
+ ret = ptsname_r (fd, buf, sizeof (buf));
+ err = errno;
+ close (fd);
+
+ if (ret == 0 || errno != ENOTTY)
+ {
+ printf ("ptsname_r(): expected: return = %d, errno = %d\n", -1, ENOTTY);
+ printf (" got: return = %d, errno = %d (%s)\n",
+ ret, err, strerror (err));
+ return 1;
+ }
+
+ /* Then try with a terminal device which is not a master. */
+ ret = ptsname_r (0, buf, sizeof (buf));
+ err = errno;
+
+ if (ret == 0 || errno != ENOTTY)
+ {
+ printf ("ptsname_r(): expected: return = %d, errno = %d\n", -1, ENOTTY);
+ printf (" got: return = %d, errno = %d (%s)\n",
+ ret, err, strerror (err));
+ return 1;
+ }
+
+ return 0;
+}
+
+int
+main (void)
+{
+ int result = 0;
+ int fd;
+
+ fd = posix_openpt (O_RDWR);
+ if (fd != -1)
+ {
+ result |= test_ok (fd);
+ result |= test_einval (fd);
+ result |= test_erange (fd);
+ close(fd);
+ }
+ else
+ {
+ printf ("posix_openpt(O_RDWR) failed\nerrno %d (%s)\n",
+ errno, strerror (errno));
+ /* We don't fail because of this; maybe the system does not have
+ SUS pseudo terminals. */
+ }
+
+ result |= test_enotty ();
+
+ return result;
+}
@@ -105,7 +105,9 @@ __ptsname_internal (int fd, char *buf, size_t buflen, struct stat64 *stp)
memcpy (__stpcpy (buf, devpts), p, &numbuf[sizeof (numbuf)] - p);
}
- else if (errno == EINVAL)
+ else if (errno != EINVAL)
+ return errno;
+ else
#endif
{
char *p;