Fix SCP stack overflow when copying over directories#565
Fix SCP stack overflow when copying over directories#565bagajjal merged 11 commits intoPowerShell:latestw_allfrom
Conversation
scp.c
Outdated
| #ifdef WINDOWS | ||
| while (len = snprintf(path, sizeof path, "D%04o %d %.1024s\n", | ||
| (u_int)(statp->st_mode & FILEMODEMASK), 0, last) >= path_len) { | ||
| path = xreallocarray(path, len + 1, sizeof(char)); |
There was a problem hiding this comment.
We wouldn't come here at all because snprintf writes at max sizeof(path) i.e., path_len?
There was a problem hiding this comment.
Have you checked this change by having a file / folder with more than 260 characters?
There was a problem hiding this comment.
snprintf returns the number of characters that would have been written if "path" was large enough to fit the whole string in that case. So we can compare path_len to the return value of snprintf to identify if we need to increase the size of "path".
scp.c
Outdated
| continue; | ||
| } | ||
| #ifdef WINDOWS | ||
| while (len = snprintf(path, sizeof path, "%s/%s", name, dp->d_name) >= path_len) { |
There was a problem hiding this comment.
Line 1457 seems to be doing wrong.. Here we are writing to path but at line we are checking the size as PATH_MAX
There was a problem hiding this comment.
If I understand correctly, I believe that what this line is trying to check is whether the path is of a valid size, so it throws an error if it's longer than "sizeof(path) - 1", which is equivalent to PATH_MAX - 1 in the original implementation. In our implementation, since we no longer have an array of size PATH_MAX as path, but we allocate up to PATH_MAX chars dynamically as needed, it makes more sense to compare the length of the path to PATH_MAX.
| #ifdef WINDOWS | ||
| if (!buf) | ||
| { | ||
| buf_len = ((strlen(last) + 20) < PATH_MAX) ? strlen(last) + 20 : PATH_MAX; |
There was a problem hiding this comment.
Based on what calculations, you are using 20 here?
There was a problem hiding this comment.
To be honest, not very precise calculations. Just by experience, "C%04o %lld", (u_int) (stb.st_mode & FILEMODEMASK), (long long)stb.st_size is less than 20 characters on all the tests I ran, I can't know for sure that this is always gonna be the case. I thought about just doing 2 * strlen(last), but I think it's more likely to have a filename with a very short name than the rest of the string to be more than 20 characters long.
2 participants
Fix: PowerShell/Win32-OpenSSH#1897
"SCP -r..." causes a stack overflow when "source" is deeper than a certain number of levels. That is caused because the stack frames for the source and rsource functions, which are called recursively, are too large due to local arrays of size PATH_MAX (defined as 32768 in Win32-OpenSSH). That problem is mitigated by this PR by dynamically allocating memory instead of declaring those arrays with such a large size.