fix wrong out of memory in edge cases, just try allocate from block for one source of truth

[laytan]

Follow up on #4835

Fixes #4834

Also fixes an already existing bug where on NetBSD and FreeBSD we are doing mmap with no protection flags set, on these targets we need to specify a special "maximum" protection value in order to properly mprotect with more permissions.

[laytan]

Not sure why netbsd and freebsd are segfaulting

[Feoramund]

I can reproduce the NetBSD segfault. However, when I hook a debugger up to it (lldb or gdb), the segfault ceases to happen, making this harder to pin down.

[Feoramund]

Looks like it's segfaulting on this line in platform_memory_alloc:

Odin/core/mem/virtual/virtual_platform.odin

Line 21 in f0b1357

block.committed = to_commit

The value of block looks like a valid pointer at the time, as in not zero and evenly divisible by 4096, but accessing it causes a segfault as if the pointer was bogus.

Digging further, it looks like posix.mprotect is returning a value != .OK and _commit returns .Out_of_Memory, but we don't check for that (this should probably be changed and panic accordingly or propagate the error up). This is starting to make more sense. The pointer's valid, but its protection hasn't been set correctly yet.

Checking errno, I see that the actual error is 13 which should be EACCES, and referencing mprotect on NetBSD 10.0's manpages:

ERRORS
     [EACCES]           A memory protection violation occurred.

                        The PROT_EXEC flag was attempted on pages which belong
                        to a file system mounted with the NOEXEC flag.

                        The new protection is less restrictive than the
                        protection originally set with mmap(2).

                        PaX mprotect restrictions prohibit the requested
                        protection.

After seeing that we use mmap with no protection flags in _reserve, I suspect it's due to the protection being less restrictive, and in fact, when I change it to be { .READ, .WRITE } by default, it works.

[laytan]

@Feoramund thanks so much for looking into this!

Digging further, it looks like posix.mprotect is returning a value != .OK and _commit returns .Out_of_Memory, but we don't check for that (this should probably be changed and panic accordingly or propagate the error up). This is starting to make more sense. The pointer's valid, but its protection hasn't been set correctly yet.

I think the fact it is actually returning .Out_Of_Memory is "fine", especially if we now fix the misuse here. We could check ENOMEM specifically though, and panic otherwise 🤔

After seeing that we use mmap with no protection flags in _reserve, I suspect it's due to the protection being less restrictive, and in fact, when I change it to be { .READ, .WRITE } by default, it works.

I've looked at the manpages too, and I am thinking we may need to do this for NetBSD, to allow any protection later being enabled with mprotect? (I don't think we can just give the mmap call .Read, .Write because it can also be used by others and they may want executable too, even though commit only needs read and write.)

As a NetBSD extension, the PROT_MPROTECT macro can be used to request
additional permissions for later use with mprotect(2). For example
PROT_MPROTECT(PROT_READ) requests that future PROT_READ mappings are
allowed and can be enabled using mprotect(2), but does not currently
grant read mappings to the returned memory segment.

FreeBSD seems to have a similar thing (although that one doesn't fail the CI 🤔 ):

In addition to these protection flags, FreeBSD provides the ability to
set the maximum protection of a region allocated by mmap and later al-
tered by mprotect(2). This is accomplished by or'ing one or more PROT_
values wrapped in the PROT_MAX() macro into the prot argument.

[Feoramund]

Interesting extensions. That looks like the real fix here then, to use that for both FreeBSD (just in case) and NetBSD.

I mentioned raising a panic on commit in platform_memory_alloc because at the very least having an assert to make sure that it's returning nil would help track down similar bugs in the future.

[laytan]

@Feoramund Thanks again, the changes seem to work. This is ready to merge.