Skip to content

Remove O_NOFOLLOW flag when mmap file in plugin#2353

Merged
sporksmith merged 5 commits intoshadow:mainfrom
Congyu-Liu:fix-mmap-nofollow
Aug 12, 2022
Merged

Remove O_NOFOLLOW flag when mmap file in plugin#2353
sporksmith merged 5 commits intoshadow:mainfrom
Congyu-Liu:fix-mmap-nofollow

Conversation

@Congyu-Liu
Copy link
Copy Markdown
Contributor

@Congyu-Liu Congyu-Liu commented Aug 11, 2022

When the plugin mmaps a file opened with O_NOFOLLOW, shadow cannot open the file for plugin and return error for mmap.

example.c:

#include <sys/mman.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdlib.h>
#include <unistd.h>
#define errExit(msg)    do { perror(msg); exit(EXIT_FAILURE); \
                                   } while (0)
int main() {
        int fd = open("file", O_CREAT|O_NOFOLLOW|O_RDWR, 0666);
        if (fd == -1) {
                errExit("open");
        }
        int size = 32;
        if (ftruncate(fd, size) == -1) {
                errExit("ftruncate");
        }
        void *addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
        if (addr == MAP_FAILED) {
                errExit("mmap");
        }
        return 0;
}

Running the above code will produce shadow log:

[memory_mapper.rs:499] [shadow_rs::host::memory_manager::memory_mapper] Handling mmap result for 7ffff7ffb000..+20
[managed_thread.c:238] [_managedthread_waitForNextEvent] received shim_event 3
[managed_thread.c:238] [_managedthread_waitForNextEvent] received shim_event 3
[mman.c:175] [_syscallhandler_openPluginFile] Failed to open path '/proc/2488142/fd/10' in plugin, error 40: Too many levels of symbolic links.

The error 40 (ELOOP) is caused by the O_NOFOLLOW flag, which prevents the plugin opening the path where the base name is a symbolic link. So reset O_NOFOLLOW since plugin is opening a symbolic link file in procfs. This flag is also unnecessary at this stage since it was already enforced when shadow opening the file.

@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Aug 11, 2022
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! We should add a regression test here, though.

LMK if you need more guidance, or if you'd rather not take that part on

@github-actions github-actions bot added the Component: Testing Unit and integration tests and frameworks label Aug 12, 2022
@Congyu-Liu
Copy link
Copy Markdown
Contributor Author

@sporksmith Please check.

Copy link
Copy Markdown
Contributor

@sporksmith sporksmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! Will merge shortly.

@sporksmith sporksmith enabled auto-merge (squash) August 12, 2022 15:02
@sporksmith sporksmith merged commit bd469e9 into shadow:main Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable Component: Testing Unit and integration tests and frameworks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants