2017-07-27 08:00:06

by ChunYu Wang

Subject: [PATCH] rpcdebug: Add proper free() to avoid memory leak

rpcdebug.c use char* cdename to store program name:

cdename = malloc(strlen(basename(argv[0])));
strcpy(cdename, basename(argv[0]));

It is better to free before exit to avoid potential leaks.

Signed-off-by: ChunYu Wang <[email protected]>
---
tools/rpcdebug/rpcdebug.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/tools/rpcdebug/rpcdebug.c b/tools/rpcdebug/rpcdebug.c
index 68206cc..722ab4d 100644
--- a/tools/rpcdebug/rpcdebug.c
+++ b/tools/rpcdebug/rpcdebug.c
@@ -131,6 +131,7 @@ main(int argc, char **argv)
print_flags(stdout, module, ~(unsigned int) 0, 1);
}

+ free(cdename);
return 0;
}

@@ -221,6 +222,7 @@ find_flag(char **module, char *name)
"please specify the module name using\n"
"the -m option.\n",
cdename, name);
+ free(cdename);
exit(1);
}
value = flagmap[i].value;
@@ -238,6 +240,7 @@ find_flag(char **module, char *name)
fprintf(stderr,
"%s: unknown flag %s\n",
cdename, name);
+ free(cdename);
exit(1);
}

@@ -255,10 +258,12 @@ get_flags(char *module)

if ((sysfd = open(filename, O_RDONLY)) < 0) {
perror(filename);
+ free(cdename);
exit(1);
}
if ((len = read(sysfd, buffer, sizeof(buffer))) < 0) {
perror("read");
+ free(cdename);
exit(1);
}
close(sysfd);
@@ -278,14 +283,17 @@ set_flags(char *module, unsigned int value)
len = sprintf(buffer, "%d", value);
if ((sysfd = open(filename, O_WRONLY)) < 0) {
perror(filename);
+ free(cdename);
exit(1);
}
if ((ret = write(sysfd, buffer, len)) < 0) {
perror("write");
+ free(cdename);
exit(1);
}
if (ret < len) {
fprintf(stderr, "error: short write in set_flags!\n");
+ free(cdename);
exit(1);
}
close(sysfd);
@@ -359,6 +367,7 @@ usage(int excode, char *module)
else
fprintf(stderr, " (use %s -vh to get a list of modules and valid flags)\n", cdename);
}
+ free(cdename);
exit (excode);
}

--
2.1.4

2017-07-27 08:44:08

by ChunYu Wang

Subject: Re: [PATCH] rpcdebug: Add proper free() to avoid memory leak

Multiple this kind of scenes can be found in nfs-utils and can be
reproduced easily with valgrind, if this kind of issue should be
resolved, maybe a series of similar patches are necessary.

---> Try to reproduce:
[chunwang@bootp rpcdebug]$ valgrind rpcdebug -h

---> valgrind report before patch:
==14199== HEAP SUMMARY:
==14199== in use at exit: 8 bytes in 1 blocks
==14199== total heap usage: 1 allocs, 0 frees, 8 bytes allocated
==14199==
==14199== LEAK SUMMARY:
==14199== definitely lost: 0 bytes in 0 blocks
==14199== indirectly lost: 0 bytes in 0 blocks
==14199== possibly lost: 0 bytes in 0 blocks
==14199== still reachable: 8 bytes in 1 blocks
==14199== suppressed: 0 bytes in 0 blocks

--> valgrind report after patch:
==14200==
==14200== HEAP SUMMARY:
==14200== in use at exit: 0 bytes in 0 blocks
==14200== total heap usage: 1 allocs, 1 frees, 8 bytes allocated
==14200==
==14200== All heap blocks were freed -- no leaks are possible

On Thu, Jul 27, 2017 at 3:59 PM, ChunYu Wang <[email protected]> wrote:
> rpcdebug.c use char* cdename to store program name:
>
> cdename = malloc(strlen(basename(argv[0])));
> strcpy(cdename, basename(argv[0]));
>
> It is better to free before exit to avoid potential leaks.
>
> Signed-off-by: ChunYu Wang <[email protected]>
> ---
> tools/rpcdebug/rpcdebug.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/tools/rpcdebug/rpcdebug.c b/tools/rpcdebug/rpcdebug.c
> index 68206cc..722ab4d 100644
> --- a/tools/rpcdebug/rpcdebug.c
> +++ b/tools/rpcdebug/rpcdebug.c
> @@ -131,6 +131,7 @@ main(int argc, char **argv)
> print_flags(stdout, module, ~(unsigned int) 0, 1);
> }
>
> + free(cdename);
> return 0;
> }
>
> @@ -221,6 +222,7 @@ find_flag(char **module, char *name)
> "please specify the module name using\n"
> "the -m option.\n",
> cdename, name);
> + free(cdename);
> exit(1);
> }
> value = flagmap[i].value;
> @@ -238,6 +240,7 @@ find_flag(char **module, char *name)
> fprintf(stderr,
> "%s: unknown flag %s\n",
> cdename, name);
> + free(cdename);
> exit(1);
> }
>
> @@ -255,10 +258,12 @@ get_flags(char *module)
>
> if ((sysfd = open(filename, O_RDONLY)) < 0) {
> perror(filename);
> + free(cdename);
> exit(1);
> }
> if ((len = read(sysfd, buffer, sizeof(buffer))) < 0) {
> perror("read");
> + free(cdename);
> exit(1);
> }
> close(sysfd);
> @@ -278,14 +283,17 @@ set_flags(char *module, unsigned int value)
> len = sprintf(buffer, "%d", value);
> if ((sysfd = open(filename, O_WRONLY)) < 0) {
> perror(filename);
> + free(cdename);
> exit(1);
> }
> if ((ret = write(sysfd, buffer, len)) < 0) {
> perror("write");
> + free(cdename);
> exit(1);
> }
> if (ret < len) {
> fprintf(stderr, "error: short write in set_flags!\n");
> + free(cdename);
> exit(1);
> }
> close(sysfd);
> @@ -359,6 +367,7 @@ usage(int excode, char *module)
> else
> fprintf(stderr, " (use %s -vh to get a list of modules and valid flags)\n", cdename);
> }
> + free(cdename);
> exit (excode);
> }
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2017-07-27 14:48:02

by Steve Dickson

Subject: Re: [PATCH] rpcdebug: Add proper free() to avoid memory leak

On 07/27/2017 03:59 AM, ChunYu Wang wrote:
> rpcdebug.c use char* cdename to store program name:
>
> cdename = malloc(strlen(basename(argv[0])));
> strcpy(cdename, basename(argv[0]));
>
> It is better to free before exit to avoid potential leaks.
I guess I don't understand how memory can be leaked
when a process is exiting... What am I missing?

steved.
>
> Signed-off-by: ChunYu Wang <[email protected]>
> ---
> tools/rpcdebug/rpcdebug.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/tools/rpcdebug/rpcdebug.c b/tools/rpcdebug/rpcdebug.c
> index 68206cc..722ab4d 100644
> --- a/tools/rpcdebug/rpcdebug.c
> +++ b/tools/rpcdebug/rpcdebug.c
> @@ -131,6 +131,7 @@ main(int argc, char **argv)
> print_flags(stdout, module, ~(unsigned int) 0, 1);
> }
>
> + free(cdename);
> return 0;
> }
>
> @@ -221,6 +222,7 @@ find_flag(char **module, char *name)
> "please specify the module name using\n"
> "the -m option.\n",
> cdename, name);
> + free(cdename);
> exit(1);
> }
> value = flagmap[i].value;
> @@ -238,6 +240,7 @@ find_flag(char **module, char *name)
> fprintf(stderr,
> "%s: unknown flag %s\n",
> cdename, name);
> + free(cdename);
> exit(1);
> }
>
> @@ -255,10 +258,12 @@ get_flags(char *module)
>
> if ((sysfd = open(filename, O_RDONLY)) < 0) {
> perror(filename);
> + free(cdename);
> exit(1);
> }
> if ((len = read(sysfd, buffer, sizeof(buffer))) < 0) {
> perror("read");
> + free(cdename);
> exit(1);
> }
> close(sysfd);
> @@ -278,14 +283,17 @@ set_flags(char *module, unsigned int value)
> len = sprintf(buffer, "%d", value);
> if ((sysfd = open(filename, O_WRONLY)) < 0) {
> perror(filename);
> + free(cdename);
> exit(1);
> }
> if ((ret = write(sysfd, buffer, len)) < 0) {
> perror("write");
> + free(cdename);
> exit(1);
> }
> if (ret < len) {
> fprintf(stderr, "error: short write in set_flags!\n");
> + free(cdename);
> exit(1);
> }
> close(sysfd);
> @@ -359,6 +367,7 @@ usage(int excode, char *module)
> else
> fprintf(stderr, " (use %s -vh to get a list of modules and valid flags)\n", cdename);
> }
> + free(cdename);
> exit (excode);
> }
>
>

2017-07-27 16:06:59

by ChunYu Wang

Subject: Re: [PATCH] rpcdebug: Add proper free() to avoid memory leak

Maybe we can not trust valgrind? Or I have just misunderstood what did
the valgrind report.

As I have just added enough free(cdename) statements before exit,
valgrind will not report "still reachable" any more.

Since I am not sure of whether this kind of report by valgrind should
be treated as potential leaks, please help me to judge!

Thanks,
- Chunyu

On Thu, Jul 27, 2017 at 10:48 PM, Steve Dickson <[email protected]> wrote:
>
>
> On 07/27/2017 03:59 AM, ChunYu Wang wrote:
>> rpcdebug.c use char* cdename to store program name:
>>
>> cdename = malloc(strlen(basename(argv[0])));
>> strcpy(cdename, basename(argv[0]));
>>
>> It is better to free before exit to avoid potential leaks.
> I guess I don't understand how memory can be leaked
> when a process is exiting... What am I missing?
>
> steved.
>>
>> Signed-off-by: ChunYu Wang <[email protected]>
>> ---
>> tools/rpcdebug/rpcdebug.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/tools/rpcdebug/rpcdebug.c b/tools/rpcdebug/rpcdebug.c
>> index 68206cc..722ab4d 100644
>> --- a/tools/rpcdebug/rpcdebug.c
>> +++ b/tools/rpcdebug/rpcdebug.c
>> @@ -131,6 +131,7 @@ main(int argc, char **argv)
>> print_flags(stdout, module, ~(unsigned int) 0, 1);
>> }
>>
>> + free(cdename);
>> return 0;
>> }
>>
>> @@ -221,6 +222,7 @@ find_flag(char **module, char *name)
>> "please specify the module name using\n"
>> "the -m option.\n",
>> cdename, name);
>> + free(cdename);
>> exit(1);
>> }
>> value = flagmap[i].value;
>> @@ -238,6 +240,7 @@ find_flag(char **module, char *name)
>> fprintf(stderr,
>> "%s: unknown flag %s\n",
>> cdename, name);
>> + free(cdename);
>> exit(1);
>> }
>>
>> @@ -255,10 +258,12 @@ get_flags(char *module)
>>
>> if ((sysfd = open(filename, O_RDONLY)) < 0) {
>> perror(filename);
>> + free(cdename);
>> exit(1);
>> }
>> if ((len = read(sysfd, buffer, sizeof(buffer))) < 0) {
>> perror("read");
>> + free(cdename);
>> exit(1);
>> }
>> close(sysfd);
>> @@ -278,14 +283,17 @@ set_flags(char *module, unsigned int value)
>> len = sprintf(buffer, "%d", value);
>> if ((sysfd = open(filename, O_WRONLY)) < 0) {
>> perror(filename);
>> + free(cdename);
>> exit(1);
>> }
>> if ((ret = write(sysfd, buffer, len)) < 0) {
>> perror("write");
>> + free(cdename);
>> exit(1);
>> }
>> if (ret < len) {
>> fprintf(stderr, "error: short write in set_flags!\n");
>> + free(cdename);
>> exit(1);
>> }
>> close(sysfd);
>> @@ -359,6 +367,7 @@ usage(int excode, char *module)
>> else
>> fprintf(stderr, " (use %s -vh to get a list of modules and valid flags)\n", cdename);
>> }
>> + free(cdename);
>> exit (excode);
>> }
>>
>>

2017-07-27 16:54:12

by Steve Dickson

Subject: Re: [PATCH] rpcdebug: Add proper free() to avoid memory leak

On 07/27/2017 12:06 PM, ChunYu Wang wrote:
> Maybe we can not trust valgrind? Or I have just misunderstood what did
> the valgrind report.
>
> As I have just added enough free(cdename) statements before exit,
> valgrind will not report "still reachable" any more.
>
> Since I am not sure of whether this kind of report by valgrind should
> be treated as potential leaks, please help me to judge!
I thinking its a false positive... since all resources are released
when a process exits. But by no means am I an expert at
reading valgrind reports.

Plus the mount of code churn would be horrible for something
that is just not needed.

steved.

>
> Thanks,
> - Chunyu
>
> On Thu, Jul 27, 2017 at 10:48 PM, Steve Dickson <[email protected]> wrote:
>>
>>
>> On 07/27/2017 03:59 AM, ChunYu Wang wrote:
>>> rpcdebug.c use char* cdename to store program name:
>>>
>>> cdename = malloc(strlen(basename(argv[0])));
>>> strcpy(cdename, basename(argv[0]));
>>>
>>> It is better to free before exit to avoid potential leaks.
>> I guess I don't understand how memory can be leaked
>> when a process is exiting... What am I missing?
>>
>> steved.
>>>
>>> Signed-off-by: ChunYu Wang <[email protected]>
>>> ---
>>> tools/rpcdebug/rpcdebug.c | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/tools/rpcdebug/rpcdebug.c b/tools/rpcdebug/rpcdebug.c
>>> index 68206cc..722ab4d 100644
>>> --- a/tools/rpcdebug/rpcdebug.c
>>> +++ b/tools/rpcdebug/rpcdebug.c
>>> @@ -131,6 +131,7 @@ main(int argc, char **argv)
>>> print_flags(stdout, module, ~(unsigned int) 0, 1);
>>> }
>>>
>>> + free(cdename);
>>> return 0;
>>> }
>>>
>>> @@ -221,6 +222,7 @@ find_flag(char **module, char *name)
>>> "please specify the module name using\n"
>>> "the -m option.\n",
>>> cdename, name);
>>> + free(cdename);
>>> exit(1);
>>> }
>>> value = flagmap[i].value;
>>> @@ -238,6 +240,7 @@ find_flag(char **module, char *name)
>>> fprintf(stderr,
>>> "%s: unknown flag %s\n",
>>> cdename, name);
>>> + free(cdename);
>>> exit(1);
>>> }
>>>
>>> @@ -255,10 +258,12 @@ get_flags(char *module)
>>>
>>> if ((sysfd = open(filename, O_RDONLY)) < 0) {
>>> perror(filename);
>>> + free(cdename);
>>> exit(1);
>>> }
>>> if ((len = read(sysfd, buffer, sizeof(buffer))) < 0) {
>>> perror("read");
>>> + free(cdename);
>>> exit(1);
>>> }
>>> close(sysfd);
>>> @@ -278,14 +283,17 @@ set_flags(char *module, unsigned int value)
>>> len = sprintf(buffer, "%d", value);
>>> if ((sysfd = open(filename, O_WRONLY)) < 0) {
>>> perror(filename);
>>> + free(cdename);
>>> exit(1);
>>> }
>>> if ((ret = write(sysfd, buffer, len)) < 0) {
>>> perror("write");
>>> + free(cdename);
>>> exit(1);
>>> }
>>> if (ret < len) {
>>> fprintf(stderr, "error: short write in set_flags!\n");
>>> + free(cdename);
>>> exit(1);
>>> }
>>> close(sysfd);
>>> @@ -359,6 +367,7 @@ usage(int excode, char *module)
>>> else
>>> fprintf(stderr, " (use %s -vh to get a list of modules and valid flags)\n", cdename);
>>> }
>>> + free(cdename);
>>> exit (excode);
>>> }
>>>
>>>

2017-07-27 17:20:53

by ChunYu Wang

Subject: Re: [PATCH] rpcdebug: Add proper free() to avoid memory leak

En, I also agree about that : )

Thanks,
- Chunyu

On Fri, Jul 28, 2017 at 12:54 AM, Steve Dickson <[email protected]> wrote:
>
>
> On 07/27/2017 12:06 PM, ChunYu Wang wrote:
>> Maybe we can not trust valgrind? Or I have just misunderstood what did
>> the valgrind report.
>>
>> As I have just added enough free(cdename) statements before exit,
>> valgrind will not report "still reachable" any more.
>>
>> Since I am not sure of whether this kind of report by valgrind should
>> be treated as potential leaks, please help me to judge!
> I thinking its a false positive... since all resources are released
> when a process exits. But by no means am I an expert at
> reading valgrind reports.
>
> Plus the mount of code churn would be horrible for something
> that is just not needed.
>
> steved.
>
>>
>> Thanks,
>> - Chunyu
>>
>> On Thu, Jul 27, 2017 at 10:48 PM, Steve Dickson <[email protected]> wrote:
>>>
>>>
>>> On 07/27/2017 03:59 AM, ChunYu Wang wrote:
>>>> rpcdebug.c use char* cdename to store program name:
>>>>
>>>> cdename = malloc(strlen(basename(argv[0])));
>>>> strcpy(cdename, basename(argv[0]));
>>>>
>>>> It is better to free before exit to avoid potential leaks.
>>> I guess I don't understand how memory can be leaked
>>> when a process is exiting... What am I missing?
>>>
>>> steved.
>>>>
>>>> Signed-off-by: ChunYu Wang <[email protected]>
>>>> ---
>>>> tools/rpcdebug/rpcdebug.c | 9 +++++++++
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/tools/rpcdebug/rpcdebug.c b/tools/rpcdebug/rpcdebug.c
>>>> index 68206cc..722ab4d 100644
>>>> --- a/tools/rpcdebug/rpcdebug.c
>>>> +++ b/tools/rpcdebug/rpcdebug.c
>>>> @@ -131,6 +131,7 @@ main(int argc, char **argv)
>>>> print_flags(stdout, module, ~(unsigned int) 0, 1);
>>>> }
>>>>
>>>> + free(cdename);
>>>> return 0;
>>>> }
>>>>
>>>> @@ -221,6 +222,7 @@ find_flag(char **module, char *name)
>>>> "please specify the module name using\n"
>>>> "the -m option.\n",
>>>> cdename, name);
>>>> + free(cdename);
>>>> exit(1);
>>>> }
>>>> value = flagmap[i].value;
>>>> @@ -238,6 +240,7 @@ find_flag(char **module, char *name)
>>>> fprintf(stderr,
>>>> "%s: unknown flag %s\n",
>>>> cdename, name);
>>>> + free(cdename);
>>>> exit(1);
>>>> }
>>>>
>>>> @@ -255,10 +258,12 @@ get_flags(char *module)
>>>>
>>>> if ((sysfd = open(filename, O_RDONLY)) < 0) {
>>>> perror(filename);
>>>> + free(cdename);
>>>> exit(1);
>>>> }
>>>> if ((len = read(sysfd, buffer, sizeof(buffer))) < 0) {
>>>> perror("read");
>>>> + free(cdename);
>>>> exit(1);
>>>> }
>>>> close(sysfd);
>>>> @@ -278,14 +283,17 @@ set_flags(char *module, unsigned int value)
>>>> len = sprintf(buffer, "%d", value);
>>>> if ((sysfd = open(filename, O_WRONLY)) < 0) {
>>>> perror(filename);
>>>> + free(cdename);
>>>> exit(1);
>>>> }
>>>> if ((ret = write(sysfd, buffer, len)) < 0) {
>>>> perror("write");
>>>> + free(cdename);
>>>> exit(1);
>>>> }
>>>> if (ret < len) {
>>>> fprintf(stderr, "error: short write in set_flags!\n");
>>>> + free(cdename);
>>>> exit(1);
>>>> }
>>>> close(sysfd);
>>>> @@ -359,6 +367,7 @@ usage(int excode, char *module)
>>>> else
>>>> fprintf(stderr, " (use %s -vh to get a list of modules and valid flags)\n", cdename);
>>>> }
>>>> + free(cdename);
>>>> exit (excode);
>>>> }
>>>>
>>>>