Char * ExpandPathName(const char* path) memory leak

ROOT
1

Dear experts,

with valgrind I am finding many

==2209== 125 bytes in 1 blocks are definitely lost in loss record 36,326 of 47,028
==2209==    at 0x4A065BA: operator new[](unsigned long) (vg_replace_malloc.c:264)
==2209==    by 0x7AF1315: StrDup(char const*) (in /cvmfs/atlas.cern.ch/repo/ATLASLocalRootBase/x86_64/root/5.34.07-x86_64-slc5-gcc4.3/lib/libCore.so)
==2209==    by 0x7B8B1C4: TUnixSystem::ExpandPathName(char const*) (in /cvmfs/atlas.cern.ch/repo/ATLASLocalRootBase/x86_64/root/5.34.07-x86_64-slc5-gcc4.3/lib/libCore.so)

I was not able to find the original StrDup, but for example in core/base/inc/TString.h there is an extern declaration with a comment:

extern char *StrDup(const char *str);        // duplicate str, free with delete []

should I free the returned char* from ExpandPathName? Please document it.

By the way, what is StrDup? I know strdup:

The strdup() function shall return a pointer to a new string, which is a duplicate of the string pointed to by s1. The returned pointer can be passed to free(). A null pointer is returned if the new string cannot be created.
2

finally I have found the function in core/base/src/TString.cxx

char *StrDup(const char *str)
{
   // Duplicate the string str. The returned string has to be deleted by
   // the user.

   if (!str) return 0;

   char *s = new char[strlen(str)+1];
   if (s) strcpy(s, str);

   return s;
}
  1. you should move this comment in all the function returning a char* created with this function
  2. why don’t use strdup?
3

[quote]
2. why don’t use strdup?[/quote]

May be, because strdup is not the part of the C/C++ standard libraries?

4

[quote=“tpochep”][quote]
2. why don’t use strdup?[/quote]

May be, because strdup is not the part of the C/C++ standard libraries?[/quote]

yes, you are right, it is posix. Probably it is not standard because of the hidden malloc.

5

Documentation of TUnixSystem::ExpandPathName seems to have lost that somewhere (at least in the online documentation), but TSystem::ExpandPathName says:

It does not, however, specify whether user should use delete or delete, which might make it easy to mix those possibly leading, as noted by some, to disastrous bugs.

In addition to the reason pointed out by tpochep, strdup returns pointer that should be deallocated with free() instead of delete. C++ library such as ROOT could perhaps have tried to avoid providing interfaces that require the use free(), although the delete-issue with new/delete style might be more of a problem.

6

Hi,

The documentation was updated. You may be interested in using:TSystem::ExpandPathName(TString &path);which avoid the need to explicit deallocation.

Cheers,
Philippe.