Here are comments about the patch, even though it has been merged.
----- Original Message -----
> From: "Raphaël Beamonte" <[email protected]>
> To: [email protected]
> Cc: "Raphaël Beamonte" <[email protected]>, [email protected]
> Sent: Wednesday, November 13, 2013 12:34:37 AM
> Subject: [lttng-dev] [lttng-tools PATCH 3/4] Correct the behavior of the
> utils_expand_path function
>
> Even if the utils_expand_path function was intended to allow to
> use unexistent directory paths, it was in fact only working for
> some kind of arguments. Paths like "foo", "bar/" or "bar/foo"
> when the "bar" directory does not exist wasn't working. This
> patch introduce a new way to expand paths in this function that
> also works properly for these kind of arguments.
>
> Signed-off-by: Raphaël Beamonte <[email protected]>
> ---
> src/common/utils.c | 86
> ++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 64 insertions(+), 22 deletions(-)
>
> diff --git a/src/common/utils.c b/src/common/utils.c
> index 4ccf36a..6938a5a 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -112,7 +112,7 @@ error:
> LTTNG_HIDDEN
> char *utils_expand_path(const char *path)
> {
> - const char *end_path = path;
> + const char *end_path = NULL;
> char *next, *cut_path = NULL, *expanded_path = NULL;
>
> /* Safety net */
> @@ -120,38 +120,80 @@ char *utils_expand_path(const char *path)
> goto error;
> }
>
> - /* Find last token delimited by '/' */
> - while ((next = strpbrk(end_path + 1, "/"))) {
> - end_path = next;
> - }
> -
> - /* Cut last token from original path */
> - cut_path = strndup(path, end_path - path);
> -
> + /* Allocate memory for the expanded path */
> expanded_path = zmalloc(PATH_MAX);
> if (expanded_path == NULL) {
> PERROR("zmalloc expand path");
> goto error;
> }
>
> - expanded_path = realpath((char *)cut_path, expanded_path);
> - if (expanded_path == NULL) {
> - switch (errno) {
> - case ENOENT:
> - ERR("%s: No such file or directory", cut_path);
> - break;
> - default:
> - PERROR("realpath utils expand path");
> - break;
> + /* If given path is already absolute */
> + if (*path == '/') {
> + strncpy(expanded_path, path, PATH_MAX);
> + /* Else, we have some work to do */
> + } else {
> + /* Pointer to the last char of the path */
> + const char *last_char = path + strlen(path) - 1;
> +
> + end_path = path;
> +
> + /* Split part that will be resolved by realpath (relative path
> from
Comments should start with a:
/*
* Then start of comment...
> + * current directory using ./ or ../ only) and part that could
> not
> + * (directory names)
> + */
> + while ((next = strpbrk(end_path, "/")) && (next != last_char)) {
> + end_path = next + 1;
> + if (strncmp(end_path, "./", 2) != 0 &&
> + strncmp(end_path, "../", 3) != 0) {
> + break;
> + }
> + }
> +
> + /* If this is the end of the string, and we still can resolve
> it */
> + if (strncmp(end_path, "..\0", 3) == 0 ||
There is no point in ending a "" with \0, it implicitly ends with a \0 already.
> + strncmp(end_path, ".\0", 2) == 0) {
Same here.
> + end_path += strlen(end_path);
> + }
> +
> + /* If the end part is the whole path, we are in the current dir
> */
Not necessarily. What happens in the unlikely case someone passes a path with
simply "/" ?
> + if (end_path == path) {
> + cut_path = strdup(".");
> + /* Else, cut the resolvable part from original path */
> + } else {
> + cut_path = strndup(path, end_path - path);
> + }
> +
> + /* Resolve the canonical path of the first part of the path */
> + expanded_path = realpath((char *)cut_path, expanded_path);
So let's say we have:
"../blah/../blah"
so only "../" passed to realpath, or the entire "../blah/../blah" ? (let's
suppose blah/ exists), right ?
> + if (expanded_path == NULL) {
> + switch (errno) {
> + case ENOENT:
> + ERR("%s: No such file or directory", cut_path);
> + break;
> + default:
> + PERROR("realpath utils expand path");
> + break;
> + }
> + goto error;
> + }
> +
> + /* Add end part to expanded path if not empty */
> + if (*end_path != 0) {
> + strncat(expanded_path, "/", PATH_MAX -
> strlen(expanded_path) - 1);
> + strncat(expanded_path, end_path,
> + PATH_MAX - strlen(expanded_path) - 1);
> }
> - goto error;
> }
>
> - /* Add end part to expanded path */
> - strncat(expanded_path, end_path, PATH_MAX - strlen(expanded_path) - 1);
> + /* Resolve the internal './' and '../' strings */
> + next = utils_resolve_relative(expanded_path);
In the "../blah/../blah" case, here the other ../ would be dealt with by
utils_resolve_relative, is that it ?
Thanks,
Mathieu
> + if (next == NULL) {
> + goto error;
> + }
>
> + free(expanded_path);
> free(cut_path);
> - return expanded_path;
> + return next;
>
> error:
> free(expanded_path);
> --
> 1.7.10.4
>
>
> _______________________________________________
> lttng-dev mailing list
> [email protected]
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
[email protected]
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
