* [PATCH] obexd/src/main: Fix memory leak on obex server @ 2014-10-01 6:31 Gowtham Anandha Babu 2014-10-01 6:58 ` Szymon Janc 0 siblings, 1 reply; 7+ messages in thread From: Gowtham Anandha Babu @ 2014-10-01 6:31 UTC (permalink / raw) To: linux-bluetooth; +Cc: d.kasatkin, bharat.panda, cpgs, Gowtham Anandha Babu Freeing the variables at appropriate place. --- obexd/src/main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/obexd/src/main.c b/obexd/src/main.c index 80645f8..7a1ab98 100644 --- a/obexd/src/main.c +++ b/obexd/src/main.c @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) char *old_root = option_root, *home = getenv("HOME"); if (home) { option_root = g_strdup_printf("%s/%s", home, old_root); - g_free(old_root); + g_free(home); } + g_free(old_root); } if (option_capability == NULL) -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] obexd/src/main: Fix memory leak on obex server 2014-10-01 6:31 [PATCH] obexd/src/main: Fix memory leak on obex server Gowtham Anandha Babu @ 2014-10-01 6:58 ` Szymon Janc 2014-10-01 8:51 ` Gowtham Anandha Babu 0 siblings, 1 reply; 7+ messages in thread From: Szymon Janc @ 2014-10-01 6:58 UTC (permalink / raw) To: Gowtham Anandha Babu; +Cc: linux-bluetooth, d.kasatkin, bharat.panda, cpgs Hi Gowtham, On Wednesday 01 of October 2014 12:01:43 Gowtham Anandha Babu wrote: > Freeing the variables at appropriate place. > --- > obexd/src/main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/obexd/src/main.c b/obexd/src/main.c > index 80645f8..7a1ab98 100644 > --- a/obexd/src/main.c > +++ b/obexd/src/main.c > @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) > char *old_root = option_root, *home = getenv("HOME"); > if (home) { > option_root = g_strdup_printf("%s/%s", home, old_root); > - g_free(old_root); > + g_free(home); > } > + g_free(old_root); > } > > if (option_capability == NULL) > This patch is incorrect in multiple ways: - freeing old_root without altering option_root would result in use-after-free few lines later in root_folder_setup() or (if program didn't crash already) double free on exit. - freeing home pointer would result in crash or undefined behavior: from getenv manual: "As typically implemented, getenv() returns a pointer to a string within the environment list. The caller must take care not to modify this string, since that would change the environment of the process. ...The string pointed to by the return value of getenv() may be statically allocated," But I agree that original code is a bit confusing. Maybe something like this would make it more readable? --- a/obexd/src/main.c +++ b/obexd/src/main.c @@ -290,8 +290,9 @@ int main(int argc, char *argv[]) } if (option_root[0] != '/') { - char *old_root = option_root, *home = getenv("HOME"); + const char *home = getenv("HOME"); if (home) { + char *old_root = option_root; option_root = g_strdup_printf("%s/%s", home, old_root); g_free(old_root); } -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] obexd/src/main: Fix memory leak on obex server 2014-10-01 6:58 ` Szymon Janc @ 2014-10-01 8:51 ` Gowtham Anandha Babu 2014-10-01 9:00 ` Szymon Janc 0 siblings, 1 reply; 7+ messages in thread From: Gowtham Anandha Babu @ 2014-10-01 8:51 UTC (permalink / raw) To: 'Szymon Janc' Cc: linux-bluetooth, 'Dmitry Kasatkin', 'Bharat Panda' Hi Szymon, > -----Original Message----- > From: Szymon Janc [mailto:szymon.janc@tieto.com] > Sent: Wednesday, October 01, 2014 12:28 PM > To: Gowtham Anandha Babu > Cc: linux-bluetooth@vger.kernel.org; d.kasatkin@samsung.com; > bharat.panda@samsung.com; cpgs@samsung.com > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > Hi Gowtham, > > On Wednesday 01 of October 2014 12:01:43 Gowtham Anandha Babu wrote: > > Freeing the variables at appropriate place. > > --- > > obexd/src/main.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/obexd/src/main.c b/obexd/src/main.c index > > 80645f8..7a1ab98 100644 > > --- a/obexd/src/main.c > > +++ b/obexd/src/main.c > > @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) > > char *old_root = option_root, *home = getenv("HOME"); > > if (home) { > > option_root = g_strdup_printf("%s/%s", home, > old_root); > > - g_free(old_root); > > + g_free(home); > > } > > + g_free(old_root); > > } > > > > if (option_capability == NULL) > > > > This patch is incorrect in multiple ways: > - freeing old_root without altering option_root would result in use-after-free > few lines later in root_folder_setup() or (if program didn't crash already) > double free on exit. > - freeing home pointer would result in crash or undefined behavior: > from getenv manual: "As typically implemented, getenv() returns a pointer > to > a string within the environment list. The caller must take care not to > modify this string, since that would change the environment of the process. > ...The string pointed to by the return value of getenv() may be statically > allocated," > > But I agree that original code is a bit confusing. Maybe something like this > would make it more readable? > > --- a/obexd/src/main.c > +++ b/obexd/src/main.c > @@ -290,8 +290,9 @@ int main(int argc, char *argv[]) > } > > if (option_root[0] != '/') { > - char *old_root = option_root, *home = getenv("HOME"); > + const char *home = getenv("HOME"); > if (home) { > + char *old_root = option_root; > option_root = g_strdup_printf("%s/%s", home, old_root); > g_free(old_root); > } > > > -- > Best regards, > Szymon Janc I agree with you. But why can't it be like this if (option_root[0] != '/') { const char *home = getenv("HOME"); if (home) { option_root = g_strdup_printf("%s/%s", home, option_root); } } --- (checked, it's not crashing) Regards, Gowtham ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] obexd/src/main: Fix memory leak on obex server 2014-10-01 8:51 ` Gowtham Anandha Babu @ 2014-10-01 9:00 ` Szymon Janc 2014-10-01 10:54 ` Gowtham Anandha Babu 0 siblings, 1 reply; 7+ messages in thread From: Szymon Janc @ 2014-10-01 9:00 UTC (permalink / raw) To: Gowtham Anandha Babu Cc: linux-bluetooth, 'Dmitry Kasatkin', 'Bharat Panda' Hi Gowtham, On Wednesday 01 of October 2014 14:21:22 Gowtham Anandha Babu wrote: > Hi Szymon, > > > -----Original Message----- > > From: Szymon Janc [mailto:szymon.janc@tieto.com] > > Sent: Wednesday, October 01, 2014 12:28 PM > > To: Gowtham Anandha Babu > > Cc: linux-bluetooth@vger.kernel.org; d.kasatkin@samsung.com; > > bharat.panda@samsung.com; cpgs@samsung.com > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > > > Hi Gowtham, > > > > On Wednesday 01 of October 2014 12:01:43 Gowtham Anandha Babu wrote: > > > Freeing the variables at appropriate place. > > > --- > > > obexd/src/main.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/obexd/src/main.c b/obexd/src/main.c index > > > 80645f8..7a1ab98 100644 > > > --- a/obexd/src/main.c > > > +++ b/obexd/src/main.c > > > @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) > > > char *old_root = option_root, *home = getenv("HOME"); > > > if (home) { > > > option_root = g_strdup_printf("%s/%s", home, > > old_root); > > > - g_free(old_root); > > > + g_free(home); > > > } > > > + g_free(old_root); > > > } > > > > > > if (option_capability == NULL) > > > > > > > This patch is incorrect in multiple ways: > > - freeing old_root without altering option_root would result in use-after-free > > few lines later in root_folder_setup() or (if program didn't crash already) > > double free on exit. > > - freeing home pointer would result in crash or undefined behavior: > > from getenv manual: "As typically implemented, getenv() returns a pointer > > to > > a string within the environment list. The caller must take care not to > > modify this string, since that would change the environment of the process. > > ...The string pointed to by the return value of getenv() may be statically > > allocated," > > > > But I agree that original code is a bit confusing. Maybe something like this > > would make it more readable? > > > > --- a/obexd/src/main.c > > +++ b/obexd/src/main.c > > @@ -290,8 +290,9 @@ int main(int argc, char *argv[]) > > } > > > > if (option_root[0] != '/') { > > - char *old_root = option_root, *home = getenv("HOME"); > > + const char *home = getenv("HOME"); > > if (home) { > > + char *old_root = option_root; > > option_root = g_strdup_printf("%s/%s", home, old_root); > > g_free(old_root); > > } > > > > > > -- > > Best regards, > > Szymon Janc > > I agree with you. > But why can't it be like this > > if (option_root[0] != '/') { > const char *home = getenv("HOME"); > if (home) { > option_root = g_strdup_printf("%s/%s", home, option_root); > } > } > --- > (checked, it's not crashing) In that case you are leaking option_root. That is why temporary old_root is needed so that original option_root can be freed. -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] obexd/src/main: Fix memory leak on obex server 2014-10-01 9:00 ` Szymon Janc @ 2014-10-01 10:54 ` Gowtham Anandha Babu 2014-10-01 13:22 ` Szymon Janc 0 siblings, 1 reply; 7+ messages in thread From: Gowtham Anandha Babu @ 2014-10-01 10:54 UTC (permalink / raw) To: 'Szymon Janc' Cc: linux-bluetooth, 'Dmitry Kasatkin', 'Bharat Panda' Hi Szymon, > -----Original Message----- > From: Szymon Janc [mailto:szymon.janc@tieto.com] > Sent: Wednesday, October 01, 2014 2:31 PM > To: Gowtham Anandha Babu > Cc: linux-bluetooth@vger.kernel.org; 'Dmitry Kasatkin'; 'Bharat Panda' > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > Hi Gowtham, > > On Wednesday 01 of October 2014 14:21:22 Gowtham Anandha Babu wrote: > > Hi Szymon, > > > > > -----Original Message----- > > > From: Szymon Janc [mailto:szymon.janc@tieto.com] > > > Sent: Wednesday, October 01, 2014 12:28 PM > > > To: Gowtham Anandha Babu > > > Cc: linux-bluetooth@vger.kernel.org; d.kasatkin@samsung.com; > > > bharat.panda@samsung.com; cpgs@samsung.com > > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > > > > > Hi Gowtham, > > > > > > On Wednesday 01 of October 2014 12:01:43 Gowtham Anandha Babu > wrote: > > > > Freeing the variables at appropriate place. > > > > --- > > > > obexd/src/main.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/obexd/src/main.c b/obexd/src/main.c index > > > > 80645f8..7a1ab98 100644 > > > > --- a/obexd/src/main.c > > > > +++ b/obexd/src/main.c > > > > @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) > > > > char *old_root = option_root, *home = getenv("HOME"); > > > > if (home) { > > > > option_root = g_strdup_printf("%s/%s", home, > > > old_root); > > > > - g_free(old_root); > > > > + g_free(home); > > > > } > > > > + g_free(old_root); > > > > } > > > > > > > > if (option_capability == NULL) > > > > > > > > > > This patch is incorrect in multiple ways: > > > - freeing old_root without altering option_root would result in use-after- > free > > > few lines later in root_folder_setup() or (if program didn't crash already) > > > double free on exit. > > > - freeing home pointer would result in crash or undefined behavior: > > > from getenv manual: "As typically implemented, getenv() returns a > > > pointer to > > > a string within the environment list. The caller must take care not to > > > modify this string, since that would change the environment of the > process. > > > ...The string pointed to by the return value of getenv() may be statically > > > allocated," > > > > > > But I agree that original code is a bit confusing. Maybe something > > > like this would make it more readable? > > > > > > --- a/obexd/src/main.c > > > +++ b/obexd/src/main.c > > > @@ -290,8 +290,9 @@ int main(int argc, char *argv[]) > > > } > > > > > > if (option_root[0] != '/') { > > > - char *old_root = option_root, *home = getenv("HOME"); > > > + const char *home = getenv("HOME"); > > > if (home) { > > > + char *old_root = option_root; > > > option_root = g_strdup_printf("%s/%s", home, old_root); > > > g_free(old_root); > > > } > > > > > > > > > -- > > > Best regards, > > > Szymon Janc > > > > I agree with you. > > But why can't it be like this > > > > if (option_root[0] != '/') { > > const char *home = getenv("HOME"); > > if (home) { > > option_root = g_strdup_printf("%s/%s", home, > option_root); > > } > > } > > --- > > (checked, it's not crashing) > > > In that case you are leaking option_root. That is why temporary old_root is > needed so that original option_root can be freed. > > -- > Best regards, > Szymon Janc Sorry If I am wrong, at the end option_root was freed after g_mail_loop_unref(main_loop). Is that enough ? Or Do we need to free that intermediate option_root inside g_strdup_printf() ? One more clarification: static gboolean parse_debug(const char *key, const char *value, gpointer user_data, GError **error) { if (value) option_debug = g_strdup(value); else option_debug = g_strdup("*"); return TRUE; } In the above function, option_debug needs be freed right? As Like below: static gboolean parse_debug(const char *key, const char *value, gpointer user_data, GError **error) { g_free(option_debug); if (value) option_debug = g_strdup(value); else option_debug = g_strdup("*"); return TRUE; } Regards, Gowtham ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] obexd/src/main: Fix memory leak on obex server 2014-10-01 10:54 ` Gowtham Anandha Babu @ 2014-10-01 13:22 ` Szymon Janc 2014-10-01 13:46 ` Gowtham Anandha Babu 0 siblings, 1 reply; 7+ messages in thread From: Szymon Janc @ 2014-10-01 13:22 UTC (permalink / raw) To: Gowtham Anandha Babu Cc: linux-bluetooth, 'Dmitry Kasatkin', 'Bharat Panda' Hi Gowtham, On Wednesday 01 of October 2014 16:24:27 Gowtham Anandha Babu wrote: > Hi Szymon, > > > -----Original Message----- > > From: Szymon Janc [mailto:szymon.janc@tieto.com] > > Sent: Wednesday, October 01, 2014 2:31 PM > > To: Gowtham Anandha Babu > > Cc: linux-bluetooth@vger.kernel.org; 'Dmitry Kasatkin'; 'Bharat Panda' > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > > > Hi Gowtham, > > > > On Wednesday 01 of October 2014 14:21:22 Gowtham Anandha Babu wrote: > > > Hi Szymon, > > > > > > > -----Original Message----- > > > > From: Szymon Janc [mailto:szymon.janc@tieto.com] > > > > Sent: Wednesday, October 01, 2014 12:28 PM > > > > To: Gowtham Anandha Babu > > > > Cc: linux-bluetooth@vger.kernel.org; d.kasatkin@samsung.com; > > > > bharat.panda@samsung.com; cpgs@samsung.com > > > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > > > > > > > Hi Gowtham, > > > > > > > > On Wednesday 01 of October 2014 12:01:43 Gowtham Anandha Babu > > wrote: > > > > > Freeing the variables at appropriate place. > > > > > --- > > > > > obexd/src/main.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/obexd/src/main.c b/obexd/src/main.c index > > > > > 80645f8..7a1ab98 100644 > > > > > --- a/obexd/src/main.c > > > > > +++ b/obexd/src/main.c > > > > > @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) > > > > > char *old_root = option_root, *home = getenv("HOME"); > > > > > if (home) { > > > > > option_root = g_strdup_printf("%s/%s", home, > > > > old_root); > > > > > - g_free(old_root); > > > > > + g_free(home); > > > > > } > > > > > + g_free(old_root); > > > > > } > > > > > > > > > > if (option_capability == NULL) > > > > > > > > > > > > > This patch is incorrect in multiple ways: > > > > - freeing old_root without altering option_root would result in use-after- > > free > > > > few lines later in root_folder_setup() or (if program didn't crash already) > > > > double free on exit. > > > > - freeing home pointer would result in crash or undefined behavior: > > > > from getenv manual: "As typically implemented, getenv() returns a > > > > pointer to > > > > a string within the environment list. The caller must take care not to > > > > modify this string, since that would change the environment of the > > process. > > > > ...The string pointed to by the return value of getenv() may be statically > > > > allocated," > > > > > > > > But I agree that original code is a bit confusing. Maybe something > > > > like this would make it more readable? > > > > > > > > --- a/obexd/src/main.c > > > > +++ b/obexd/src/main.c > > > > @@ -290,8 +290,9 @@ int main(int argc, char *argv[]) > > > > } > > > > > > > > if (option_root[0] != '/') { > > > > - char *old_root = option_root, *home = getenv("HOME"); > > > > + const char *home = getenv("HOME"); > > > > if (home) { > > > > + char *old_root = option_root; > > > > option_root = g_strdup_printf("%s/%s", home, old_root); > > > > g_free(old_root); > > > > } > > > > > > > > > > > > -- > > > > Best regards, > > > > Szymon Janc > > > > > > I agree with you. > > > But why can't it be like this > > > > > > if (option_root[0] != '/') { > > > const char *home = getenv("HOME"); > > > if (home) { > > > option_root = g_strdup_printf("%s/%s", home, > > option_root); > > > } > > > } > > > --- > > > (checked, it's not crashing) > > > > > > In that case you are leaking option_root. That is why temporary old_root is > > needed so that original option_root can be freed. > > > > -- > > Best regards, > > Szymon Janc > > Sorry If I am wrong, at the end option_root was freed after g_mail_loop_unref(main_loop). Is that enough ? > Or Do we need to free that intermediate option_root inside g_strdup_printf() ? Memory pointed by old option_root needs to be freed since pointer is overwritten. I suggest exploring valgrind for tracking memory leaks (see HACKING for guideline). > One more clarification: > > static gboolean parse_debug(const char *key, const char *value, > gpointer user_data, GError **error) > { > if (value) > option_debug = g_strdup(value); > else > option_debug = g_strdup("*"); > > return TRUE; > } > > > In the above function, option_debug needs be freed right? As Like below: > > static gboolean parse_debug(const char *key, const char *value, > gpointer user_data, GError **error) > { > g_free(option_debug); > if (value) > option_debug = g_strdup(value); > else > option_debug = g_strdup("*"); > > return TRUE; > } There is no need to free that since option_debug is initially NULL. -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] obexd/src/main: Fix memory leak on obex server 2014-10-01 13:22 ` Szymon Janc @ 2014-10-01 13:46 ` Gowtham Anandha Babu 0 siblings, 0 replies; 7+ messages in thread From: Gowtham Anandha Babu @ 2014-10-01 13:46 UTC (permalink / raw) To: 'Szymon Janc' Cc: linux-bluetooth, 'Dmitry Kasatkin', 'Bharat Panda' Hi Szymon, > -----Original Message----- > From: Szymon Janc [mailto:szymon.janc@tieto.com] > Sent: Wednesday, October 01, 2014 6:52 PM > To: Gowtham Anandha Babu > Cc: linux-bluetooth@vger.kernel.org; 'Dmitry Kasatkin'; 'Bharat Panda' > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > Hi Gowtham, > > On Wednesday 01 of October 2014 16:24:27 Gowtham Anandha Babu wrote: > > Hi Szymon, > > > > > -----Original Message----- > > > From: Szymon Janc [mailto:szymon.janc@tieto.com] > > > Sent: Wednesday, October 01, 2014 2:31 PM > > > To: Gowtham Anandha Babu > > > Cc: linux-bluetooth@vger.kernel.org; 'Dmitry Kasatkin'; 'Bharat Panda' > > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > > > > > Hi Gowtham, > > > > > > On Wednesday 01 of October 2014 14:21:22 Gowtham Anandha Babu > wrote: > > > > Hi Szymon, > > > > > > > > > -----Original Message----- > > > > > From: Szymon Janc [mailto:szymon.janc@tieto.com] > > > > > Sent: Wednesday, October 01, 2014 12:28 PM > > > > > To: Gowtham Anandha Babu > > > > > Cc: linux-bluetooth@vger.kernel.org; d.kasatkin@samsung.com; > > > > > bharat.panda@samsung.com; cpgs@samsung.com > > > > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex > > > > > server > > > > > > > > > > Hi Gowtham, > > > > > > > > > > On Wednesday 01 of October 2014 12:01:43 Gowtham Anandha Babu > > > wrote: > > > > > > Freeing the variables at appropriate place. > > > > > > --- > > > > > > obexd/src/main.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/obexd/src/main.c b/obexd/src/main.c index > > > > > > 80645f8..7a1ab98 100644 > > > > > > --- a/obexd/src/main.c > > > > > > +++ b/obexd/src/main.c > > > > > > @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) > > > > > > char *old_root = option_root, *home = > getenv("HOME"); > > > > > > if (home) { > > > > > > option_root = g_strdup_printf("%s/%s", > home, > > > > > old_root); > > > > > > - g_free(old_root); > > > > > > + g_free(home); > > > > > > } > > > > > > + g_free(old_root); > > > > > > } > > > > > > > > > > > > if (option_capability == NULL) > > > > > > > > > > > > > > > > This patch is incorrect in multiple ways: > > > > > - freeing old_root without altering option_root would result in > > > > > use-after- > > > free > > > > > few lines later in root_folder_setup() or (if program didn't crash > already) > > > > > double free on exit. > > > > > - freeing home pointer would result in crash or undefined behavior: > > > > > from getenv manual: "As typically implemented, getenv() > > > > > returns a pointer to > > > > > a string within the environment list. The caller must take care not to > > > > > modify this string, since that would change the environment of > > > > > the > > > process. > > > > > ...The string pointed to by the return value of getenv() may be > statically > > > > > allocated," > > > > > > > > > > But I agree that original code is a bit confusing. Maybe > > > > > something like this would make it more readable? > > > > > > > > > > --- a/obexd/src/main.c > > > > > +++ b/obexd/src/main.c > > > > > @@ -290,8 +290,9 @@ int main(int argc, char *argv[]) > > > > > } > > > > > > > > > > if (option_root[0] != '/') { > > > > > - char *old_root = option_root, *home = getenv("HOME"); > > > > > + const char *home = getenv("HOME"); > > > > > if (home) { > > > > > + char *old_root = option_root; > > > > > option_root = g_strdup_printf("%s/%s", home, old_root); > > > > > g_free(old_root); > > > > > } > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Szymon Janc > > > > > > > > I agree with you. > > > > But why can't it be like this > > > > > > > > if (option_root[0] != '/') { > > > > const char *home = getenv("HOME"); > > > > if (home) { > > > > option_root = g_strdup_printf("%s/%s", home, > > > option_root); > > > > } > > > > } > > > > --- > > > > (checked, it's not crashing) > > > > > > > > > In that case you are leaking option_root. That is why temporary > > > old_root is needed so that original option_root can be freed. > > > > > > -- > > > Best regards, > > > Szymon Janc > > > > Sorry If I am wrong, at the end option_root was freed after > g_mail_loop_unref(main_loop). Is that enough ? > > Or Do we need to free that intermediate option_root inside > g_strdup_printf() ? > > Memory pointed by old option_root needs to be freed since pointer is > overwritten. I suggest exploring valgrind for tracking memory leaks (see > HACKING for guideline). > > > One more clarification: > > > > static gboolean parse_debug(const char *key, const char *value, > > gpointer user_data, GError **error) { > > if (value) > > option_debug = g_strdup(value); > > else > > option_debug = g_strdup("*"); > > > > return TRUE; > > } > > > > > > In the above function, option_debug needs be freed right? As Like below: > > > > static gboolean parse_debug(const char *key, const char *value, > > gpointer user_data, GError **error) { > > g_free(option_debug); > > if (value) > > option_debug = g_strdup(value); > > else > > option_debug = g_strdup("*"); > > > > return TRUE; > > } > > There is no need to free that since option_debug is initially NULL. > > -- > Best regards, > Szymon Janc I agree with you. Then I will sent the updated patch to make it more readable as suggested by you in the above discussion with some coding style fixes. Regards, Gowtham ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-10-01 13:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-01 6:31 [PATCH] obexd/src/main: Fix memory leak on obex server Gowtham Anandha Babu 2014-10-01 6:58 ` Szymon Janc 2014-10-01 8:51 ` Gowtham Anandha Babu 2014-10-01 9:00 ` Szymon Janc 2014-10-01 10:54 ` Gowtham Anandha Babu 2014-10-01 13:22 ` Szymon Janc 2014-10-01 13:46 ` Gowtham Anandha Babu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).