From mboxrd@z Thu Jan 1 00:00:00 1970 From: mathieu.desnoyers@efficios.com (Mathieu Desnoyers) Date: Thu, 14 Nov 2013 14:10:14 +0000 (UTC) Subject: [lttng-dev] [lttng-tools PATCH 3/4] Correct the behavior of the utils_expand_path function In-Reply-To: <1384320878-28808-4-git-send-email-raphael.beamonte@gmail.com> References: <1384320878-28808-1-git-send-email-raphael.beamonte@gmail.com> <20131112195134.GB12183@thessa> <1384320878-28808-4-git-send-email-raphael.beamonte@gmail.com> Message-ID: <722861522.67118.1384438214771.JavaMail.zimbra@efficios.com> Here are comments about the patch, even though it has been merged. ----- Original Message ----- > From: "Rapha?l Beamonte" > To: dgoulet at efficios.com > Cc: "Rapha?l Beamonte" , lttng-dev at lists.lttng.org > 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 > --- > 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 > lttng-dev at lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com