Bug 12685

Summary: fopen doesn't honor last byte of valid modes
Product: glibc Reporter: Eric Blake <eblake>
Component: stdioAssignee: Not yet assigned to anyone <unassigned>
Status: NEW ---    
Severity: normal CC: bugdal, mtk.manpages, ondra, ppluzhnikov
Priority: P2 Flags: fweimer: security-
Version: 2.13   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:

Description Eric Blake 2011-04-20 00:46:39 UTC
$ cat foo.c
#include <stdio.h>
#include <fcntl.h>
#include <stdlib.h>

int main (void)
{
  FILE *f = fopen ("/dev/null", "rb+cmxe");
  if (!f) exit (1);
  int fd = fileno (f);
  if (fd < 0) exit (2);
  int mode = fcntl (fd, F_GETFD);
  if (mode < 0) exit (3);
  return !(mode & FD_CLOEXEC);
}
$ ./foo; echo $?
1

Oops - an off-by-one error in fileops.c ended up ignoring 'e'.  The same goes for any other valid byte that doesn't appear until mode[6].

diff --git i/libio/fileops.c w/libio/fileops.c
index 4698953..ee6ce13 100644
--- i/libio/fileops.c
+++ w/libio/fileops.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 1993, 1995, 1997-2005, 2006, 2007, 2008, 2009
+/* Copyright (C) 1993, 1995, 1997-2005, 2006, 2007, 2008, 2009, 2011
    Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Written by Per Bothner <bothner@cygnus.com>.
@@ -290,7 +290,7 @@ _IO_new_file_fopen (fp, filename, mode, is32not64)
 #ifdef _LIBC
   last_recognized = mode;
 #endif
-  for (i = 1; i < 6; ++i)
+  for (i = 1; i < 7; ++i)
     {
       switch (*++mode)
 	{
Comment 1 Ulrich Drepper 2011-04-23 03:41:46 UTC
The patch didn't apply.  I did it manually.
Comment 2 Michael Kerrisk 2012-04-22 01:58:14 UTC
Actually, is there any reason at all to limit the length of the string? Whay not parse until NUL? Given that "mode" may be a constructed string, or that other flags may be added in the future, it seems that arbitrarily limiting the length of the string to some number of bytes is just setting up both glibc and userspace bugs for the future.
Comment 3 Michael Kerrisk 2012-04-22 02:01:46 UTC
A similar remark applies for fdopen(), which uses different code for parsing 'mode'.
Comment 4 Michael Kerrisk 2012-04-22 02:11:48 UTC
Sorry, my original comment should have read:

Actually, is there any reason at all to limit the length of the string? Why
not parse until NUL *or comma*? Given that "mode" may be a constructed string, or that other flags may be added in the future, it seems that arbitrarily limiting the length of the string to some number of bytes is just setting up both glibc and userspace bugs for the future.
Comment 5 OndrejBilka 2013-05-09 15:13:53 UTC
Could you specify why do you want comma as terminator?
Comment 6 Rich Felker 2013-05-09 16:08:22 UTC
I believe it's to ignore some non-standard syntax certain implementations use for binding a non-default character encoding at open time. I recall seeing such a usage before but I can't think where.