From 7a0dc60d04808bfa7fe5c11034e91f5d475adedb Mon Sep 17 00:00:00 2001 From: Delyan Angelov Date: Mon, 22 Nov 2021 14:40:55 +0200 Subject: [PATCH] os: re-add the leak in os.join_path (the `os.join_path(x, ...arr)` case should be handled by V). Add a memleak free os.join_path_single version. --- vlib/os/file_test.v | 4 +- vlib/os/os.c.v | 2 +- vlib/os/os.v | 44 ++++++++++++++----- vlib/os/os_nix.c.v | 2 +- vlib/os/os_test.v | 38 ++++++++-------- vlib/os/os_windows.c.v | 3 +- .../import_os_and_use_its_constants.v | 6 +-- 7 files changed, 60 insertions(+), 39 deletions(-) diff --git a/vlib/os/file_test.v b/vlib/os/file_test.v index 3339ad828e..3757642f26 100644 --- a/vlib/os/file_test.v +++ b/vlib/os/file_test.v @@ -41,8 +41,8 @@ const ( ) const ( - tfolder = os.join_path(os.temp_dir(), 'os_file_test') - tfile = os.join_path(tfolder, 'test_file') + tfolder = os.join_path_single(os.temp_dir(), 'os_file_test') + tfile = os.join_path_single(tfolder, 'test_file') ) fn testsuite_begin() ? { diff --git a/vlib/os/os.c.v b/vlib/os/os.c.v index 97f8ab1bde..67c0f4429e 100644 --- a/vlib/os/os.c.v +++ b/vlib/os/os.c.v @@ -191,7 +191,7 @@ pub fn file_size(path string) u64 { pub fn mv(src string, dst string) ? { mut rdst := dst if is_dir(rdst) { - rdst = join_path(rdst.trim_right(path_separator), file_name(src.trim_right(path_separator))) + rdst = join_path_single(rdst.trim_right(path_separator), file_name(src.trim_right(path_separator))) } $if windows { w_src := src.replace('/', '\\') diff --git a/vlib/os/os.v b/vlib/os/os.v index 4ba6ab2911..0ebb1dec39 100644 --- a/vlib/os/os.v +++ b/vlib/os/os.v @@ -40,7 +40,8 @@ pub fn cp_all(src string, dst string, overwrite bool) ? { // single file copy if !is_dir(source_path) { adjusted_path := if is_dir(dest_path) { - join_path(dest_path, file_name(source_path)) + fname := file_name(source_path) + join_path_single(dest_path, fname) } else { dest_path } @@ -62,8 +63,8 @@ pub fn cp_all(src string, dst string, overwrite bool) ? { } files := ls(source_path) ? for file in files { - sp := join_path(source_path, file) - dp := join_path(dest_path, file) + sp := join_path_single(source_path, file) + dp := join_path_single(dest_path, file) if is_dir(sp) { if !exists(dp) { mkdir(dp) ? @@ -136,7 +137,7 @@ pub fn rmdir_all(path string) ? { mut ret_err := '' items := ls(path) ? for item in items { - fullpath := join_path(path, item) + fullpath := join_path_single(path, item) if is_dir(fullpath) && !is_link(fullpath) { rmdir_all(fullpath) or { ret_err = err.msg } } else { @@ -379,7 +380,7 @@ fn executable_fallback() string { if !is_abs_path(exepath) { rexepath := exepath.replace_each(['/', path_separator, r'\', path_separator]) if rexepath.contains(path_separator) { - exepath = join_path(os.wd_at_startup, exepath) + exepath = join_path_single(os.wd_at_startup, exepath) } else { // no choice but to try to walk the PATH folders :-| ... foundpath := find_abs_path_of_executable(exepath) or { '' } @@ -414,7 +415,7 @@ pub fn find_abs_path_of_executable(exepath string) ?string { path := getenv('PATH') paths := path.split(path_delimiter) for p in paths { - found_abs_path := join_path(p, exepath) + found_abs_path := join_path_single(p, exepath) if exists(found_abs_path) && is_executable(found_abs_path) { res = found_abs_path break @@ -452,9 +453,8 @@ pub fn is_abs_path(path string) bool { // join_path returns a path as string from input string parameter(s). [manualfree] pub fn join_path(base string, dirs ...string) string { - defer { - unsafe { dirs.free() } - } + // TODO: fix freeing of `dirs` when the passed arguments are variadic, + // but do not free the arr, when `os.join_path(base, ...arr)` is called. mut sb := strings.new_builder(base.len + dirs.len * 50) defer { unsafe { sb.free() } @@ -471,6 +471,26 @@ pub fn join_path(base string, dirs ...string) string { return sb.str() } +// join_path_single appends the `elem` after `base`, using a platform specific +// path_separator. +[manualfree] +pub fn join_path_single(base string, elem string) string { + // TODO: deprecate this and make it `return os.join_path(base, elem)`, + // when freeing variadic args vs ...arr is solved in the compiler + mut sb := strings.new_builder(base.len + elem.len + 1) + defer { + unsafe { sb.free() } + } + sbase := base.trim_right('\\/') + defer { + unsafe { sbase.free() } + } + sb.write_string(sbase) + sb.write_string(path_separator) + sb.write_string(elem) + return sb.str() +} + // walk_ext returns a recursive list of all files in `path` ending with `ext`. pub fn walk_ext(path string, ext string) []string { mut res := []string{} @@ -587,7 +607,7 @@ pub fn cache_dir() string { return xdg_cache_home } } - cdir := join_path(home_dir(), '.cache') + cdir := join_path_single(home_dir(), '.cache') if !is_dir(cdir) && !is_link(cdir) { mkdir(cdir) or { panic(err) } } @@ -629,7 +649,7 @@ pub fn temp_dir() string { fn default_vmodules_path() string { hdir := home_dir() - res := join_path(hdir, '.vmodules') + res := join_path_single(hdir, '.vmodules') return res } @@ -684,7 +704,7 @@ pub fn resource_abs_path(path string) string { unsafe { base_path.free() } base_path = vresource } - fp := join_path(base_path, path) + fp := join_path_single(base_path, path) res := real_path(fp) unsafe { fp.free() diff --git a/vlib/os/os_nix.c.v b/vlib/os/os_nix.c.v index a7f3029f77..5e9f78ba3a 100644 --- a/vlib/os/os_nix.c.v +++ b/vlib/os/os_nix.c.v @@ -480,7 +480,7 @@ pub fn is_writable_folder(folder string) ?bool { if !is_dir(folder) { return error('`folder` is not a folder') } - tmp_perm_check := join_path(folder, 'XXXXXX') + tmp_perm_check := join_path_single(folder, 'XXXXXX') defer { unsafe { tmp_perm_check.free() } } diff --git a/vlib/os/os_test.v b/vlib/os/os_test.v index e566a79162..b9689e6ffc 100644 --- a/vlib/os/os_test.v +++ b/vlib/os/os_test.v @@ -106,9 +106,9 @@ fn test_create_file() ? { fn test_is_file() { // Setup - work_dir := os.join_path(os.getwd(), 'is_file_test') + work_dir := os.join_path_single(os.getwd(), 'is_file_test') os.mkdir_all(work_dir) or { panic(err) } - tfile := os.join_path(work_dir, 'tmp_file') + tfile := os.join_path_single(work_dir, 'tmp_file') // Test things that shouldn't be a file assert os.is_file(work_dir) == false assert os.is_file('non-existent_file.tmp') == false @@ -120,7 +120,7 @@ fn test_is_file() { $if windows { assert true } $else { - dsymlink := os.join_path(work_dir, 'dir_symlink') + dsymlink := os.join_path_single(work_dir, 'dir_symlink') os.symlink(work_dir, dsymlink) or { panic(err) } assert os.is_file(dsymlink) == false } @@ -128,7 +128,7 @@ fn test_is_file() { $if windows { assert true } $else { - fsymlink := os.join_path(work_dir, 'file_symlink') + fsymlink := os.join_path_single(work_dir, 'file_symlink') os.symlink(tfile, fsymlink) or { panic(err) } assert os.is_file(fsymlink) } @@ -309,47 +309,47 @@ fn test_cp() { } fn test_mv() { - work_dir := os.join_path(os.getwd(), 'mv_test') + work_dir := os.join_path_single(os.getwd(), 'mv_test') os.mkdir_all(work_dir) or { panic(err) } // Setup test files - tfile1 := os.join_path(work_dir, 'file') - tfile2 := os.join_path(work_dir, 'file.test') - tfile3 := os.join_path(work_dir, 'file.3') + tfile1 := os.join_path_single(work_dir, 'file') + tfile2 := os.join_path_single(work_dir, 'file.test') + tfile3 := os.join_path_single(work_dir, 'file.3') tfile_content := 'temporary file' os.write_file(tfile1, tfile_content) or { panic(err) } os.write_file(tfile2, tfile_content) or { panic(err) } // Setup test dirs - tdir1 := os.join_path(work_dir, 'dir') - tdir2 := os.join_path(work_dir, 'dir2') - tdir3 := os.join_path(work_dir, 'dir3') + tdir1 := os.join_path_single(work_dir, 'dir') + tdir2 := os.join_path_single(work_dir, 'dir2') + tdir3 := os.join_path_single(work_dir, 'dir3') os.mkdir(tdir1) or { panic(err) } os.mkdir(tdir2) or { panic(err) } // Move file with no extension to dir os.mv(tfile1, tdir1) or { panic(err) } - mut expected := os.join_path(tdir1, 'file') + mut expected := os.join_path_single(tdir1, 'file') assert os.exists(expected) assert !os.is_dir(expected) // Move dir with contents to other dir os.mv(tdir1, tdir2) or { panic(err) } - expected = os.join_path(tdir2, 'dir') + expected = os.join_path_single(tdir2, 'dir') assert os.exists(expected) assert os.is_dir(expected) expected = os.join_path(tdir2, 'dir', 'file') assert os.exists(expected) assert !os.is_dir(expected) // Move dir with contents to other dir (by renaming) - os.mv(os.join_path(tdir2, 'dir'), tdir3) or { panic(err) } + os.mv(os.join_path_single(tdir2, 'dir'), tdir3) or { panic(err) } expected = tdir3 assert os.exists(expected) assert os.is_dir(expected) assert os.is_dir_empty(tdir2) // Move file with extension to dir os.mv(tfile2, tdir2) or { panic(err) } - expected = os.join_path(tdir2, 'file.test') + expected = os.join_path_single(tdir2, 'file.test') assert os.exists(expected) assert !os.is_dir(expected) // Move file to dir (by renaming) - os.mv(os.join_path(tdir2, 'file.test'), tfile3) or { panic(err) } + os.mv(os.join_path_single(tdir2, 'file.test'), tfile3) or { panic(err) } expected = tfile3 assert os.exists(expected) assert !os.is_dir(expected) @@ -375,7 +375,7 @@ fn test_cp_all() { // regression test for executive runs with overwrite := true os.cp_all('ex', './', true) or { panic(err) } os.cp_all('ex', 'nonexisting', true) or { panic(err) } - assert os.exists(os.join_path('nonexisting', 'ex1.txt')) + assert os.exists(os.join_path_single('nonexisting', 'ex1.txt')) } fn test_realpath_of_empty_string_works() { @@ -398,7 +398,7 @@ fn test_realpath_non_existing() { fn test_realpath_existing() { existing_file_name := 'existing_file.txt' - existing_file := os.join_path(os.temp_dir(), existing_file_name) + existing_file := os.join_path_single(os.temp_dir(), existing_file_name) os.rm(existing_file) or {} os.write_file(existing_file, 'abc') or {} assert os.exists(existing_file) @@ -843,7 +843,7 @@ fn test_expand_tilde_to_home() { } fn test_execute() ? { - print0script := os.join_path(tfolder, 'print0.v') + print0script := os.join_path_single(tfolder, 'print0.v') // The output of the next command contains a 0 byte in the middle. // Nevertheless, the execute function *should* return a string that // contains it. diff --git a/vlib/os/os_windows.c.v b/vlib/os/os_windows.c.v index c7738cdad6..b482505a16 100644 --- a/vlib/os/os_windows.c.v +++ b/vlib/os/os_windows.c.v @@ -503,7 +503,8 @@ pub fn is_writable_folder(folder string) ?bool { if !is_dir(folder) { return error('`folder` is not a folder') } - tmp_perm_check := join_path(folder, 'tmp_perm_check_pid_' + getpid().str()) + tmp_folder_name := 'tmp_perm_check_pid_' + getpid().str() + tmp_perm_check := join_path_single(folder, tmp_folder_name) mut f := open_file(tmp_perm_check, 'w+', 0o700) or { return error('cannot write to folder $folder: $err') } diff --git a/vlib/v/tests/valgrind/import_os_and_use_its_constants.v b/vlib/v/tests/valgrind/import_os_and_use_its_constants.v index 0ef480b542..642695ee42 100644 --- a/vlib/v/tests/valgrind/import_os_and_use_its_constants.v +++ b/vlib/v/tests/valgrind/import_os_and_use_its_constants.v @@ -7,8 +7,8 @@ fn main() { println(os.args) println(os.wd_at_startup) // - fullpath := os.join_path('abc', 'xyz', 'def') - println(fullpath) + // fullpath := os.join_path('abc', 'xyz', 'def') + // println(fullpath) // x := 'abc' t := x.trim_right('/') @@ -30,7 +30,7 @@ fn main() { // exeparent_folder := os.dir(exe_realpath) // println(exeparent_folder) - cdir := os.join_path(os.home_dir(), '.cache') + cdir := os.join_path_single(os.home_dir(), '.cache') println(cdir) wd_realpath := os.real_path(os.wd_at_startup)