* [PATCH lttng-tools 2/2] Fix: lttng-crash: remove tmp working directory
[not found] <1441317127-10878-1-git-send-email-jonathan.rajotte-julien@efficios.com>
@ 2015-09-03 21:52 ` Jonathan Rajotte
2015-09-05 16:19 ` [PATCH lttng-tools 1/2] Fix: lttng-crash: fd leak Jérémie Galarneau
[not found] ` <1441317127-10878-2-git-send-email-jonathan.rajotte-julien@efficios.com>
2 siblings, 0 replies; 3+ messages in thread
From: Jonathan Rajotte @ 2015-09-03 21:52 UTC (permalink / raw
To: lttng-dev; +Cc: jgalar
Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
src/bin/lttng-crash/lttng-crash.c | 65 +++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 17 deletions(-)
diff --git a/src/bin/lttng-crash/lttng-crash.c b/src/bin/lttng-crash/lttng-crash.c
index 8e57034..00a97df 100644
--- a/src/bin/lttng-crash/lttng-crash.c
+++ b/src/bin/lttng-crash/lttng-crash.c
@@ -1053,35 +1053,62 @@ end:
}
static
-int delete_trace(const char *trace_path)
+int delete_recursive_dir(const char *path)
{
- DIR *trace_dir;
- int trace_dir_fd, ret = 0, closeret;
+ DIR *dir;
+ int dir_fd, ret = 0, closeret;
struct dirent *entry;
/* Open trace directory */
- trace_dir = opendir(trace_path);
- if (!trace_dir) {
- PERROR("Cannot open '%s' path", trace_path);
+ dir = opendir(path);
+ if (!dir) {
+ PERROR("Cannot open '%s' path", path);
return -1;
}
- trace_dir_fd = dirfd(trace_dir);
- if (trace_dir_fd < 0) {
+ dir_fd = dirfd(dir);
+ if (dir_fd < 0) {
PERROR("dirfd");
return -1;
}
- while ((entry = readdir(trace_dir))) {
+ while ((entry = readdir(dir))) {
if (!strcmp(entry->d_name, ".")
|| !strcmp(entry->d_name, "..")) {
continue;
}
switch (entry->d_type) {
case DT_DIR:
- unlinkat(trace_dir_fd, entry->d_name, AT_REMOVEDIR);
+ {
+ char subpath[PATH_MAX];
+
+ strncpy(subpath, path,
+ sizeof(subpath));
+ subpath[sizeof(subpath) - 1] = '\0';
+ strncat(subpath, "/",
+ sizeof(subpath) - strlen(subpath) - 1);
+ strncat(subpath, entry->d_name,
+ sizeof(subpath) - strlen(subpath) - 1);
+
+ ret = delete_recursive_dir(subpath);
+ if (ret) {
+ goto end;
+ }
+
+ ret = unlinkat(dir_fd, entry->d_name, AT_REMOVEDIR);
+ if (ret) {
+ PERROR("Unlinking '%s'", entry->d_name);
+ goto end;
+ }
+
break;
+ }
case DT_REG:
- unlinkat(trace_dir_fd, entry->d_name, 0);
+ ret = unlinkat(dir_fd, entry->d_name, 0);
+ if (ret) {
+ PERROR("Unlinking '%s'", entry->d_name);
+ goto end;
+ }
+
break;
default:
ret = -EINVAL;
@@ -1089,13 +1116,10 @@ int delete_trace(const char *trace_path)
}
}
end:
- closeret = closedir(trace_dir);
+ closeret = closedir(dir);
if (closeret) {
PERROR("closedir");
}
- if (!ret) {
- ret = rmdir(trace_path);
- }
return ret;
}
@@ -1178,9 +1202,16 @@ int main(int argc, char *argv[])
has_warning = 1;
/* unlink temporary trace */
- ret = delete_trace(output_path);
- if (ret)
+ ret = delete_recursive_dir(output_path);
+ if (ret){
has_warning = 1;
+ }else{
+ ret = rmdir(output_path);
+ if (ret) {
+ PERROR("Deleting '%s'", output_path);
+ has_warning = 1;
+ }
+ }
}
if (has_warning)
exit(EXIT_FAILURE);
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH lttng-tools 1/2] Fix: lttng-crash: fd leak
[not found] <1441317127-10878-1-git-send-email-jonathan.rajotte-julien@efficios.com>
2015-09-03 21:52 ` [PATCH lttng-tools 2/2] Fix: lttng-crash: remove tmp working directory Jonathan Rajotte
@ 2015-09-05 16:19 ` Jérémie Galarneau
[not found] ` <1441317127-10878-2-git-send-email-jonathan.rajotte-julien@efficios.com>
2 siblings, 0 replies; 3+ messages in thread
From: Jérémie Galarneau @ 2015-09-05 16:19 UTC (permalink / raw
To: Jonathan Rajotte; +Cc: lttng-dev@lists.lttng.org, Jeremie Galarneau
Merged with modifications.
While this does correct the leak in the common case, it will also leak
when the "error" label is jumped to or close an uninitialized file
descriptor. The edited version I have merged takes care of both
issues.
Thanks!
Jérémie
On Thu, Sep 3, 2015 at 5:52 PM, Jonathan Rajotte
<jonathan.rajotte-julien@efficios.com> wrote:
> At the same time make sure to have on one exit-point.
>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
> src/bin/lttng-crash/lttng-crash.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/src/bin/lttng-crash/lttng-crash.c b/src/bin/lttng-crash/lttng-crash.c
> index 4f1efe9..8e57034 100644
> --- a/src/bin/lttng-crash/lttng-crash.c
> +++ b/src/bin/lttng-crash/lttng-crash.c
> @@ -103,7 +103,7 @@ enum rb_modes {
>
> struct crash_abi_unknown {
> uint8_t magic[RB_CRASH_DUMP_ABI_MAGIC_LEN];
> - uint64_t mmap_length; /* Overall lenght of crash record */
> + uint64_t mmap_length; /* Overall length of crash record */
> uint16_t endian; /*
> * { 0x12, 0x34 }: big endian
> * { 0x34, 0x12 }: little endian
> @@ -326,26 +326,30 @@ int copy_file(const char *file_dest, const char *file_src)
> int fd_src, fd_dest;
> ssize_t readlen, writelen;
> char buf[COPY_BUFLEN];
> + int ret;
>
> DBG("Copy metadata file '%s' into '%s'", file_src, file_dest);
>
> fd_src = open(file_src, O_RDONLY);
> if (fd_src < 0) {
> PERROR("Error opening %s for reading", file_src);
> - return fd_src;
> + ret = fd_src;
> + goto error;
> }
> fd_dest = open(file_dest, O_RDWR | O_CREAT | O_EXCL,
> S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
> if (fd_dest < 0) {
> PERROR("Error opening %s for writing", file_dest);
> - return fd_dest;
> + ret = fd_dest;
> + goto error;
> }
>
> for (;;) {
> readlen = lttng_read(fd_src, buf, COPY_BUFLEN);
> if (readlen < 0) {
> PERROR("Error reading input file");
> - return -1;
> + ret = -1;
> + goto error;
> }
> if (!readlen) {
> break;
> @@ -353,10 +357,26 @@ int copy_file(const char *file_dest, const char *file_src)
> writelen = lttng_write(fd_dest, buf, readlen);
> if (writelen < readlen) {
> PERROR("Error writing to output file");
> - return -1;
> + ret = -1;
> + goto error;
> }
> }
> +
> + ret = close(fd_src);
> + if (ret < 0) {
> + PERROR("Error closing %s", file_src);
> + goto error;
> + }
> +
> + ret = close(fd_dest);
> + if (ret < 0) {
> + PERROR("Error closing %s", file_dest);
> + goto error;
> + }
> +
> return 0;
> +error:
> + return ret;
> }
>
> static
> --
> 2.1.4
>
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH lttng-tools 2/2] Fix: lttng-crash: remove tmp working directory
[not found] ` <1441317127-10878-2-git-send-email-jonathan.rajotte-julien@efficios.com>
@ 2015-09-05 18:54 ` Jérémie Galarneau
0 siblings, 0 replies; 3+ messages in thread
From: Jérémie Galarneau @ 2015-09-05 18:54 UTC (permalink / raw
To: Jonathan Rajotte; +Cc: lttng-dev@lists.lttng.org, Jeremie Galarneau
Merged with modifications, see below.
Thanks!
Jérémie
On Thu, Sep 3, 2015 at 5:52 PM, Jonathan Rajotte
<jonathan.rajotte-julien@efficios.com> wrote:
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
> src/bin/lttng-crash/lttng-crash.c | 65 +++++++++++++++++++++++++++++----------
> 1 file changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/src/bin/lttng-crash/lttng-crash.c b/src/bin/lttng-crash/lttng-crash.c
> index 8e57034..00a97df 100644
> --- a/src/bin/lttng-crash/lttng-crash.c
> +++ b/src/bin/lttng-crash/lttng-crash.c
> @@ -1053,35 +1053,62 @@ end:
> }
>
> static
> -int delete_trace(const char *trace_path)
> +int delete_recursive_dir(const char *path)
Renamed to "delete_dir_recursive".
> {
> - DIR *trace_dir;
> - int trace_dir_fd, ret = 0, closeret;
> + DIR *dir;
> + int dir_fd, ret = 0, closeret;
> struct dirent *entry;
>
> /* Open trace directory */
> - trace_dir = opendir(trace_path);
> - if (!trace_dir) {
> - PERROR("Cannot open '%s' path", trace_path);
> + dir = opendir(path);
> + if (!dir) {
> + PERROR("Cannot open '%s' path", path);
> return -1;
> }
> - trace_dir_fd = dirfd(trace_dir);
> - if (trace_dir_fd < 0) {
> + dir_fd = dirfd(dir);
> + if (dir_fd < 0) {
> PERROR("dirfd");
> return -1;
> }
Not introduced by your patch, but this will leak dir. I have fixed the
problem and modified your patch so that it applies cleanly on top of
it.
>
> - while ((entry = readdir(trace_dir))) {
> + while ((entry = readdir(dir))) {
> if (!strcmp(entry->d_name, ".")
> || !strcmp(entry->d_name, "..")) {
> continue;
> }
> switch (entry->d_type) {
> case DT_DIR:
> - unlinkat(trace_dir_fd, entry->d_name, AT_REMOVEDIR);
> + {
> + char subpath[PATH_MAX];
>
^ I changed this for dynamic allocation since "PATH_MAX" can become
quite large on some system configurations and we may "recurse" rather
deeply in a trace hierarchy.
> + strncpy(subpath, path,
> + sizeof(subpath));
> + subpath[sizeof(subpath) - 1] = '\0';
> + strncat(subpath, "/",
> + sizeof(subpath) - strlen(subpath) - 1);
> + strncat(subpath, entry->d_name,
> + sizeof(subpath) - strlen(subpath) - 1);
> +
> + ret = delete_recursive_dir(subpath);
> + if (ret) {
> + goto end;
> + }
> +
> + ret = unlinkat(dir_fd, entry->d_name, AT_REMOVEDIR);
> + if (ret) {
> + PERROR("Unlinking '%s'", entry->d_name);
> + goto end;
> + }
I have removed this unlinkat() to perform an rmdir() at the end to
always remove the "root" on which we are called.
This simplifies the caller in main() and this "case".
> +
> break;
> + }
> case DT_REG:
> - unlinkat(trace_dir_fd, entry->d_name, 0);
> + ret = unlinkat(dir_fd, entry->d_name, 0);
> + if (ret) {
> + PERROR("Unlinking '%s'", entry->d_name);
> + goto end;
> + }
> +
> break;
> default:
> ret = -EINVAL;
> @@ -1089,13 +1116,10 @@ int delete_trace(const char *trace_path)
> }
> }
> end:
> - closeret = closedir(trace_dir);
> + closeret = closedir(dir);
> if (closeret) {
> PERROR("closedir");
> }
> - if (!ret) {
> - ret = rmdir(trace_path);
> - }
> return ret;
> }
>
> @@ -1178,9 +1202,16 @@ int main(int argc, char *argv[])
> has_warning = 1;
>
> /* unlink temporary trace */
> - ret = delete_trace(output_path);
> - if (ret)
> + ret = delete_recursive_dir(output_path);
> + if (ret){
Missing space here ^.
> has_warning = 1;
> + }else{
Missing spaces around "else" here ^.
> + ret = rmdir(output_path);
> + if (ret) {
> + PERROR("Deleting '%s'", output_path);
> + has_warning = 1;
> + }
> + }
> }
> if (has_warning)
> exit(EXIT_FAILURE);
> --
> 2.1.4
>
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-05 18:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1441317127-10878-1-git-send-email-jonathan.rajotte-julien@efficios.com>
2015-09-03 21:52 ` [PATCH lttng-tools 2/2] Fix: lttng-crash: remove tmp working directory Jonathan Rajotte
2015-09-05 16:19 ` [PATCH lttng-tools 1/2] Fix: lttng-crash: fd leak Jérémie Galarneau
[not found] ` <1441317127-10878-2-git-send-email-jonathan.rajotte-julien@efficios.com>
2015-09-05 18:54 ` [PATCH lttng-tools 2/2] Fix: lttng-crash: remove tmp working directory Jérémie Galarneau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.