[kbkpbot]

Fix #24356

If not set code, error_win32() will call C.GetLastError() again and get a zero value.

[huly-for-github]

Connected to Huly®: V_0.6-22733

[spytheman]

For me, the test in the issue passes with all compilers on master:

d:\programs\v>v vlib\os\os_test.c.v
testsuite_begin, tfolder = C:\Users\delia\AppData\Local\Temp\v_0\os_tests
skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

>>> os.Command is not implemented fully on Windows yet
skipping test_mv_by_cp, because bindfs was not present
skipping test_mv_by_cp, because bindfs was not present

d:\programs\v>
d:\programs\v>v -cc msvc vlib\os\os_test.c.v
testsuite_begin, tfolder = C:\Users\delia\AppData\Local\Temp\v_0\os_tests
skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

>>> os.Command is not implemented fully on Windows yet
skipping test_mv_by_cp, because bindfs was not present
skipping test_mv_by_cp, because bindfs was not present

d:\programs\v>
d:\programs\v>v -cc gcc vlib\os\os_test.c.v
testsuite_begin, tfolder = C:\Users\delia\AppData\Local\Temp\v_0\os_tests
skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

>>> os.Command is not implemented fully on Windows yet
skipping test_mv_by_cp, because bindfs was not present
skipping test_mv_by_cp, because bindfs was not present

d:\programs\v>v -cc clang vlib\os\os_test.c.v
testsuite_begin, tfolder = C:\Users\delia\AppData\Local\Temp\v_0\os_tests
skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

skipping handle_privilege_error on windows, since the user is not administrator, err:
    A required privilege is not held by the client.

>>> os.Command is not implemented fully on Windows yet
skipping test_mv_by_cp, because bindfs was not present
skipping test_mv_by_cp, because bindfs was not present

d:\programs\v>

[spytheman]

The other remaining call does:
return error_win32(msg: 'failed to copy ${src} to ${dst}'), i.e. without explicitly setting the code: field, and the implementation does:

@[inline; manualfree]
pub fn error_win32(e SystemError) IError {
    $if windows {
        code := if e.code == error_code_not_set { int(C.GetLastError()) } else { e.code }
        message := if e.msg == '' { get_error_msg(code) } else { e.msg }
        return error_with_code(message, code)
    } $else {
        panic('Win32 API not available on this platform.')
    }
}        

we also have:

// Magic constant because zero is used explicitly at times
pub const error_code_not_set = int(0x7EFEFEFE)

@[params]
pub struct SystemError {
pub:
    msg  string
    code int = error_code_not_set
}

Is it possible that "magic" constant is not so magic, and has to be changed to some value that is not actually used as a return error?

[spytheman]

According to this: https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes all system error codes are positive numbers. Can we use pub const error_code_not_set = int(-1) instead of 0x7EFEFEFE?

[kbkpbot]

I think there should be some system calls in get_error_msg() and overwrite GetLastError() error code, I will check the generated C code.

[kbkpbot]

I think there should be some system calls in get_error_msg() and overwrite GetLastError() error code, I will check the generated C code.

get_error_msg() ->ptr_win_get_error_msg()->C.FormatMessageW(), as C.FormatMessageW() may overwrite error code, so it need some way save the original error code.

also, get_error_msg() ->string_from_wide()->string_from_wide2()->C.WideCharToMultiByte() may has same problem.

[kbkpbot]

According to this: https://learn.microsoft.com/en-us/windows/win32/debug/system-error-codes all system error codes are positive numbers. Can we use pub const error_code_not_set = int(-1) instead of 0x7EFEFEFE?

Yes, I agree

[kbkpbot]

bad news:

str_intp also overwrite GetLastError() error code

pub fn rmdir(path string) ! {
	$if windows {
		rc := C.RemoveDirectory(path.to_wide())
		if !rc {
			// https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-removedirectorya - 0 == false, is failure
			_ := 'Failed to remove "${path}"'
			code := int(C.GetLastError())                 // here , we get error code 0, which is overwritten by previous `str_intp` 
			dump(code)
			return error_win32( msg: 'Failed to remove "${path}"')
		}
	} $else {
		rc := C.rmdir(&char(path.str))
		if rc == -1 {
			return error_posix()
		}
	}
}

[spytheman]

str_intp also overwrite GetLastError() error code

I have not thought about that at all 🤦🏻‍♂️. Good find.

[spytheman]

Excellent work and investigation.