Skip to content

Fix SCP stack overflow when copying over directories #565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bagajjal merged 11 commits into from
Feb 18, 2022
Merged

Fix SCP stack overflow when copying over directories #565

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
57ec6ef
avoid local arrays of size PATH_MAX
vthiebaut10 Feb 16, 2022
d9149a1
Change xrealloc to xreallocarray
vthiebaut10 Feb 16, 2022
346e224
Add #ifdef blocks for my changes
vthiebaut10 Feb 16, 2022
2abdca2
Fixed a couple of things
vthiebaut10 Feb 16, 2022
bb42857
Address review comments
vthiebaut10 Feb 16, 2022
10950f4
Finish addressing review comments
vthiebaut10 Feb 16, 2022
ce2594c
Fix regression
vthiebaut10 Feb 16, 2022
0830d82
PATH_MAX vs MAX_PATH confusion
vthiebaut10 Feb 17, 2022
9949b9b
Fixed a potential memory leak
vthiebaut10 Feb 17, 2022
c0f51c8
add comment
vthiebaut10 Feb 17, 2022
a4b4ab4
fix a couple of accidental extra lines
vthiebaut10 Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 43 additions & 9 deletions scp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,8 +1262,9 @@ source(int argc, char **argv)
off_t i, statbytes;
size_t amt, nr;
int fd = -1, haderr, indx;
char *last, *name, buf[PATH_MAX + 128], encname[PATH_MAX];
char *last, *name, *buf, *encname;
Comment thread
vthiebaut10 marked this conversation as resolved.
Outdated
int len;
size_t encname_len, buf_len, tmp_len;

for (indx = 0; indx < argc; ++indx) {
name = argv[indx];
Expand All @@ -1274,7 +1275,15 @@ source(int argc, char **argv)
if ((fd = open(name, O_RDONLY|O_NONBLOCK, 0)) == -1)
goto syserr;
if (strchr(name, '\n') != NULL) {
strnvis(encname, name, sizeof(encname), VIS_NL);
if (!encname) {
encname = xmalloc(len + 1);
encname_len = len + 1;
}
while (tmp_len = strnvis(encname, name, encname_len, VIS_NL) > encname_len) {
// check if tmp_len is less than MAX_PATH?
Comment thread
vthiebaut10 marked this conversation as resolved.
Outdated
encname = xrealloc(encname, tmp_len + 1);
encname_len = tmp_len;
}
name = encname;
}
if (fstat(fd, &stb) == -1) {
Expand Down Expand Up @@ -1309,9 +1318,14 @@ syserr: run_err("%s: %s", name, strerror(errno));
goto next;
}
#define FILEMODEMASK (S_ISUID|S_ISGID|S_IRWXU|S_IRWXG|S_IRWXO)
snprintf(buf, sizeof buf, "C%04o %lld %s\n",
(u_int) (stb.st_mode & FILEMODEMASK),
(long long)stb.st_size, last);
buf_len = strlen(last) * 2;
Comment thread
vthiebaut10 marked this conversation as resolved.
Outdated
buf = xmalloc(buf_len);
while (tmp_len = snprintf(buf, sizeof buf, "C%04o %lld %s\n",
(u_int) (stb.st_mode & FILEMODEMASK),
(long long)stb.st_size, last) >= buf_len) {
buf = xrealloc(buf, tmp_len + 1);
buf_len = tmp_len + 1;
}
if (verbose_mode)
fmprintf(stderr, "Sending file modes: %s", buf);
(void) atomicio(vwrite, remout, buf, strlen(buf));
Expand Down Expand Up @@ -1363,14 +1377,21 @@ next: if (fd != -1) {
if (showprogress)
stop_progress_meter();
}
if (encname)
free(encname);
if (buf)
free(buf);
}

void
rsource(char *name, struct stat *statp)
{
DIR *dirp;
struct dirent *dp;
char *last, *vect[1], path[PATH_MAX];
char *last, *vect[1], *path;
size_t path_len = 260, len;

path = xmalloc(path_len);

if (!(dirp = opendir(name))) {
run_err("%s: %s", name, strerror(errno));
Expand All @@ -1387,8 +1408,13 @@ rsource(char *name, struct stat *statp)
return;
}
}
(void) snprintf(path, sizeof path, "D%04o %d %.1024s\n",
(u_int) (statp->st_mode & FILEMODEMASK), 0, last);

while (len = snprintf(path, sizeof path, "D%04o %d %.1024s\n",
(u_int)(statp->st_mode & FILEMODEMASK), 0, last) >= path_len) {
path = xrealloc(path, len + 1);
path_len = len + 1;
}

if (verbose_mode)
fmprintf(stderr, "Entering directory: %s", path);
(void) atomicio(vwrite, remout, path, strlen(path));
Expand All @@ -1401,14 +1427,22 @@ rsource(char *name, struct stat *statp)
continue;
if (!strcmp(dp->d_name, ".") || !strcmp(dp->d_name, ".."))
continue;
if (strlen(name) + 1 + strlen(dp->d_name) >= sizeof(path) - 1) {
if (strlen(name) + 1 + strlen(dp->d_name) >= PATH_MAX - 1) {
run_err("%s/%s: name too long", name, dp->d_name);
continue;
}
(void) snprintf(path, sizeof path, "%s/%s", name, dp->d_name);

while (len = snprintf(path, sizeof path, "%s/%s", name, dp->d_name) >= path_len) {

@bagajjal bagajjal Feb 16, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 1457 seems to be doing wrong.. Here we are writing to path but at line we are checking the size as PATH_MAX

@vthiebaut10 vthiebaut10 Feb 16, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

path = xrealloc(path, len + 1);
path_len = len + 1;
}

vect[0] = path;
source(1, vect);
}
if (path)
free(path);
(void) closedir(dirp);
(void) atomicio(vwrite, remout, "E\n", 2);
(void) response();
Expand Down