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

Fix runtime.memory_compare buffer overrun #4411

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tf2spi
Copy link
Contributor

@tf2spi tf2spi commented Oct 23, 2024

Fixes #4405

Test Program

package repro
import "base:runtime"
import "core:fmt"

main :: proc() {
        Z : [16]byte = 0
        X : [48]byte
        for i in 1..=16 {
                for j in 0..<16 {
                        A := X[0:j]
                        B := X[j:i+j]
                        C := X[i+j:]
                        for &a in A { a = 0xff }
                        for &b in B { b = 0 }
                        for &c in C { c = 0xff }

                        result := runtime.memory_compare_zero(raw_data(B), i)
                        if result != 0 {
                                fmt.println("Test failed zero!", i, j, result)
                                return
                        }

                        result = runtime.memory_compare(raw_data(B), raw_data(Z[:i]), i)
                        if result != 0 {
                                fmt.println("Test failed cmp!", i, j, result)
                                return
                        }
                }
        }
        fmt.println("All Tests Passed!")
}

EDIT:
Wanted a test program that tested different lengths and alignments but it's not working right now.
This was the original test program and this change does fix the crash.

package repro

main :: proc() {
    a := reserve_and_commit(mem.DEFAULT_PAGE_SIZE) or_else panic("1")
    b := reserve_and_commit(mem.DEFAULT_PAGE_SIZE) or_else panic("2")

    // prove that sufficient space is mapped
    fmt.println("last a:", a[mem.DEFAULT_PAGE_SIZE-1])
    fmt.println("last b:", b[mem.DEFAULT_PAGE_SIZE-1])
    fmt.println("last b address:", &b[mem.DEFAULT_PAGE_SIZE-1])

    // XXX: all of these crash
    runtime.memory_compare_zero(raw_data(b), len(b))
    runtime.memory_compare(raw_data(a), raw_data(b), len(b))
    fmt.println(mem.compare(a, b))
}


// NOTE: copied directly from core:mem/virtual darwin

MAP_ANONYMOUS :: 0x1000 /* allocated from memory, swap space */
_reserve :: proc "contextless" (size: uint) -> (data: []byte, err: Allocator_Error) {
	flags  := posix.Map_Flags{ .PRIVATE } + transmute(posix.Map_Flags)i32(MAP_ANONYMOUS)
	result := posix.mmap(nil, size, {}, flags)
	if result == posix.MAP_FAILED {
		return nil, .Out_Of_Memory
	}

	return ([^]byte)(uintptr(result))[:size], nil
}

_commit :: proc "contextless" (data: rawptr, size: uint) -> Allocator_Error {
	if posix.mprotect(data, size, { .READ, .WRITE }) != .OK {
		return .Out_Of_Memory
	}

	return nil
}

reserve_and_commit :: proc "contextless" (size: uint) -> (data: []byte, err: Allocator_Error) {
	data = _reserve(size) or_return
	_commit(raw_data(data), size) or_return
	return
}


Allocator_Error :: runtime.Allocator_Error

import "core:fmt"
import "core:mem"
import "core:sys/posix"
import "base:runtime"

How This Fixes It

It was kind of an awkward rewrite. It's changed by checking alignment at every access and then adjusting counts and pointers at every access so that fancy pointer math doesn't have to be done.

@Lperlind
Copy link
Contributor

Is there any major performance impact with the change?

@tf2spi
Copy link
Contributor Author

tf2spi commented Oct 23, 2024

I'm not sure. I haven't stressed it out much.

@laytan
Copy link
Collaborator

laytan commented Oct 23, 2024

We should consider @Feoramund's #4063 a fix too, if that is still planned to go ahead

@laytan laytan changed the title Issue 4405 fix Fix runtime.memory_compare buffer overrun Oct 23, 2024
@Feoramund
Copy link
Contributor

We should consider @Feoramund's #4063 a fix too, if that is still planned to go ahead

I've been working on another project that has taken up most of my time, but I do plan to get back to that PR. There was a significant performance improvement in one of the procedures, if I recall correctly.

@tf2spi
Copy link
Contributor Author

tf2spi commented Oct 23, 2024

Yeah, #4063 definitely looks a lot better. This was kind of a hack to fix the issue. Would it be better to close this when the other PR is done and reference it at that point or close it right now?

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

Successfully merging this pull request may close these issues.

possible buffer overrun of runtime.memory_compare() and runtime.memory_compare_zero()
4 participants