Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1461477 > unrolled thread
| Started by | Colin King <colin.king@canonical.com> |
|---|---|
| First post | 2016-08-12 23:50 +0200 |
| Last post | 2016-08-16 20:20 +0200 |
| Articles | 4 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] perf probe: check for dup and fdopen failures Colin King <colin.king@canonical.com> - 2016-08-12 23:50 +0200
Re: [PATCH] perf probe: check for dup and fdopen failures Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-08-13 00:00 +0200
Re: [PATCH] perf probe: check for dup and fdopen failures Masami Hiramatsu <mhiramat@kernel.org> - 2016-08-13 02:00 +0200
[tip:perf/urgent] perf probe: Check for dup and fdopen failures tip-bot for Colin Ian King <tipbot@zytor.com> - 2016-08-16 20:20 +0200
| From | Colin King <colin.king@canonical.com> |
|---|---|
| Date | 2016-08-12 23:50 +0200 |
| Subject | [PATCH] perf probe: check for dup and fdopen failures |
| Message-ID | <s5suZ-7jU-17@gated-at.bofh.it> |
From: Colin Ian King <colin.king@canonical.com>
dup and fdopen can potentially fail, so add some extra
error handling checks rather than assuming they always work.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
tools/perf/util/probe-file.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 9aed9c3..f3df939 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -133,7 +133,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag)
/* Get raw string list of current kprobe_events or uprobe_events */
struct strlist *probe_file__get_rawlist(int fd)
{
- int ret, idx;
+ int ret, idx, fddup;
FILE *fp;
char buf[MAX_CMDLEN];
char *p;
@@ -144,7 +144,12 @@ struct strlist *probe_file__get_rawlist(int fd)
sl = strlist__new(NULL, NULL);
- fp = fdopen(dup(fd), "r");
+ fddup = dup(fd);
+ if (fddup < 0)
+ return NULL;
+ fp = fdopen(fddup, "r");
+ if (!fp)
+ return NULL;
while (!feof(fp)) {
p = fgets(buf, MAX_CMDLEN, fp);
if (!p)
@@ -447,10 +452,13 @@ static int probe_cache__load(struct probe_cache *pcache)
{
struct probe_cache_entry *entry = NULL;
char buf[MAX_CMDLEN], *p;
- int ret = 0;
+ int ret = 0, fddup;
FILE *fp;
- fp = fdopen(dup(pcache->fd), "r");
+ fddup = dup(pcache->fd);
+ if (fddup < 0)
+ return -errno;
+ fp = fdopen(fddup, "r");
if (!fp)
return -EINVAL;
--
2.8.1
[toc] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2016-08-13 00:00 +0200 |
| Message-ID | <s5sEG-7nQ-27@gated-at.bofh.it> |
| In reply to | #1461477 |
Em Fri, Aug 12, 2016 at 10:44:56PM +0100, Colin King escreveu:
> From: Colin Ian King <colin.king@canonical.com>
>
> dup and fdopen can potentially fail, so add some extra
> error handling checks rather than assuming they always work.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> tools/perf/util/probe-file.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 9aed9c3..f3df939 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -133,7 +133,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag)
> /* Get raw string list of current kprobe_events or uprobe_events */
> struct strlist *probe_file__get_rawlist(int fd)
> {
> - int ret, idx;
> + int ret, idx, fddup;
> FILE *fp;
> char buf[MAX_CMDLEN];
> char *p;
> @@ -144,7 +144,12 @@ struct strlist *probe_file__get_rawlist(int fd)
>
> sl = strlist__new(NULL, NULL);
>
> - fp = fdopen(dup(fd), "r");
> + fddup = dup(fd);
> + if (fddup < 0)
> + return NULL;
> + fp = fdopen(fddup, "r");
> + if (!fp)
here we leak fddup, no?
> + return NULL;
I.e. this should be:
if (!fp) {
close(fddup);
return NULL;
}
Ack?
- Arnaldo
> while (!feof(fp)) {
> p = fgets(buf, MAX_CMDLEN, fp);
> if (!p)
> @@ -447,10 +452,13 @@ static int probe_cache__load(struct probe_cache *pcache)
> {
> struct probe_cache_entry *entry = NULL;
> char buf[MAX_CMDLEN], *p;
> - int ret = 0;
> + int ret = 0, fddup;
> FILE *fp;
>
> - fp = fdopen(dup(pcache->fd), "r");
> + fddup = dup(pcache->fd);
> + if (fddup < 0)
> + return -errno;
> + fp = fdopen(fddup, "r");
> if (!fp)
> return -EINVAL;
>
> --
> 2.8.1
[toc] | [prev] | [next] | [standalone]
| From | Masami Hiramatsu <mhiramat@kernel.org> |
|---|---|
| Date | 2016-08-13 02:00 +0200 |
| Message-ID | <s5uwN-ac-3@gated-at.bofh.it> |
| In reply to | #1461487 |
On Fri, 12 Aug 2016 18:55:17 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> Em Fri, Aug 12, 2016 at 10:44:56PM +0100, Colin King escreveu:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > dup and fdopen can potentially fail, so add some extra
> > error handling checks rather than assuming they always work.
Thanks, I forgot to handle that.
> >
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > tools/perf/util/probe-file.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> > index 9aed9c3..f3df939 100644
> > --- a/tools/perf/util/probe-file.c
> > +++ b/tools/perf/util/probe-file.c
> > @@ -133,7 +133,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag)
> > /* Get raw string list of current kprobe_events or uprobe_events */
> > struct strlist *probe_file__get_rawlist(int fd)
> > {
> > - int ret, idx;
> > + int ret, idx, fddup;
> > FILE *fp;
> > char buf[MAX_CMDLEN];
> > char *p;
> > @@ -144,7 +144,12 @@ struct strlist *probe_file__get_rawlist(int fd)
> >
> > sl = strlist__new(NULL, NULL);
> >
> > - fp = fdopen(dup(fd), "r");
> > + fddup = dup(fd);
> > + if (fddup < 0)
> > + return NULL;
> > + fp = fdopen(fddup, "r");
> > + if (!fp)
>
> here we leak fddup, no?
>
> > + return NULL;
>
> I.e. this should be:
>
> if (!fp) {
> close(fddup);
> return NULL;
> }
>
> Ack?
Correct, it should be closed. So both fixes look good to me.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thanks!
>
> - Arnaldo
>
> > while (!feof(fp)) {
> > p = fgets(buf, MAX_CMDLEN, fp);
> > if (!p)
> > @@ -447,10 +452,13 @@ static int probe_cache__load(struct probe_cache *pcache)
> > {
> > struct probe_cache_entry *entry = NULL;
> > char buf[MAX_CMDLEN], *p;
> > - int ret = 0;
> > + int ret = 0, fddup;
> > FILE *fp;
> >
> > - fp = fdopen(dup(pcache->fd), "r");
> > + fddup = dup(pcache->fd);
> > + if (fddup < 0)
> > + return -errno;
> > + fp = fdopen(fddup, "r");
> > if (!fp)
> > return -EINVAL;
> >
> > --
> > 2.8.1
--
Masami Hiramatsu <mhiramat@kernel.org>
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Colin Ian King <tipbot@zytor.com> |
|---|---|
| Date | 2016-08-16 20:20 +0200 |
| Subject | [tip:perf/urgent] perf probe: Check for dup and fdopen failures |
| Message-ID | <s6R7Y-6tU-33@gated-at.bofh.it> |
| In reply to | #1461477 |
Commit-ID: 0325862dc364d8af524bf2db53ef4360ed55b989
Gitweb: http://git.kernel.org/tip/0325862dc364d8af524bf2db53ef4360ed55b989
Author: Colin Ian King <colin.king@canonical.com>
AuthorDate: Fri, 12 Aug 2016 22:44:56 +0100
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 15 Aug 2016 17:06:19 -0300
perf probe: Check for dup and fdopen failures
dup and fdopen can potentially fail, so add some extra
error handling checks rather than assuming they always work.
Signed-off-by: Colin King <colin.king@canonical.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1471038296-12956-1-git-send-email-colin.king@canonical.com
[ Free resources when those functions (now being verified) fail ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/probe-file.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index 9aed9c3..a8e7623 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -133,7 +133,7 @@ int probe_file__open_both(int *kfd, int *ufd, int flag)
/* Get raw string list of current kprobe_events or uprobe_events */
struct strlist *probe_file__get_rawlist(int fd)
{
- int ret, idx;
+ int ret, idx, fddup;
FILE *fp;
char buf[MAX_CMDLEN];
char *p;
@@ -144,7 +144,14 @@ struct strlist *probe_file__get_rawlist(int fd)
sl = strlist__new(NULL, NULL);
- fp = fdopen(dup(fd), "r");
+ fddup = dup(fd);
+ if (fddup < 0)
+ goto out_free_sl;
+
+ fp = fdopen(fddup, "r");
+ if (!fp)
+ goto out_close_fddup;
+
while (!feof(fp)) {
p = fgets(buf, MAX_CMDLEN, fp);
if (!p)
@@ -163,6 +170,12 @@ struct strlist *probe_file__get_rawlist(int fd)
fclose(fp);
return sl;
+
+out_close_fddup:
+ close(fddup);
+out_free_sl:
+ strlist__delete(sl);
+ return NULL;
}
static struct strlist *__probe_file__get_namelist(int fd, bool include_group)
@@ -447,10 +460,13 @@ static int probe_cache__load(struct probe_cache *pcache)
{
struct probe_cache_entry *entry = NULL;
char buf[MAX_CMDLEN], *p;
- int ret = 0;
+ int ret = 0, fddup;
FILE *fp;
- fp = fdopen(dup(pcache->fd), "r");
+ fddup = dup(pcache->fd);
+ if (fddup < 0)
+ return -errno;
+ fp = fdopen(fddup, "r");
if (!fp)
return -EINVAL;
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web