fix for Issue648 #136
fix for Issue648 #136
Conversation
contrib/win32/win32compat/misc.c
Outdated
| path_start = path + 2; | ||
| else | ||
| path_start = path; | ||
| if ((*path_start == L'\0') || ((*path_start == L'\\' || *path_start == L'/' ) && path_start[1] == L'\0')) |
There was a problem hiding this comment.
could you provide a scenario when we run into this logic?
There was a problem hiding this comment.
*path_start == L'\0' means the path is empty, or path like c:\ (for this case, path_start is from index 2)
(*path_start == L'\' || *path_start == L'/' ) && path_start[1] == L'\0' means the path is root directory. your can try "dir \"
There was a problem hiding this comment.
I added one line comment for the sample path
| last_dot = wcsrchr(path, L'.'); | ||
| if (!last_dot) | ||
| return FALSE; | ||
| if (_wcsnicmp(last_dot, L".exe", 4) != 0 && _wcsnicmp(last_dot, L".cmd", 4) != 0 && |
There was a problem hiding this comment.
You might need to retrieve the executable extension from %PATHEXT% environment variable..
http://environmentvariables.org/PathExt
If you fail to find / retrieve the environment variable then you can fall back on to the standard ones
.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC (This is not an exhaustive list)
There was a problem hiding this comment.
I compared the results for other extension, the current implementation mirror the functionality of _wstat64 except it doesn't require permission to access the file. I will keep it for now until three is a need to consider other file extension.
| } | ||
|
|
||
| int | ||
| file_attr_to_st_mode(wchar_t * path, DWORD attributes) |
There was a problem hiding this comment.
I think we can use stat, st_mode for doing all this?
https://msdn.microsoft.com/en-us/library/14h5k7ff.aspx
https://msdn.microsoft.com/en-us/library/3kyc8381.aspx
There was a problem hiding this comment.
The change is to replace the usage of stat because stat require some (read) permission. When the user doesn't have the permission, stat return file_not_found. that is the reason we don't want to use stat.
| r = _wstat64(wpath, buf); | ||
|
|
||
| /* | ||
| * If we doesn't have sufficient permissions then _wstat64() is returning "file not found" |
There was a problem hiding this comment.
I would suggest to remove all this code and have _wstat64() but before making that attempt call GetFileAttributesExW() since we are just worried about the wrong error message.. we shouldn't be re-writing _wstat64() implementation again? If GetFileAttributesExW() success then call _wstat64
There was a problem hiding this comment.
No, with GetFileAttributesExW, we don't need to have that permission. GetFileAttributesExW success does not mean you can call _wstat64.
|
@bagajjal please review my response to your feedback and let me know your think. Thanks! |
| @@ -542,18 +542,22 @@ function UnInstall-OpenSSH | |||
| [string]$OpenSSHDir = "$env:SystemDrive\OpenSSH" | |||
| ) | |||
|
|
|||
| if (-not (Test-Path $OpenSSHDir)) | |||
There was a problem hiding this comment.
this should be independent of stopping the ssh-agent.. can you have this check down?
There was a problem hiding this comment.
this check is needed. if the folder does not exists, there is no need to do any of the below steps. The purpose of stopping ssh-agent is to run uninstall script. If the folder does not exists, there is no need to stop ssh-agent either.
PowerShell/Win32-OpenSSH#648
PowerShell/Win32-OpenSSH#718