On Sunday 02 December 2007 02:44, Denys Vlasenko wrote:
> Proposed patch uses user-supplied buffer directly,
> without intermediate on-stack copy.
> This can only make a difference if user supplied
> a buffer which is too small - thus user breaks API.
>
> Failure scenario:
>
> realpath("/link_name", user_buffer)
>
> /link_name -> /very_long_name_which_fits_into_PATH_MAX_and_is_also_a_link
> -> -> /shorter_name
>
> If user will give e.g. 40-char user_buffer, current implementation
> will work, patched one will overflow user_buffer by intermediate name.
>
> This should not be a problem - user must supply PATH_MAX sized buffer,
> and in this case patched version also works correctly.
And the following patch on top of previous one reuses copy_buf[]
for readlink, eliminating link_buf[]. In order to make it work,
"source" pathname is kept at the end of copy_buf, not at the
beginning (so that last NUL byte is the last byte of the copy_buf[]).
The situation when readlink returns link name which is too long
(so that it overwrites pathname), was resulting in ENAMETOOLONG
error return. This patch does the same - the fact the we now trash
pathname does not matter, as we are not returning it to the user.
Run tested.
Can somebody review these patches please?
--
vda
--- uClibc.t8/libc/stdlib/realpath.c 2007-12-02 02:18:09.000000000 -0800
+++ uClibc.t9/libc/stdlib/realpath.c 2007-12-02 02:59:11.000000000 -0800
@@ -53,11 +53,9 @@ char got_path[];
char copy_path[PATH_MAX];
/* use user supplied buffer directly - reduces stack usage */
/* char got_path[PATH_MAX]; */
-#ifdef S_IFLNK
- char link_path[PATH_MAX];
-#endif
const char *max_path;
char *new_path = got_path;
+ size_t path_len;
int readlinks = 0;
int n;
@@ -70,13 +68,15 @@ char got_path[];
return NULL;
}
/* Make a copy of the source path since we may need to modify it. */
- if (strlen(path) >= PATH_MAX - 2) {
+ path_len = strlen(path);
+ if (path_len >= PATH_MAX - 2) {
__set_errno(ENAMETOOLONG);
return NULL;
}
- strcpy(copy_path, path);
- path = copy_path;
- max_path = copy_path + PATH_MAX - 2;
+ /* Copy so that data is at the end of copy_path[] */
+ strcpy(copy_path + (PATH_MAX-1) - path_len, path);
+ path = copy_path + (PATH_MAX-1) - path_len;
+ max_path = copy_path + PATH_MAX - 2; /* points to last non-NUL char */
/* If it's a relative pathname use getcwd for starters. */
if (*path != '/') {
/* Ohoo... */
@@ -127,9 +127,10 @@ char got_path[];
__set_errno(ELOOP);
return NULL;
}
+ path_len = strlen(path);
/* See if last (so far) pathname component is a symlink. */
*new_path = '\0';
- n = readlink(got_path, link_path, PATH_MAX - 1);
+ n = readlink(got_path, copy_path, PATH_MAX - 1);
if (n < 0) {
/* EINVAL means the file exists but isn't a symlink. */
if (errno != EINVAL) {
@@ -138,22 +139,21 @@ char got_path[];
}
} else {
/* Safe sex check. */
- if (strlen(path) + n >= PATH_MAX - 2) {
+ if (path_len + n >= PATH_MAX - 2) {
__set_errno(ENAMETOOLONG);
return NULL;
}
/* Note: readlink doesn't add the null byte. */
- link_path[n] = '\0';
- if (*link_path == '/')
+ copy_path[n] = '\0';
+ if (*copy_path == '/')
/* Start over for an absolute symlink. */
new_path = got_path;
else
/* Otherwise back up over this component. */
while (*(--new_path) != '/');
- /* Insert symlink contents into path. */
- strcpy(link_path + n, path);
- strcpy(copy_path, link_path);
- path = copy_path;
+ /* Prepend symlink contents to path. */
+ memmove(copy_path + (PATH_MAX-1) - n - path_len, copy_path, n);
+ path = copy_path + (PATH_MAX-1) - n - path_len;
}
#endif /* S_IFLNK */
*new_path++ = '/';
_______________________________________________
uClibc mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/uclibc
