Bug 6887 - mmap documentation bugs, perhaps also 2 functional bugs
Summary: mmap documentation bugs, perhaps also 2 functional bugs
Status: RESOLVED INVALID
Alias: None
Product: glibc
Classification: Unclassified
Component: manual (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-15 08:26 UTC by Siward de Groot
Modified: 2016-08-12 20:05 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Siward de Groot 2008-09-15 08:26:14 UTC
Good morning everyone,
i just tested mmap() &co ,
and found a few bugs, mainly in documentation,
  but i think two of them are also functional bugs.

* opening file rw and mmapping it w
  causes segfault at first write to a mapped memory location.
  (functional bug, therefore documentation bug until it is fixed).

* opening file w and mmapping it w causes mmap error 'permission denied',
  which is not mentioned in documentation
  (rather it is said that i may GET more permissions than i asked for).

* trying to read from an empty file generates a 'bus error',
  terminating process,
  which is not mentioned in documentation.

* compiler didn't find O_READ ,
  though fcntl.h was #included,
  and both __USE_GNU and _GNU_SOURCE were #defined.
  (working around by using O_RDONLY is trivial, but it is a documentation bug)

* when mmapping an empty file, mmap() returns a valid address,
  but reading from this address causes a segfault.
  (functional bug, therefore documentation bug until fixed).

* description of mremap says that it returns -1 on error,
  but this is not possible since it's returntype is void* .

* description of MS_ASYNC says
  "This tells `msync' to begin synchronization, but not to wait for it to complete."
   which should be
   "This tells `msync' to begin synchronization, but to not wait for it to
complete."
   because it is confusing.
   (never mind whether it is official english ; it is braindamage.)

* description of MAP_SHARED says
  "This specifies that writes to region will be written back to file."
   but it is valid (or at least it works) to use MAP_SHARED with MAP_ANON,
   in which case there is no file to write back to.
   Perhaps in this case MAP_SHARED controls whether other processes see changed
content ?

For the rest, mmap, mremap, munmap, and msync seem to work perfectly.
Thanks !
Have a nice day :-)

  Siward
Comment 1 Siward de Groot 2008-09-15 08:35:44 UTC
In case it is helpfull, here is code i used for testing :
(macros ex(), esx(), and t() are simple error-reporting thingies)

/* test files :
  /tmp/test  : permissions 0770 : content "#!/bin/bash\necho hello\nexit\n"
  /tmp/empty : permissions 0770 : no content
  /tmp/small : permissions 0770 : initially 10 bytes random content
 */

/* compile with :
 cd /my/test/c
 cc mmap2.c -o mmap2 -I /my/c/include -W -Wall
 */

#include "defines.c"
#include <stdio.h>
#include <sys/mman.h>		 /* mmap()	 */
#include <unistd.h>		 /* sysconf()	 */
#include <fcntl.h>		 /* open()	 */
#include <sys/stat.h>		 /* stat()	 */
#include <errno.h>		 /* ENOENT	 */
#include <string.h>		 /* strcpy()	 */
#include "errordef.c"

char*		 filename	 ;
int		 ck		 ;
struct stat	 stats		 ;
size_t		 filesize	 ; /* unsigned ! */
int		 fd		 ; /* file descriptor */
size_t		 pagesize	 ; /* unsigned ! */
void*		 adres		 ;
size_t		 mappedsize	 ; /* unsigned ! */


void TestMmapFile( void ){
  char* pc ;

  filename = "/tmp/test" ;
  ck = stat( filename, &stats );
  if( ck ){ esx("stat nok"); }
  filesize = stats.st_size ;
  t("filesize = %d", filesize );

  fd = open( filename, O_RDWR );
  /* bug: O_READ not found, though fcntl.h is included */
  if( fd < 0 ){ esx("open nok"); }
  t("open ok");

  pagesize = sysconf( _SC_PAGESIZE ) ;
  if( pagesize <= 0 ){ esx("sysconf nok"); }
  t("pagesize = %d", pagesize );
  if( pagesize < filesize ){ ex("pagesize is smaller than filesize"); }

  adres = mmap( NULL, pagesize, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0 );
  /* what if mmap permissions don't match permissions with which file was opened
? */
  /*   open rw , mmap rw , action w -> ok      		 */
  /*   open rw , mmap r  , action w -> segfault		 */ /* pebcak	 */
  /*   open rw , mmap w  , action w -> segfault		 */ /* BUG !	 */
  /*   open r  , mmap r  , action w -> segfault		 */ /* pebcak	 */
  /*   open r  , mmap w             -> permission denied */
  /*   open w  , mmap r             -> permission denied */
  /*   open w  , mmap w             -> permission denied */ /* doc bug	 */

  if( adres == MAP_FAILED ){ esx("mmap nok"); }
  t("mmap ok");
  mappedsize = pagesize ;

  pc = (char*) adres ; pc[21]++ ;	 /* changes 'echo hello' to 'echo hellp' */
  t("change content ok");

  ck = munmap( adres, mappedsize );
  /* what if i munmap filesize ? -> ok */
  if( ck < 0 ){ esx("munmap nok"); }
  t("munmap ok");

  ck = close( fd );
  if( ck < 0 ){ esx("close nok"); }
  t("close ok");
  }

void TestMmapEmptyfile( void ){
  char	 c	 ;

  filename = "/tmp/empty" ;
  ck = stat( filename, &stats );
  if( ck ){ esx("stat nok"); }
  filesize = stats.st_size ;
  t("filesize = %d", filesize );

  fd = open( filename, O_RDWR );
  /* bug: O_READ not found, though fcntl.h is included */
  if( fd < 0 ){ esx("open nok"); }
  t("open ok");

  pagesize = sysconf( _SC_PAGESIZE ) ;
  if( pagesize <= 0 ){ esx("sysconf nok"); }
  t("pagesize = %d", pagesize );
  if( pagesize < filesize ){ ex("pagesize is smaller than filesize"); }

  adres = mmap( NULL, pagesize, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0 );
  if( adres == MAP_FAILED ){ esx("mmap nok"); }
  t("mmap ok ; adres = %p", adres );
  mappedsize = 0 ;

  c = *((char*) adres) ;
  t("read content ok");
  }

void TestMmapResize( void ){
  size_t newsize ;
  int	 i	 ;
  int	 imax	 ;
  char*	 pc	 ;

  filename = "/tmp/small" ;
  ck = stat( filename, &stats );
  if( ck ){ esx("stat nok"); }
  filesize = stats.st_size ;
  t("filesize = %d", filesize );

  fd = open( filename, O_RDWR );
  if( fd < 0 ){ esx("open nok"); }
  t("open ok");

  pagesize = sysconf( _SC_PAGESIZE ) ;
  if( pagesize <= 0 ){ esx("sysconf nok"); }
  t("pagesize = %d", pagesize );

  adres = mmap( NULL, pagesize, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0 );
  if( adres == MAP_FAILED ){ esx("mmap nok"); }
  t("mmap ok ; adres = %p", adres );
  mappedsize = pagesize ;

  newsize = 2 * pagesize ;
  ck = ftruncate( fd, newsize );
  if( ck < 0 ){ esx("ftruncate nok"); }
  t("ftruncate ok");

  adres = mremap( adres, mappedsize, newsize, MREMAP_MAYMOVE );
  t("mremap ok ; new adres = %p", adres );
  mappedsize = newsize ;

  imax = pagesize + pagesize / 2 ;
  pc = (char*) adres ;
  for( i=0 ; i<imax ; i++ ){ pc[i] = 'a' ; }
  t("write ok ; imax = %d", imax );

  ck = ftruncate( fd, imax );
  if( ck < 0 ){ esx("ftruncate nok"); }
  t("ftruncate ok");

  ck = msync( adres, imax, 0 );
  /* tested with all 3 possible values of flags ; all work ok */
  if( ck < 0 ){ esx("msync nok"); }
  t("msync ok");

  ck = munmap( adres, mappedsize );
  if( ck < 0 ){ esx("munmap nok"); }
  t("munmap ok");

  ck = close( fd );
  if( ck < 0 ){ esx("close nok"); }
  t("close ok");
  }

void TestMmapAnon( void ){
  int i ;
  int* pi ;

  pagesize = sysconf( _SC_PAGESIZE ) ;
  if( pagesize <= 0 ){ esx("sysconf nok"); }
  t("pagesize = %d", pagesize );

  adres = mmap( NULL, pagesize, PROT_READ|PROT_WRITE, MAP_ANON, 0, 0 );
  /* with flags MAP_PRIVATE | MAP_ANON : ok  */
  /* with flags MAP_SHARED  | MAP_ANON : ok  */
  /* with flags MAP_ANON               : nok */

  if( adres == MAP_FAILED ){ esx("mmap nok"); }
  t("mmap ok ; adres = %p", adres );
  mappedsize = pagesize ;
  pi = (int*) adres ;
  for( i=0 ; i < 100 ; i++ ){ pi[i] = i ; }
  t("assign ok");
  for( i=0 ; i < 100 ; i++ ){ if( pi[i] != i ){ ex("pi[%d] = %d",i,pi[i] ); } }
  t("readback ok");
  }

int main( void ){
  TestMmapAnon();
  return(0);
  }
Comment 2 Siward de Groot 2008-09-15 08:52:16 UTC
Oops,
3rd and 5th items are both about same thing,
and neither of them is completely correct.
Please replace these two with :

* when mmapping an empty file, mmap() returns a valid address,
  but reading from this address causes a 'bus error'.
  Bus error is not mentioned in documentation, and
  A valid address is returned despite that there is no valid memorylocation.
Comment 3 Adhemerval Zanella 2016-08-11 14:40:08 UTC
I would suggest to next time when open bugs against kernel interfaces to specify which architecture and kernel version the issues were observed. The mmap interface has some arch-specific implementation and behaviours (for instance MAP_32BIT on x86).

About the questionings:

* opening file rw and mmapping it w
  causes segfault at first write to a mapped memory location.
  (functional bug, therefore documentation bug until it is fixed).

  I did not observed in my testing system (x86_64 4.4.0-31-generic) and if it is happening it is clearly a kernel issue.

* opening file w and mmapping it w causes mmap error 'permission denied',
  which is not mentioned in documentation
  (rather it is said that i may GET more permissions than i asked for).

  Again this is clearly a kernel issue if it is really happening.

* trying to read from an empty file generates a 'bus error',
  terminating process,
  which is not mentioned in documentation.

  This is a implementation detail which is covered by POSIX:

  "An implementation may generate SIGBUS signals when a reference would cause an error in the mapped object, such as out-of-space condition." 

  And it is currently covered by manual as:

"ERRORS

  [...]

  SIGBUS Attempted  access  to  a portion of the buffer that does not correspond to the file (for example, beyond the end of the file, including the case where another process has truncated the file)."

* when mmapping an empty file, mmap() returns a valid address,
  but reading from this address causes a segfault.
  (functional bug, therefore documentation bug until fixed).

  This is the same issue as before.

* compiler didn't find O_READ ,
  though fcntl.h was #included,
  and both __USE_GNU and _GNU_SOURCE were #defined.
  (working around by using O_RDONLY is trivial, but it is a documentation bug)

  O_READ is GNU-specific identifier only used on GNU/Hurd.  GLIBC on Linux only defines the POSIX name O_RDONLY.

* description of mremap says that it returns -1 on error,
  but this is not possible since it's returntype is void* .

  The manual states "On error, the value MAP_FAILED (that is, (void *) -1)".

* description of MS_ASYNC says
  "This tells `msync' to begin synchronization, but not to wait for it to complete."
   which should be
   "This tells `msync' to begin synchronization, but to not wait for it to
complete."
   because it is confusing.
   (never mind whether it is official english ; it is braindamage.)

  The text has changed and it does not contain this wording any more.

* description of MAP_SHARED says
  "This specifies that writes to region will be written back to file."
   but it is valid (or at least it works) to use MAP_SHARED with MAP_ANON,
   in which case there is no file to write back to.
   Perhaps in this case MAP_SHARED controls whether other processes see changed
content ?

   It currenty states that "Share  this mapping.  Updates to the mapping are visible to other processes that map this file, and are carried through to the underlying file.  (To precisely control when updates are carried through to the underlying file requires the use of msync(2).)"

So mostly of the mentioned issues are currently documented and some are just user errors. I will close this bug.
Comment 4 Michael Kerrisk 2016-08-12 19:17:08 UTC
(In reply to Adhemerval Zanella from comment #3)
> I would suggest to next time when open bugs against kernel interfaces to
> specify which architecture and kernel version the issues were observed. The
> mmap interface has some arch-specific implementation and behaviours (for
> instance MAP_32BIT on x86).
> 
> About the questionings:
> 
> * opening file rw and mmapping it w
>   causes segfault at first write to a mapped memory location.
>   (functional bug, therefore documentation bug until it is fixed).
> 
>   I did not observed in my testing system (x86_64 4.4.0-31-generic) and if
> it is happening it is clearly a kernel issue.

I also could not reproduce this.

> * opening file w and mmapping it w causes mmap error 'permission denied',
>   which is not mentioned in documentation
>   (rather it is said that i may GET more permissions than i asked for).
> 
>   Again this is clearly a kernel issue if it is really happening.

In the mmap(2) man page, there is mention of this case under the EACCES error

MAP_SHARED  was  requested and PROT_WRITE is set, but fd is
              not open in read/write (O_RDWR)  mode.
> 
> * trying to read from an empty file generates a 'bus error',
>   terminating process,
>   which is not mentioned in documentation.
> 
>   This is a implementation detail which is covered by POSIX:
> 
>   "An implementation may generate SIGBUS signals when a reference would
> cause an error in the mapped object, such as out-of-space condition." 
> 
>   And it is currently covered by manual as:

(Here, Adhemerval, you refer to the mmap(2) man page, but I believe the Siward was reporting documentation bugs against the glibc manual, which is rather less complete than the mmap(2) man page.

> "ERRORS
> 
>   [...]
> 
>   SIGBUS Attempted  access  to  a portion of the buffer that does not
> correspond to the file (for example, beyond the end of the file, including
> the case where another process has truncated the file)."
> 
> * when mmapping an empty file, mmap() returns a valid address,
>   but reading from this address causes a segfault.
>   (functional bug, therefore documentation bug until fixed).
> 
>   This is the same issue as before.
> 
> * compiler didn't find O_READ ,
>   though fcntl.h was #included,
>   and both __USE_GNU and _GNU_SOURCE were #defined.
>   (working around by using O_RDONLY is trivial, but it is a documentation
> bug)
> 
>   O_READ is GNU-specific identifier only used on GNU/Hurd.  GLIBC on Linux
> only defines the POSIX name O_RDONLY.
> 
> * description of mremap says that it returns -1 on error,
>   but this is not possible since it's returntype is void* .
> 
>   The manual states "On error, the value MAP_FAILED (that is, (void *) -1)".
> 
> * description of MS_ASYNC says
>   "This tells `msync' to begin synchronization, but not to wait for it to
> complete."
>    which should be
>    "This tells `msync' to begin synchronization, but to not wait for it to
> complete."
>    because it is confusing.
>    (never mind whether it is official english ; it is braindamage.)

Again, I think that Siward was reporting a bug against the glibc manual, which does indeed still contain the text:

     'MS_ASYNC'

          This tells 'msync' to begin the synchronization, but not to
          wait for it to complete.

>   The text has changed and it does not contain this wording any more.
> 
> * description of MAP_SHARED says
>   "This specifies that writes to region will be written back to file."
>    but it is valid (or at least it works) to use MAP_SHARED with MAP_ANON,
>    in which case there is no file to write back to.
>    Perhaps in this case MAP_SHARED controls whether other processes see
> changed
> content ?
> 
>    It currenty states that "Share  this mapping.  Updates to the mapping are
> visible to other processes that map this file, and are carried through to
> the underlying file.  (To precisely control when updates are carried through
> to the underlying file requires the use of msync(2).)"

So the text here could be a little more precise in mmap(2). I changed it to

       MAP_SHARED
              Share this mapping.  Updates to the mapping are visible  to
              other  processes  mapping the same region, and (in the case
              of file-backed mappings) are carried through to the  under‐
              lying file.

Cheers,

Michael

> 
> So mostly of the mentioned issues are currently documented and some are just
> user errors. I will close this bug.
Comment 5 Adhemerval Zanella 2016-08-12 19:30:43 UTC
Thanks for following on this.  Indeed I was referring to the man pages, should we sync glibc manual with man in this case?
Comment 6 Michael Kerrisk 2016-08-12 20:05:29 UTC
On 08/13/2016 07:30 AM, adhemerval.zanella at linaro dot org wrote:
> https://sourceware.org/bugzilla/show_bug.cgi?id=6887
>
> --- Comment #5 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
> Thanks for following on this.  Indeed I was referring to the man pages, should
> we sync glibc manual with man in this case?

That's up to you folks, really. I'm willing to release content I've created
for man-pages under some GNU-friendly license (but I don't have a signed
GNU CLA, and don't plan to obtain one).

Cheers,

Michael