[dpdk-dev,v2] mem: memory leaks of hubedir caused by strdup
Checks
Commit Message
Coverity issue: 272585
Fixes: cb97d93e9d3b ("mem: share hugepage info primary and secondary")
Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
Comments
17/04/2018 12:06, Yangchao Zhou:
> Coverity issue: 272585
> Fixes: cb97d93e9d3b ("mem: share hugepage info primary and secondary")
>
> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Better to provide a small explanation.
> - retval = strdup(splitstr[MOUNTPT]);
> + snprintf(hugedir, len, "%s", splitstr[MOUNTPT]);
I think it is candidate to be replaced by strlcpy.
Please check
On 17-Apr-18 11:24 AM, Thomas Monjalon wrote:
> 17/04/2018 12:06, Yangchao Zhou:
>> Coverity issue: 272585
>> Fixes: cb97d93e9d3b ("mem: share hugepage info primary and secondary")
>>
>> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
> Better to provide a small explanation.
>
>> - retval = strdup(splitstr[MOUNTPT]);
>> + snprintf(hugedir, len, "%s", splitstr[MOUNTPT]);
>
> I think it is candidate to be replaced by strlcpy.
> Please check
>
Yes, it seems that strlcpy thingie was merged without much fanfare. I'll
be submitting a patch fixing various usages of snprintf in my recent
commits. I'm inclined to leave this as is for this commit, as it's not
the purpose of this fix.
As Burakov said, for no other reason, I just followed the old version.
On Tue, Apr 17, 2018 at 6:31 PM Burakov, Anatoly <anatoly.burakov@intel.com>
wrote:
> On 17-Apr-18 11:24 AM, Thomas Monjalon wrote:
> > 17/04/2018 12:06, Yangchao Zhou:
> >> Coverity issue: 272585
> >> Fixes: cb97d93e9d3b ("mem: share hugepage info primary and secondary")
> >>
> >> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
> >> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >
> > Better to provide a small explanation.
> >
> >> - retval = strdup(splitstr[MOUNTPT]);
> >> + snprintf(hugedir, len, "%s",
> splitstr[MOUNTPT]);
> >
> > I think it is candidate to be replaced by strlcpy.
> > Please check
> >
>
> Yes, it seems that strlcpy thingie was merged without much fanfare. I'll
> be submitting a patch fixing various usages of snprintf in my recent
> commits. I'm inclined to leave this as is for this commit, as it's not
> the purpose of this fix.
>
> --
> Thanks,
> Anatoly
>
I see no reason to accept this patch, replacing strdup by snprintf,
given that we have strlcpy.
Please do a v3 with strlcpy.
17/04/2018 13:16, zhouyangchao:
> As Burakov said, for no other reason, I just followed the old version.
>
> On Tue, Apr 17, 2018 at 6:31 PM Burakov, Anatoly <anatoly.burakov@intel.com>
> wrote:
>
> > On 17-Apr-18 11:24 AM, Thomas Monjalon wrote:
> > > 17/04/2018 12:06, Yangchao Zhou:
> > >> Coverity issue: 272585
> > >> Fixes: cb97d93e9d3b ("mem: share hugepage info primary and secondary")
> > >>
> > >> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
> > >> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > >
> > > Better to provide a small explanation.
> > >
> > >> - retval = strdup(splitstr[MOUNTPT]);
> > >> + snprintf(hugedir, len, "%s",
> > splitstr[MOUNTPT]);
> > >
> > > I think it is candidate to be replaced by strlcpy.
> > > Please check
> > >
> >
> > Yes, it seems that strlcpy thingie was merged without much fanfare. I'll
> > be submitting a patch fixing various usages of snprintf in my recent
> > commits. I'm inclined to leave this as is for this commit, as it's not
> > the purpose of this fix.
On 17-Apr-18 12:47 PM, Thomas Monjalon wrote:
> I see no reason to accept this patch, replacing strdup by snprintf,
> given that we have strlcpy.
> Please do a v3 with strlcpy.
OK. Please also fix the typo in patch headline :)
@@ -169,8 +169,8 @@
return size;
}
-static const char *
-get_hugepage_dir(uint64_t hugepage_sz)
+static int
+get_hugepage_dir(uint64_t hugepage_sz, char *hugedir, int len)
{
enum proc_mount_fieldnames {
DEVICE = 0,
@@ -188,7 +188,7 @@
const char split_tok = ' ';
char *splitstr[_FIELDNAME_MAX];
char buf[BUFSIZ];
- char *retval = NULL;
+ int retval = -1;
FILE *fd = fopen(proc_mounts, "r");
if (fd == NULL)
@@ -215,7 +215,8 @@
/* if no explicit page size, the default page size is compared */
if (pagesz_str == NULL){
if (hugepage_sz == default_size){
- retval = strdup(splitstr[MOUNTPT]);
+ snprintf(hugedir, len, "%s", splitstr[MOUNTPT]);
+ retval = 0;
break;
}
}
@@ -223,7 +224,8 @@
else {
uint64_t pagesz = rte_str_to_size(&pagesz_str[pagesize_opt_len]);
if (pagesz == hugepage_sz) {
- retval = strdup(splitstr[MOUNTPT]);
+ snprintf(hugedir, len, "%s", splitstr[MOUNTPT]);
+ retval = 0;
break;
}
}
@@ -351,7 +353,6 @@
for (dirent = readdir(dir); dirent != NULL; dirent = readdir(dir)) {
struct hugepage_info *hpi;
- const char *hugedir;
if (strncmp(dirent->d_name, dirent_start_text,
dirent_start_len) != 0)
@@ -363,10 +364,10 @@
hpi = &internal_config.hugepage_info[num_sizes];
hpi->hugepage_sz =
rte_str_to_size(&dirent->d_name[dirent_start_len]);
- hugedir = get_hugepage_dir(hpi->hugepage_sz);
/* first, check if we have a mountpoint */
- if (hugedir == NULL) {
+ if (get_hugepage_dir(hpi->hugepage_sz,
+ hpi->hugedir, sizeof(hpi->hugedir)) < 0) {
uint32_t num_pages;
num_pages = get_num_hugepages(dirent->d_name);
@@ -378,7 +379,6 @@
num_pages, hpi->hugepage_sz);
continue;
}
- snprintf(hpi->hugedir, sizeof(hpi->hugedir), "%s", hugedir);
/* try to obtain a writelock */
hpi->lock_descriptor = open(hpi->hugedir, O_RDONLY);