mikaku / Fiwix

A UNIX-like kernel for the i386 architecture
https://www.fiwix.org
Other
407 stars 32 forks source link

Memory mapped files not written to disk correctly #18

Closed rick-masters closed 1 year ago

rick-masters commented 1 year ago

The contents of memory mapped files created using mmap are not written correctly.

The following test program (adapted from https://stackoverflow.com/questions/26259421/use-mmap-in-c-to-write-into-memory) can reproduce the problem:

#include <stdlib.h>
#include <unistd.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/mman.h> /* mmap() is defined in this header */
#include <fcntl.h>
#include <string.h>

void err_quit(char *msg)
{
    puts(msg);
    exit(1);
}
int main (int argc, char *argv[])
{
 int fdin, fdout;
 char *src, *dst;
 struct stat statbuf;
 int mode = 0x0777;

 if (argc != 3)
   err_quit ("usage: a.out <fromfile> <tofile>");

 /* open the input file */
 if ((fdin = open (argv[1], O_RDONLY)) < 0)
   {printf("can't open %s for reading", argv[1]);
    return 0;
   }

 /* open/create the output file */
 if ((fdout = open (argv[2], O_RDWR | O_CREAT | O_TRUNC, mode )) < 0)//edited here
   {printf ("can't create %s for writing", argv[2]);
    return 0;
   }

 /* find size of input file */
 if (fstat (fdin,&statbuf) < 0)
   {printf ("fstat error");
    return 0;
   }

 /* go to the location corresponding to the last byte */
 if (lseek (fdout, statbuf.st_size - 1, SEEK_SET) == -1)
   {printf ("lseek error");
    return 0;
   }

 /* write a dummy byte at the last location */
 if (write (fdout, "", 1) != 1)
   {printf ("write error");
     return 0;
   }

 /* mmap the input file */
 if ((src = mmap (0, statbuf.st_size, PROT_READ, MAP_SHARED, fdin, 0))
   == (caddr_t) -1)
   {printf ("mmap error for input");
    return 0;
   }

 /* mmap the output file */
 if ((dst = mmap (0, statbuf.st_size, PROT_READ | PROT_WRITE,
   MAP_SHARED, fdout, 0)) == (caddr_t) -1)
   {printf ("mmap error for output");
    return 0;
   }

 /* this copies the input file to the output file */
 memcpy (dst, src, statbuf.st_size);
 return 0;

} /* main */

The following commands will run the test.

cc mmaptest.c -o mmaptest
dd if=/dev/zero of=testin bs=1 count=10037
./mmaptest testin testout
diff testin testout

The result should be that testin and testout are the same. Instead testout is mostly filled with "random" data.

The root cause of the problem is this line of code: https://github.com/mikaku/Fiwix/blob/abc14a046e017331894c865beaa0977ec6b29e58/mm/mmap.c#L288

The code above writes the full length of the file from each and every page. So, even though a page is at most 4096 bytes, the write_page routine is trying to copy data from each page far past the end of the page boundary. This puts random data into the file.

A PR with a suggested fix is forthcoming.

mikaku commented 1 year ago

You found another bug! I've completed a new massive package build with your patch and I haven't experienced any regression, good. Thank you very much!