fix for issue 685 #122
fix for issue 685 #122
Conversation
ssh-keygen prompt the user if he/she want to override the existing file, but exit
contrib/win32/win32compat/misc.c
Outdated
| cp++; | ||
| } while ((actual_read < n - 1) && *str_tmp != '\n'); | ||
|
|
||
| if (actual_read > n - 1) { | ||
| /* shouldn't happen. but handling in case */ | ||
| errno = EINVAL; |
contrib/win32/win32compat/misc.c
Outdated
| } | ||
| else | ||
| break; | ||
| cp++; |
There was a problem hiding this comment.
cp should be incremented by the strlen(str_tmp)
contrib/win32/win32compat/misc.c
Outdated
| } | ||
|
|
||
| if((actual_read + strlen(str_tmp)) < n) { | ||
| memcpy(cp, str_tmp, strlen(str_tmp) + 1); |
There was a problem hiding this comment.
why +1? is it for '\0'?
If yes then we can do it in the end before returning.. not required for every byte.. this might confuse us.
| @@ -283,33 +283,49 @@ char* | |||
| w32_fgets(char *str, int n, FILE *stream) { | |||
| HANDLE h = (HANDLE)_get_osfhandle(_fileno(stream)); | |||
| wchar_t* str_w = NULL; | |||
| char *ret = NULL, *str_tmp = NULL; | |||
| char *ret = NULL, *str_tmp = NULL, *cp = NULL; | |||
There was a problem hiding this comment.
ret is not required. we can return str itself?
There was a problem hiding this comment.
No, according to https://linux.die.net/man/3/fgets, only return str on success; will return NULL on error
There was a problem hiding this comment.
I mean, ret variable is not required.. we can still use str variable itself..at the start of the function initialize str to NULL and everywhere return str..
Its a minor comment, fix as you like.
There was a problem hiding this comment.
No, str is pre-allocated memory in caller function
contrib/win32/win32compat/misc.c
Outdated
| goto cleanup; | ||
| } | ||
|
|
||
| if((actual_read + strlen(str_tmp)) < n) { |
There was a problem hiding this comment.
I think we can remove this if/else and add it as part of the while condition..
} while ( (actual_read = (actual_read + strlen(str_tmp)) && *str_tmp != '\n');
though line 321 is required.
There was a problem hiding this comment.
No, we need to check it before copying. strlen(str_tmp) may be more than the remaining length of str, we may copy more than n.
| @@ -283,33 +283,49 @@ char* | |||
| w32_fgets(char *str, int n, FILE *stream) { | |||
| HANDLE h = (HANDLE)_get_osfhandle(_fileno(stream)); | |||
| wchar_t* str_w = NULL; | |||
| char *ret = NULL, *str_tmp = NULL; | |||
| char *ret = NULL, *str_tmp = NULL, *cp = NULL; | |||
There was a problem hiding this comment.
I mean, ret variable is not required.. we can still use str variable itself..at the start of the function initialize str to NULL and everywhere return str..
Its a minor comment, fix as you like.
bingbing8
changed the title
bingbing8
changed the title
| free(str_tmp); | ||
| if (fgetws(str_w, 2, stream) == NULL) | ||
| goto cleanup; | ||
| if ((str_tmp = utf16_to_utf8(str_w)) == NULL) { |
There was a problem hiding this comment.
utf16_to_utf8 will alloc memory for each call. To optimize, you may create a static buffer (to accommodate 2 utf-8 character, max 8 bytes) and call WideCharToMultiByte directly)
| do { | ||
| if (str_tmp) | ||
| free(str_tmp); | ||
| if (fgetws(str_w, 2, stream) == NULL) |
There was a problem hiding this comment.
This will not work for UTF-16 charactors that will occupy 2 wide chars (or 4 bytes)
fix for issue that ssh-keygen exit after prompt the user if he/she want to override the existing file.
PowerShell/Win32-OpenSSH#685