Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

munmap fails if file has been closed #20459

Open
joemarshall opened this issue Oct 16, 2023 · 4 comments
Open

munmap fails if file has been closed #20459

joemarshall opened this issue Oct 16, 2023 · 4 comments

Comments

@joemarshall
Copy link
Contributor

In linux, you can do:

  1. open file
  2. memory map a region of the file (mmap)
  3. close file
  4. do stuff with the memory mapped region
  5. munmap region

In emscripten, it mostly works, but munmap returns -1 if the file is closed.

test.c

#include <sys/mman.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdio.h>

int main()
{
    int fd;
    char *addr;
    int result;

    fd=open("test-unmap.c",O_RDONLY);
    addr=mmap(NULL,64,PROT_READ,MAP_SHARED,fd,0);
    close(fd);
    // this call should succeed, but actually fails
    result=munmap(addr,64);
    printf("%d\n",result);
}

To reproduce:
emcc test-unmap.c -lnodefs.js -lnoderawfs.js

@tlively
Copy link
Member

tlively commented Oct 16, 2023

Yep, our current implementation of mmap is completely incorrect. Failing when the mapped file is closed is not even the worst thing that can happen. If the mapped file is closed and another file is subsequently opened and assigned the same file descriptor, then munmap will write the original file's contents to the other file!

Related issues: #15140, #17801

@joemarshall
Copy link
Contributor Author

joemarshall commented Oct 18, 2023

Looking at this, it feels like the easiest way to fix would be:

  1. Make mmap_js save a map from mapped memory addresses -> FSNodes (or pointers to DataFile in the wasmfs version)

  2. Make msync_js and munmap_js look up the file node from the memory map instead of using the file descriptor.

Does that make sense? Is there anything I'm missing? It looks to me like file deletion is likely to break things?

@tlively
Copy link
Member

tlively commented Oct 18, 2023

Yes, that sounds like the correct direction. In addition, it would be nice to make the file systems aware of the mapping so that the read and write syscalls could access the mapped buffer so their view of the data would be consistent with directly accessing the buffer.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 12, 2023

@joemarshall is there some reason you need / want to depend on the fake mmap support in emscripten? For most programs its better to build them as if mmap is not available. This should reduce code size and increase match between what the program wants to do and what emscirpten can actually provide.

Do you have the ability to build with something like HAVE_MMAP=0 in your codebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants