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

os2.process_start uses the global allocator instead of context.allocator which can lead to a panic/failed assert #4265

Closed
gaultier opened this issue Sep 18, 2024 · 4 comments

Comments

@gaultier
Copy link

gaultier commented Sep 18, 2024

Context

  • Operating System & Odin Version: Linux, Fedora
  • Please paste odin report output:
Where to find more information and get into contact when you encounter a bug:

	Website: https://odin-lang.org
	GitHub:  https://github.com/odin-lang/Odin/issues


Useful information to add to a bug report:

	Odin:    dev-2024-09:8814170ed
	OS:      Fedora Linux 40 (Workstation Edition), Linux 6.10.7-200.fc40.x86_64
	CPU:     11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
	RAM:     15659 MiB
	Backend: LLVM 18.1.6

Expected Behavior

When compiling with -default-to-nil-allocator and setting context.allocator and context.temp_allocator to custom allocators, I expect os2.process_start to use these. But it does not.

Current Behavior

os2.process_start accesses temp_allocator() meaning the default global temp allocator instead of using context.allocator or context.temp_allocator. With the build option -default-to-nil-allocator, that leads eventually to a panic or failed assert.

Additionally, that means that it is not visible from the signature of the function that it allocates and we cannot provide it a specific allocator just for one call e.g. os.process_start(desc, allocator=foobar).

Steps to Reproduce

Here is a program that sets context.allocator and context.temp_allocator to fixed arenas and tries to spawn a process (does not matter which one) with os2.process_start. It will end up in a failed assert because it tries to use the default general temp allocator to allocate some memory (to convert the executable name to a cstring). This general temp allocator cannot allocate any memory because its backing allocator (the general purpose default allocator) is nil.

package main

import "core:fmt"
import "core:mem"
import "core:mem/virtual"
import "core:os/os2"

main :: proc() {

	arena: virtual.Arena

	{
		arena_size := uint(8) * mem.Megabyte
		mmapped, err := virtual.reserve_and_commit(arena_size)
		if err != nil {
			panic("err")
		}
		if err = virtual.arena_init_buffer(&arena, mmapped); err != nil {
			panic("err")
		}
	}
	context.allocator = virtual.arena_allocator(&arena)

	tmp_arena: virtual.Arena
	{
		tmp_arena_size := uint(1) * mem.Megabyte
		tmp_mmapped, err := virtual.reserve_and_commit(tmp_arena_size)
		if err != nil {
			fmt.eprintln(err)
			panic("err")
		}
		if err = virtual.arena_init_buffer(&tmp_arena, tmp_mmapped); err != nil {
			fmt.eprintln(err)
			panic("err")
		}
	}
	context.temp_allocator = virtual.arena_allocator(&tmp_arena)

	desc := os2.Process_Desc {
		command = []string{"ls"},
	}

	process, err := os2.process_start(desc) // Assert failed!
	if err != nil {
		panic("err")
	}

	process_state, err2 := os2.process_wait(process)
	fmt.println(process_state, err2)
}

Failure Logs

Run:

$ odin run src/  -vet -strict-style -default-to-nil-allocator -debug
/home/pg/not-my-code/Odin/core/strings/builder.odin(299:2) runtime assertion: len(array) > 0
fish: Job 1, 'odin run src/  -vet -strict-sty…' terminated by signal SIGILL (Illegal instruction)

GDB:

(gdb) r
Starting program: /home/pg/scratch/odin-nil-allocator/src.bin 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
/home/pg/not-my-code/Odin/core/strings/builder.odin(299:2) runtime assertion: len(array) > 0

Program received signal SIGILL, Illegal instruction.
runtime.default_assertion_contextless_failure_proc (prefix=..., message=..., loc=...) at /home/pg/not-my-code/Odin/base/runtime/core.odin:811
811		trap()
(gdb) bt
#0  runtime.default_assertion_contextless_failure_proc (prefix=..., message=..., loc=...) at /home/pg/not-my-code/Odin/base/runtime/core.odin:811
#1  0x000000000043201d in runtime.default_assertion_failure_proc (prefix=..., message=..., loc=...) at /home/pg/not-my-code/Odin/base/runtime/core.odin:782
#2  0x000000000044d24a in runtime.assert.internal-0 (message=..., loc=...) at /home/pg/not-my-code/Odin/base/runtime/core_builtin.odin:928
#3  0x000000000043ae3a in runtime.assert (condition=false, message=..., loc=...) at /home/pg/not-my-code/Odin/base/runtime/core_builtin.odin:930
#4  0x000000000041d330 in runtime.pop-25293 (array=0x7fffffffdc60, loc=...) at /home/pg/not-my-code/Odin/base/runtime/core_builtin.odin:115
#5  0x000000000041d2cd in strings.to_cstring (b=0x7fffffffdc60) at /home/pg/not-my-code/Odin/core/strings/builder.odin:299
#6  0x000000000041f4d4 in os2._process_start-1575 (desc=...) at /home/pg/not-my-code/Odin/core/os/os2/process_linux.odin:446
#7  0x0000000000422a1b in os2.process_start (desc=...) at /home/pg/not-my-code/Odin/core/os/os2/process.odin:347
#8  0x0000000000403549 in main.main () at main.odin:43
#9  0x0000000000409065 in main (argc=1, argv=0x7fffffffe438) at /home/pg/not-my-code/Odin/base/runtime/entry_unix.odin:57

Suggestion: A allocator should be present as one of the arguments of os2.process_start and be used inside it.

@gaultier gaultier changed the title os2.process_start uses the global allocator instead of context.allocator which can lead to a panic os2.process_start uses the global allocator instead of context.allocator which can lead to a panic/failed assert Sep 18, 2024
@flysand7
Copy link
Contributor

I'm not sure how -default-to-nil-allocators is supposed to work, so can't tell enough about the bug. Seems like os2 is implemented correctly though.

The crash is happening on this line of code:

exe_path := strings.to_cstring(&exe_builder)

For now figuring out what exactly is broken, and whether it's broken at all. I thought that maybe my core:mem PR was breaking it, but checking out the commit before this PR was merged still reproe'd the bug.

When compiling with -default-to-nil-allocator and setting context.allocator and context.temp_allocator to custom allocators, I expect os2.process_start to use these. But it does not.

Yes, it shouldn't. os2's temp_allocator() is supposed to not be affected by context, that's on purpose. The idea is that if it comes from the context it can break (because we have to handle all allocator errors from user context), but if have a separate allocator just for the things that are alive during the oscall procedure, we don't have to handle allocator errors as much.

Additionally, that means that it is not visible from the signature of the function that it allocates and we cannot provide it a specific allocator just for one call e.g. os.process_start(desc, allocator=foobar).

This is also expected, because process_start(), actually does not allocate any memory that you have to free in a later call. Everything that this function allocates is deallocated within the same function, so it's all temporary allocations to get the right strings to supply to the OS.

@flysand7
Copy link
Contributor

Running hypothesis is that -default-to-nil-allocator somehow makes os2.temp_allocator() a nil allocator.

@jasonKercher
Copy link
Contributor

jasonKercher commented Sep 18, 2024

I believe context.temp_allocator works despite this because of the when block at the top of base/runtime/default_temporary_allocator.odin. However, the user can always overwrite that. This is not the case for the os2's temp_allocator.

E: -default-to-nil-allocator causes runtime.default_allocator to be the nil_allocator. The os2 temp_allocator uses runtime.arena_allocator_proc. When that requires more memory it requests it from runtime.default_allocator.

@laytan laytan closed this as completed in 7491b3c Sep 18, 2024
@gaultier
Copy link
Author

gaultier commented Sep 19, 2024

Thank you for the prompt response.
The fix should work, I’ll try it when I get back to a computer. (EDIT: I tried, Confirmed to work).

However, one disadvantage to this approach is that if someone wants to prevent any allocations using the default allocators, meaning they want to control how every and any allocation is done (for example on a tiny ARM 32 device), then the current code paired with the option “-default-to-nil-allocator” is misleading, this goal won’t be achieved. Behind the scenes, the default allocators are still being used silently sometimes.

Now I don’t know if this goal is supported by Odin, or if the docs should clarify what this command line option really does?

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