Skip to content

Fix signedness mismatches in the syscall layer #19631

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

Merged
merged 3 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/library_sigs.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,9 @@ sigs = {
_gmtime_js__sig: 'vpp',
_localtime_js__sig: 'vpp',
_mktime_js__sig: 'ip',
_mmap_js__sig: 'ipiiippp',
_msync_js__sig: 'ippiiip',
_munmap_js__sig: 'ippiiip',
_mmap_js__sig: 'ipiiijpp',
_msync_js__sig: 'ippiiij',
_munmap_js__sig: 'ippiiij',
_setitimer_js__sig: 'iid',
_timegm_js__sig: 'ip',
_tzset_js__sig: 'vppp',
Expand Down
18 changes: 11 additions & 7 deletions src/library_syscall.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,12 @@ var SyscallsLibrary = {
'$mmapAlloc',
'emscripten_builtin_memalign',
#endif
],
_mmap_js: function(len, prot, flags, fd, off, allocated, addr) {
].concat(i53ConversionDeps),
_mmap_js: function(len, prot, flags, fd, {{{ defineI64Param('offset') }}}, allocated, addr) {
{{{ receiveI64ParamAsI53('offset', -cDefs.EOVERFLOW) }}}
#if FILESYSTEM && SYSCALLS_REQUIRE_FILESYSTEM
var stream = SYSCALLS.getStreamFromFD(fd);
var res = FS.mmap(stream, len, off, prot, flags);
var res = FS.mmap(stream, len, offset, prot, flags);
var ptr = res.ptr;
{{{ makeSetValue('allocated', 0, 'res.allocated', 'i32') }}};
#if CAN_ADDRESS_2GB
Expand All @@ -154,8 +155,9 @@ var SyscallsLibrary = {
#if FILESYSTEM && SYSCALLS_REQUIRE_FILESYSTEM
'$FS',
#endif
],
_munmap_js: function(addr, len, prot, flags, fd, offset) {
].concat(i53ConversionDeps),
_munmap_js: function(addr, len, prot, flags, fd, {{{ defineI64Param('offset') }}}) {
{{{ receiveI64ParamAsI53('offset', -cDefs.EOVERFLOW) }}}
#if FILESYSTEM && SYSCALLS_REQUIRE_FILESYSTEM
var stream = SYSCALLS.getStreamFromFD(fd);
if (prot & {{{ cDefs.PROT_WRITE }}}) {
Expand Down Expand Up @@ -623,8 +625,10 @@ var SyscallsLibrary = {

return total;
},
_msync_js: function(addr, len, prot, flags, fd, offset) {
SYSCALLS.doMsync(addr, SYSCALLS.getStreamFromFD(fd), len, flags, 0);
Copy link
Collaborator Author

@kleisauke kleisauke Jun 15, 2023

Choose a reason for hiding this comment

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

I'm not sure why offset was previously ignored here.

_msync_js__deps: i53ConversionDeps,
_msync_js: function(addr, len, prot, flags, fd, {{{ defineI64Param('offset') }}}) {
{{{ receiveI64ParamAsI53('offset', -cDefs.EOVERFLOW) }}}
SYSCALLS.doMsync(addr, SYSCALLS.getStreamFromFD(fd), len, flags, offset);
return 0;
},
__syscall_fdatasync: function(fd) {
Expand Down
6 changes: 3 additions & 3 deletions system/lib/libc/emscripten_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ int _mmap_js(size_t length,
int prot,
int flags,
int fd,
size_t offset,
off_t offset,
int* allocated,
void** addr);
int _munmap_js(
intptr_t addr, size_t length, int prot, int flags, int fd, size_t offset);
intptr_t addr, size_t length, int prot, int flags, int fd, off_t offset);
int _msync_js(
intptr_t addr, size_t length, int prot, int flags, int fd, size_t offset);
intptr_t addr, size_t length, int prot, int flags, int fd, off_t offset);

struct dso;

Expand Down
8 changes: 4 additions & 4 deletions system/lib/libc/emscripten_mmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,13 @@ int __syscall_msync(intptr_t addr, size_t len, int flags) {
return _msync_js(addr, len, map->prot, map->flags, map->fd, map->offset);
}

intptr_t __syscall_mmap2(intptr_t addr, size_t len, int prot, int flags, int fd, size_t off) {
intptr_t __syscall_mmap2(intptr_t addr, size_t len, int prot, int flags, int fd, off_t offset) {
if (addr != 0) {
// We don't currently support location hints for the address of the mapping
return -EINVAL;
}

off *= SYSCALL_MMAP2_UNIT;
offset *= SYSCALL_MMAP2_UNIT;
struct map* new_map;

// MAP_ANONYMOUS (aka MAP_ANON) isn't actually defined by POSIX spec,
Expand All @@ -132,7 +132,7 @@ intptr_t __syscall_mmap2(intptr_t addr, size_t len, int prot, int flags, int fd,
} else {
new_map = emscripten_builtin_malloc(sizeof(struct map));
int rtn =
_mmap_js(len, prot, flags, fd, off, &new_map->allocated, &new_map->addr);
_mmap_js(len, prot, flags, fd, offset, &new_map->allocated, &new_map->addr);
if (rtn < 0) {
emscripten_builtin_free(new_map);
return rtn;
Expand All @@ -142,7 +142,7 @@ intptr_t __syscall_mmap2(intptr_t addr, size_t len, int prot, int flags, int fd,

new_map->length = len;
new_map->flags = flags;
new_map->offset = off;
new_map->offset = offset;
new_map->prot = prot;

LOCK(lock);
Expand Down
12 changes: 6 additions & 6 deletions system/lib/libc/musl/arch/emscripten/syscall_arch.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#include <sys/types.h>
#include <wasi/api.h>
#include <wasi/wasi-helpers.h>
#include <emscripten/em_macros.h>

// Compile as if we can pass uint64 values directly to the
// host. Binaryen will take care of splitting any i64 params
Expand Down Expand Up @@ -55,9 +55,9 @@ int __syscall_mremap(intptr_t old_addr, size_t old_size, size_t new_size, int fl
int __syscall_poll(intptr_t fds, int nfds, int timeout);
int __syscall_getcwd(intptr_t buf, size_t size);
int __syscall_ugetrlimit(int resource, intptr_t rlim);
intptr_t __syscall_mmap2(intptr_t addr, size_t len, int prot, int flags, int fd, size_t off);
int __syscall_truncate64(intptr_t path, uint64_t length);
int __syscall_ftruncate64(int fd, uint64_t length);
intptr_t __syscall_mmap2(intptr_t addr, size_t len, int prot, int flags, int fd, off_t offset);
int __syscall_truncate64(intptr_t path, off_t length);
int __syscall_ftruncate64(int fd, off_t length);
int __syscall_stat64(intptr_t path, intptr_t buf);
int __syscall_lstat64(intptr_t path, intptr_t buf);
int __syscall_fstat64(int fd, intptr_t buf);
Expand All @@ -81,7 +81,7 @@ int __syscall_getdents64(int fd, intptr_t dirp, size_t count);
int __syscall_fcntl64(int fd, int cmd, ...);
int __syscall_statfs64(intptr_t path, size_t size, intptr_t buf);
int __syscall_fstatfs64(int fd, size_t size, intptr_t buf);
int __syscall_fadvise64(int fd, uint64_t offset, uint64_t length, int advice);
int __syscall_fadvise64(int fd, off_t offset, off_t length, int advice);
int __syscall_openat(int dirfd, intptr_t path, int flags, ...); // mode is optional
int __syscall_mkdirat(int dirfd, intptr_t path, int mode);
int __syscall_mknodat(int dirfd, intptr_t path, int mode, int dev);
Expand All @@ -96,7 +96,7 @@ int __syscall_fchmodat(int dirfd, intptr_t path, int mode, ...);
int __syscall_faccessat(int dirfd, intptr_t path, int amode, int flags);
int __syscall_pselect6(int nfds, intptr_t readfds, intptr_t writefds, intptr_t exceptfds, intptr_t timeout, intptr_t sigmaks);
int __syscall_utimensat(int dirfd, intptr_t path, intptr_t times, int flags);
int __syscall_fallocate(int fd, int mode, int64_t off, int64_t len);
int __syscall_fallocate(int fd, int mode, off_t offset, off_t len);
int __syscall_dup3(int fd, int suggestfd, int flags);
int __syscall_pipe2(intptr_t fds, int flags);
int __syscall_recvmmsg(int sockfd, intptr_t msgvec, size_t vlen, int flags, ...);
Expand Down
4 changes: 2 additions & 2 deletions system/lib/standalone/standalone.c
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,14 @@ weak int _mmap_js(size_t length,
int prot,
int flags,
int fd,
size_t offset,
off_t offset,
int* allocated,
void** addr) {
return -ENOSYS;
}

weak int _munmap_js(
intptr_t addr, size_t length, int prot, int flags, int fd, size_t offset) {
intptr_t addr, size_t length, int prot, int flags, int fd, off_t offset) {
return -ENOSYS;
}

Expand Down
4 changes: 2 additions & 2 deletions system/lib/wasmfs/js_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ int _wasmfs_open(char* path, int flags, mode_t mode) {
return __syscall_openat(AT_FDCWD, (intptr_t)path, flags, mode);
}

int _wasmfs_allocate(int fd, int64_t off, int64_t len) {
return __syscall_fallocate(fd, 0, off, len);
int _wasmfs_allocate(int fd, off_t offset, off_t len) {
return __syscall_fallocate(fd, 0, offset, len);
}

int _wasmfs_mknod(char* path, mode_t mode, dev_t dev) {
Expand Down
18 changes: 9 additions & 9 deletions system/lib/wasmfs/syscalls.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1226,15 +1226,15 @@ static int doTruncate(std::shared_ptr<File>& file, off_t size) {
return ret;
}

int __syscall_truncate64(intptr_t path, uint64_t size) {
int __syscall_truncate64(intptr_t path, off_t size) {
auto parsed = path::parseFile((char*)path);
if (auto err = parsed.getError()) {
return err;
}
return doTruncate(parsed.getFile(), size);
}

int __syscall_ftruncate64(int fd, uint64_t size) {
int __syscall_ftruncate64(int fd, off_t size) {
auto openFile = wasmFS.getFileTable().locked().getEntry(fd);
if (!openFile) {
return -EBADF;
Expand Down Expand Up @@ -1358,7 +1358,7 @@ int __syscall_poll(intptr_t fds_, int nfds, int timeout) {
return nonzero;
}

int __syscall_fallocate(int fd, int mode, int64_t off, int64_t len) {
int __syscall_fallocate(int fd, int mode, off_t offset, off_t len) {
assert(mode == 0); // TODO, but other modes were never supported in the old FS

auto fileTable = wasmFS.getFileTable().locked();
Expand All @@ -1378,14 +1378,14 @@ int __syscall_fallocate(int fd, int mode, int64_t off, int64_t len) {
return -EBADF;
}

if (off < 0 || len <= 0) {
if (offset < 0 || len <= 0) {
return -EINVAL;
}

// TODO: We could only fill zeros for regions that were completely unused
// before, which for a backend with sparse data storage could make a
// difference. For that we'd need a new backend API.
auto newNeededSize = off + len;
auto newNeededSize = offset + len;
off_t size = locked.getSize();
if (size < 0) {
return size;
Expand Down Expand Up @@ -1534,7 +1534,7 @@ int _mmap_js(size_t length,
int prot,
int flags,
int fd,
size_t offset,
off_t offset,
int* allocated,
void** addr) {
// PROT_EXEC is not supported (although we pretend to support the absence of
Expand Down Expand Up @@ -1621,7 +1621,7 @@ int _mmap_js(size_t length,
}

int _msync_js(
intptr_t addr, size_t length, int prot, int flags, int fd, size_t offset) {
intptr_t addr, size_t length, int prot, int flags, int fd, off_t offset) {
// TODO: This is not correct! Mappings should be associated with files, not
// fds. Only need to sync if shared and writes are allowed.
int mapType = flags & MAP_TYPE;
Expand All @@ -1637,7 +1637,7 @@ int _msync_js(
}

int _munmap_js(
intptr_t addr, size_t length, int prot, int flags, int fd, size_t offset) {
intptr_t addr, size_t length, int prot, int flags, int fd, off_t offset) {
// TODO: This is not correct! Mappings should be associated with files, not
// fds.
// TODO: Syncing should probably be handled in __syscall_munmap instead.
Expand Down Expand Up @@ -1718,7 +1718,7 @@ int __syscall_recvmsg(
return -ENOSYS;
}

int __syscall_fadvise64(int fd, uint64_t offset, uint64_t length, int advice) {
int __syscall_fadvise64(int fd, off_t offset, off_t length, int advice) {
// Advice is currently ignored. TODO some backends might use it
return 0;
}
Expand Down