| Message ID | 20210630082724.50838-5-wuguanghao3@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,01/12] profile_create_node: set magic before strdup(name) to avoid memory leak | expand |
On Wed, Jun 30, 2021 at 04:27:16PM +0800, wuguanghao wrote: > In ss_add_info_dir(), need free info->info_dirs before return, > otherwise it will cause memory leak. At the same time, it is necessary > to check whether dirs[n_dirs] is a null pointer, otherwise a segmentation > fault will occur. > > Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > Reviewed-by: Wu Bo <wubo40@huawei.com> > --- > lib/ss/help.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/lib/ss/help.c b/lib/ss/help.c > index 5204401b..6768b9b1 100644 > --- a/lib/ss/help.c > +++ b/lib/ss/help.c > @@ -148,6 +148,7 @@ void ss_add_info_dir(int sci_idx, char *info_dir, int *code_ptr) > dirs = (char **)realloc((char *)dirs, > (unsigned)(n_dirs + 2)*sizeof(char *)); > if (dirs == (char **)NULL) { > + free(info->info_dirs); > info->info_dirs = (char **)NULL; > *code_ptr = errno; > return; Adding the free() isn't right fix. The real problem is that this line should be removed: info->info_dirs = (char **)NULL; The function is trying to add a string (a directory name) to list, and the realloc() is trying to extend the list. If the realloc fils, we shouldn't be zapping the original list. We should just be returning, and leaving the original list of directories unchanged and untouched. - Ted
On 2021/7/16 11:27, Theodore Y. Ts'o wrote: > On Wed, Jun 30, 2021 at 04:27:16PM +0800, wuguanghao wrote: >> In ss_add_info_dir(), need free info->info_dirs before return, >> otherwise it will cause memory leak. At the same time, it is necessary >> to check whether dirs[n_dirs] is a null pointer, otherwise a segmentation >> fault will occur. >> >> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com> >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> Reviewed-by: Wu Bo <wubo40@huawei.com> >> --- >> lib/ss/help.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/lib/ss/help.c b/lib/ss/help.c >> index 5204401b..6768b9b1 100644 >> --- a/lib/ss/help.c >> +++ b/lib/ss/help.c >> @@ -148,6 +148,7 @@ void ss_add_info_dir(int sci_idx, char *info_dir, int *code_ptr) >> dirs = (char **)realloc((char *)dirs, >> (unsigned)(n_dirs + 2)*sizeof(char *)); >> if (dirs == (char **)NULL) { >> + free(info->info_dirs); >> info->info_dirs = (char **)NULL; >> *code_ptr = errno; >> return; > Adding the free() isn't right fix. The real problem is that this line > should be removed: > > info->info_dirs = (char **)NULL; > > The function is trying to add a string (a directory name) to list, and > the realloc() is trying to extend the list. If the realloc fils, we > shouldn't be zapping the original list. We should just be returning, > and leaving the original list of directories unchanged and untouched. > > - Ted Thanks for your patient reply. We will correct that in v3 patch. > > .
diff --git a/lib/ss/help.c b/lib/ss/help.c index 5204401b..6768b9b1 100644 --- a/lib/ss/help.c +++ b/lib/ss/help.c @@ -148,6 +148,7 @@ void ss_add_info_dir(int sci_idx, char *info_dir, int *code_ptr) dirs = (char **)realloc((char *)dirs, (unsigned)(n_dirs + 2)*sizeof(char *)); if (dirs == (char **)NULL) { + free(info->info_dirs); info->info_dirs = (char **)NULL; *code_ptr = errno; return; @@ -155,6 +156,10 @@ void ss_add_info_dir(int sci_idx, char *info_dir, int *code_ptr) info->info_dirs = dirs; dirs[n_dirs + 1] = (char *)NULL; dirs[n_dirs] = malloc((unsigned)strlen(info_dir)+1); + if (dirs[n_dirs] == (char *)NULL) { + *code_ptr = errno; + return; + } strcpy(dirs[n_dirs], info_dir); *code_ptr = 0; }