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

incorrect usage of mmap return value in the standard library #2415

Closed
andrewrk opened this issue May 3, 2019 · 5 comments · Fixed by #2527
Closed

incorrect usage of mmap return value in the standard library #2415

andrewrk opened this issue May 3, 2019 · 5 comments · Fixed by #2527
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented May 3, 2019

../std/heap.zig:                if (addr == p.MAP_FAILED) return error.OutOfMemory;
../std/os.zig:    if (mmap_addr == posix.MAP_FAILED) return error.OutOfMemory;

These assume the libc API of mmap. However that is a libc thing. mmap returns an error in the normal syscall return code format and posix.getErrno() is needed to find out the error code.

Related: #2380

@andrewrk andrewrk added bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library. labels May 3, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone May 3, 2019
@pcsx22
Copy link

pcsx22 commented May 5, 2019

I will take this one. Just to be clear, the error returnType for these functions should be changed to generic errorType, right?

@pcsx22
Copy link

pcsx22 commented May 7, 2019

I introduced a error set (posixMmapError) which has all kinds of error to be thrown by mmap(). I merged posixMmapError error set with the already used error set in mem.zig->Allocator struct->reallocFn. Since, mem.allocator is used in tons of files(fmt.zig, child_process.zig), I'm merging posixMmapError set to the error set used in these files. Is this the correct approach ? @andrewrk

@andrewrk
Copy link
Member Author

andrewrk commented May 9, 2019

@pcsx22 it's hard to understand what you mean - can you open a draft PR to show what you mean?

@pcsx22
Copy link

pcsx22 commented May 10, 2019

@andrewrk Have a look at this commit: pcsx22@6885f71. I have put some comments regarding the code change; have a look at it

@andrewrk
Copy link
Member Author

coincidentally musl just fixed an equivalent bug:

commit 77846800722914eeba170505c2e7f89e12a6beff
Author: Ilya Matveychikov <[email protected]>
Date:   Sat Feb 9 18:56:17 2019 +0400

    fix the use of syscall result in dl_mmap

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 206427fe..7cb66db9 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -920,7 +920,7 @@ static void *dl_mmap(size_t n)
 #else
        p = (void *)__syscall(SYS_mmap, 0, n, prot, flags, -1, 0);
 #endif
-       return p == MAP_FAILED ? 0 : p;
+       return (unsigned long)p > -4096UL ? 0 : p;
 }
 
 static void makefuncdescs(struct dso *p)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants