Fix #4890

`strings.to_cstring` previously would not check if the buffer could handle the extra null byte and could lead to segmentation violations when using the resulting string in an API expecting the terminator.
odin-lang · Feb 27, 2025 · b2e3b34 · b2e3b34
1 parent 94152ca
commit b2e3b34
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 16 deletions.
diff --git a/core/os/os2/path_openbsd.odin b/core/os/os2/path_openbsd.odin
@@ -46,7 +46,7 @@ _get_executable_path :: proc(allocator: runtime.Allocator) -> (path: string, err
 		strings.write_string(&buf, "/")
 		strings.write_string(&buf, sarg)
 
-		cpath := strings.to_cstring(&buf)
+		cpath := strings.to_cstring(&buf) or_return
 		if posix.access(cpath, {.X_OK}) == .OK {
 			return real(cpath, allocator)
 		}

diff --git a/core/os/os2/pipe_posix.odin b/core/os/os2/pipe_posix.odin
@@ -29,7 +29,7 @@ _pipe :: proc() -> (r, w: ^File, err: Error) {
 	strings.write_string(&rname, "/dev/fd/")
 	strings.write_int(&rname, int(fds[0]))
 	ri.name  = strings.to_string(rname)
-	ri.cname = strings.to_cstring(&rname)
+	ri.cname = strings.to_cstring(&rname) or_return
 
 	w = __new_file(fds[1], file_allocator())
 	wi := (^File_Impl)(w.impl)
@@ -39,7 +39,7 @@ _pipe :: proc() -> (r, w: ^File, err: Error) {
 	strings.write_string(&wname, "/dev/fd/")
 	strings.write_int(&wname, int(fds[1]))
 	wi.name  = strings.to_string(wname)
-	wi.cname = strings.to_cstring(&wname)
+	wi.cname = strings.to_cstring(&wname) or_return
 
 	return
 }

diff --git a/core/os/os2/process_linux.odin b/core/os/os2/process_linux.odin
@@ -111,7 +111,7 @@ _process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator
 
 	strings.write_string(&path_builder, "/proc/")
 	strings.write_int(&path_builder, pid)
-	proc_fd, errno := linux.open(strings.to_cstring(&path_builder), _OPENDIR_FLAGS)
+	proc_fd, errno := linux.open(strings.to_cstring(&path_builder) or_return, _OPENDIR_FLAGS)
 	if errno != .NONE {
 		err = _get_platform_error(errno)
 		return
@@ -169,7 +169,7 @@ _process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator
 		strings.write_int(&path_builder, pid)
 		strings.write_string(&path_builder, "/cmdline")
 
-		cmdline_bytes, cmdline_err := _read_entire_pseudo_file(strings.to_cstring(&path_builder), temp_allocator())
+		cmdline_bytes, cmdline_err := _read_entire_pseudo_file(strings.to_cstring(&path_builder) or_return, temp_allocator())
 		if cmdline_err != nil || len(cmdline_bytes) == 0 {
 			err = cmdline_err
 			break cmdline_if
@@ -190,7 +190,7 @@ _process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator
 			strings.write_int(&path_builder, pid)
 			strings.write_string(&path_builder, "/cwd")
 
-			cwd, cwd_err = _read_link_cstr(strings.to_cstring(&path_builder), temp_allocator()) // allowed to fail
+			cwd, cwd_err = _read_link_cstr(strings.to_cstring(&path_builder) or_return, temp_allocator()) // allowed to fail
 			if cwd_err == nil && .Working_Dir in selection {
 				info.working_dir = strings.clone(cwd, allocator) or_return
 				info.fields += {.Working_Dir}
@@ -258,7 +258,7 @@ _process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator
 		strings.write_int(&path_builder, pid)
 		strings.write_string(&path_builder, "/stat")
 
-		proc_stat_bytes, stat_err := _read_entire_pseudo_file(strings.to_cstring(&path_builder), temp_allocator())
+		proc_stat_bytes, stat_err := _read_entire_pseudo_file(strings.to_cstring(&path_builder) or_return, temp_allocator())
 		if stat_err != nil {
 			err = stat_err
 			break stat_if
@@ -330,7 +330,7 @@ _process_info_by_pid :: proc(pid: int, selection: Process_Info_Fields, allocator
 		strings.write_int(&path_builder, pid)
 		strings.write_string(&path_builder, "/environ")
 
-		if env_bytes, env_err := _read_entire_pseudo_file(strings.to_cstring(&path_builder), temp_allocator()); env_err == nil {
+		if env_bytes, env_err := _read_entire_pseudo_file(strings.to_cstring(&path_builder) or_return, temp_allocator()); env_err == nil {
 			env := string(env_bytes)
 
 			env_list := make([dynamic]string, allocator) or_return
@@ -418,7 +418,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 			strings.write_byte(&exe_builder, '/')
 			strings.write_string(&exe_builder, executable_name)
 
-			exe_path = strings.to_cstring(&exe_builder)
+			exe_path = strings.to_cstring(&exe_builder) or_return
 			if linux.access(exe_path, linux.X_OK) == .NONE {
 				found = true
 				break
@@ -430,7 +430,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 			strings.write_string(&exe_builder, "./")
 			strings.write_string(&exe_builder, executable_name)
 
-			exe_path = strings.to_cstring(&exe_builder)
+			exe_path = strings.to_cstring(&exe_builder) or_return
 			if linux.access(exe_path, linux.X_OK) != .NONE {
 				return process, .Not_Exist
 			}
@@ -594,7 +594,7 @@ _process_state_update_times :: proc(state: ^Process_State) -> (err: Error) {
 	strings.write_string(&path_builder, "/stat")
 
 	stat_buf: []u8
-	stat_buf, err = _read_entire_pseudo_file(strings.to_cstring(&path_builder), temp_allocator())
+	stat_buf, err = _read_entire_pseudo_file(strings.to_cstring(&path_builder) or_return, temp_allocator())
 	if err != nil {
 		return
 	}

diff --git a/core/os/os2/process_posix.odin b/core/os/os2/process_posix.odin
@@ -71,7 +71,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 			strings.write_byte(&exe_builder, '/')
 			strings.write_string(&exe_builder, exe_name)
 
-			if exe_fd := posix.open(strings.to_cstring(&exe_builder), {.CLOEXEC, .EXEC}); exe_fd == -1 {
+			if exe_fd := posix.open(strings.to_cstring(&exe_builder) or_return, {.CLOEXEC, .EXEC}); exe_fd == -1 {
 				continue
 			} else {
 				posix.close(exe_fd)
@@ -91,7 +91,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 
 			// "hello/./world" is fine right?
 
-			if exe_fd := posix.open(strings.to_cstring(&exe_builder), {.CLOEXEC, .EXEC}); exe_fd == -1 {
+			if exe_fd := posix.open(strings.to_cstring(&exe_builder) or_return, {.CLOEXEC, .EXEC}); exe_fd == -1 {
 				err = .Not_Exist
 				return
 			} else {
@@ -102,7 +102,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 		strings.builder_reset(&exe_builder)
 		strings.write_string(&exe_builder, exe_name)
 
-		if exe_fd := posix.open(strings.to_cstring(&exe_builder), {.CLOEXEC, .EXEC}); exe_fd == -1 {
+		if exe_fd := posix.open(strings.to_cstring(&exe_builder) or_return, {.CLOEXEC, .EXEC}); exe_fd == -1 {
 			err = .Not_Exist
 			return
 		} else {
@@ -181,7 +181,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) {
 			if posix.chdir(cwd) != .OK { abort(pipe[WRITE]) }
 		}
 
-		res := posix.execve(strings.to_cstring(&exe_builder), raw_data(cmd), env)
+		res := posix.execve(strings.to_cstring(&exe_builder) or_return, raw_data(cmd), env)
 		assert(res == -1)
 		abort(pipe[WRITE])
 

diff --git a/core/strings/builder.odin b/core/strings/builder.odin
@@ -288,18 +288,41 @@ to_string :: proc(b: Builder) -> (res: string) {
 /*
 Appends a trailing null byte after the end of the current Builder byte buffer and then casts it to a cstring
 
+NOTE: This procedure will not check if the backing buffer has enough space to include the extra null byte.
+
 Inputs:
 - b: A pointer to builder
 
 Returns:
 - res: A cstring of the Builder's buffer
 */
-to_cstring :: proc(b: ^Builder) -> (res: cstring) {
+unsafe_to_cstring :: proc(b: ^Builder) -> (res: cstring) {
 	append(&b.buf, 0)
 	pop(&b.buf)
 	return cstring(raw_data(b.buf))
 }
 /*
+Appends a trailing null byte after the end of the current Builder byte buffer and then casts it to a cstring
+
+Inputs:
+- b: A pointer to builder
+
+Returns:
+- res: A cstring of the Builder's buffer upon success
+- err: An optional allocator error if one occured, `nil` otherwise
+*/
+to_cstring :: proc(b: ^Builder) -> (res: cstring, err: mem.Allocator_Error) {
+	n := append(&b.buf, 0) or_return
+	if n != 1 {
+		return nil, .Out_Of_Memory
+	}
+	pop(&b.buf)
+	#no_bounds_check {
+		assert(b.buf[len(b.buf)] == 0)
+	}
+	return cstring(raw_data(b.buf)), nil
+}
+/*
 Returns the length of the Builder's buffer, in bytes
 
 Inputs: