Is there a memory leakage in this simple code snippet

does strdup cause memory leakage?

Code:

---

    string str("/usr/bin/");                                                                                                                                               
    string str2 = string(dirname(strdup(str.c_str())));                                                                                                                   
    cout<<str2<<endl;

---

I modified the code as:

Code:

---

    string str("/usr/bin/");                                   
    char * tmp = strdup(str.c_str());                                                                                                                                       
    string str2 = string(dirname( tmp ));                                                                                                                                   
    delete(tmp);

---

Using valgrind it reports:

Quote:

---

==18859== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 17 from 1)
==18859== malloc/free: in use at exit: 0 bytes in 0 blocks.
==18859== malloc/free: 3 allocs, 3 frees, 49 bytes allocated.
==18859== For counts of detected errors, rerun with: -v
==18859== No malloc'd blocks -- no leaks are possible.

---

try 'string(dirname( *tmp ));' instead ?

strdup is nonstandard, I think, and use free, not delete.

The first example is a memory leak, as you never free the result of strdup.

In the second code-snippet, strdup uses malloc, so you should use free to free that memory.

Also, there is absolutely no need to copy the string with strdup before calling dirname, as dirname has a local static variable that it returns as the result - and std::string will copy the string itself. However, If you are using multiple threads that may call dirname, you will need to make sure that the result from dirname is copied whilst in a lock situation.

--
Mats

No, as a matter of fact, you have to use strdup here.
If your code is:

Code:

---

string str2 = string(dirname(str.c_str()));

---

You will get errors from compiler:

Quote:

---

main.cc: In function `int main(int, char**)':
main.cc:25: error: invalid conversion from `const char*' to `char*'

---

Quote:

---

Originally Posted by matsp

The first example is a memory leak, as you never free the result of strdup.

In the second code-snippet, strdup uses malloc, so you should use free to free that memory.

Also, there is absolutely no need to copy the string with strdup before calling dirname, as dirname has a local static variable that it returns as the result - and std::string will copy the string itself. However, If you are using multiple threads that may call dirname, you will need to make sure that the result from dirname is copied whilst in a lock situation.

--
Mats

---

Perhaps you could use this to avoid worrying about the free/delete:

Code:

---

    string str("/usr/bin/");
    string str2 = string(dirname( &(std::vector<char>(str.begin(), str.end()))[0] ));

---

How about just

Code:

---

string str2 = string( const_cast<char*>(dirname( str.c_str() )) );

---

EDIT: Bad suggestion.

Quote:

---

Originally Posted by Elysia

How about just

Code:

---

string str2 = string( const_cast<char*>(dirname( str.c_str() )) );

---

---

No, dirname() is actually allowed to modify the parameter, not just legacy const-incorrect.

Elysia, I do not think your example is correct, since the const_cast is applied to the return value of dirname(), but of course there is no need to do that. The const_cast should be applied to the argument to dirname(), but then the assumption is that dirname() does not modify its argument.

I need a prototype for dirname, since obviously std::string should be able to accept any string passed to it.
Dang. No prototype for dirname >_<
Then it's beyond me why you have to use such pesky workarounds.

Quote:

---

I need a prototype for dirname, since obviously std::string should be able to accept any string passed to it.
Dang. No prototype for dirname >_<

---

RTFM: dirname :)

Funny how just useless results showed up on google...
But I see the problem.
OK, so wouldn't this work then?

Code:

---

    string str("/usr/bin/");
    string str2 = std::string( dirname(&str.begin()[0]) );

---

Quote:

---

OK, so wouldn't this work then?

---

No, you're not allowed to rely on &str[0] or even &str.begin()[0] being continuous memory, or being in a plain format that is safe to modify.

Quote:

---

Funny how just useless results showed up on google...

---

When I typed "man dirname", laserlight's suggestion was the first result.
When I typed just "dirname", the third result was the specification of dirname from the OpenGroup. (They don't actually spell it out that the function may modify the input.)

Naturally the stupid std::string doesn't even have a function to access its buffer without a lot of hacks or workarounds.
So much for flexibility of the standard library.

It's a rule that's there so that the implementors have flexibility.

So, the correct solution, hidden in an overloaded function, would be:

Code:

---

std::string dirname(const std::string &fullname)
{
    char *tmp = strdup(fullname.c_str());
    std::string result(dirname(tmp));
    free tmp;
    return result;
}

---

--
Mats

No, this leaks if std::string's constructor throws. The correct solution is

Code:

---

vector<char> tmp(fullname.c_str(), fullname.c_str() + fullname.size() + 1);
return std::string(dirname(&tmp[0]));

---

Alternatively (more for my own practice, really):

Code:

---

std::string dirname(const std::string &fullname)
{
    char *tmp = strdup(fullname.c_str());
    try {
      std::string result;
      result = dirname(tmp);
    }
    catch(...)
    {
        free tmp;
        throw;
    }
    free(tmp);
    return result;
}

---

But it's a bit messy.

--
Mats

And your forgot to free the string in the non-error case.

I believe an auto_ptr may be in place?

Quote:

---

Originally Posted by CornedBee

And your forgot to free the string in the non-error case.

---

Of course... :-(

--
Mats

Quote:

---

Originally Posted by Elysia

I believe an auto_ptr may be in place?

---

It would apply delete to the malloced memory of strdup, so no. If anything, it would have to be

Code:

---

std::tr1::shared_ptr<char> tmp(strdup(input.c_str()), std::free);
... dirname(tmp.get())

---

What's wrong with the vector, though?

I find the vector an even bigger hack, but one I'd prefer over mats's slightly longer code.
The best solution would just be to have a function that returns a buffer, char* or wchar_t* from std::string, however. But once again we are not as lucky.
I find it funny how the things in the standard library seems to be more oriented towards a C interface rather than a C++ interface (just look at the streams and how they only accept char* or wchar_t*), but with std::string, it's lacking a critical function for C compability.

Quote:

---

Originally Posted by Elysia

I find the vector an even bigger hack, but one I'd prefer over mats's slightly longer code.
The best solution would just be to have a function that returns a buffer, char* or wchar_t* from std::string, however. But once again we are not as lucky.
I find it funny how the things in the standard library seems to be more oriented towards a C interface rather than a C++ interface (just look at the streams and how they only accept char* or wchar_t*), but with std::string, it's lacking a critical function for C compability.

---

Or, we can do it this way:

Code:

---

std::string dirname(const std::string &fullname)
{
    char tmp[MAX_PATH+1];
    strncpy(tmp, fullname.c_str(), MAX_PATH);
    tmp[MAX_PATH] = 0; // Make sure there is a zero somewhere!
    std::string result(dirname(tmp));
    return result;
}

---

Despite what you may think, this is better than strdup, since strdup will do strlen first, then strcpy, where this code only does strncpy(). It may take 4 or 8KB of stack-space, but it eliminates the need to allocate dynamic memory (and thus handle problems with freeing it).

--
Mats

And "taking 4k of stack space" in a near-leaf function such as this only means modifying the stack pointer, so yeah, that's a good way.

Of course, MAX_PATH is Windows-only. There's a PATH_MAX in limits.h on POSIX.

Quote:

---

Originally Posted by CornedBee

And "taking 4k of stack space" in a near-leaf function such as this only means modifying the stack pointer, so yeah, that's a good way.

Of course, MAX_PATH is Windows-only. There's a PATH_MAX in limits.h on POSIX.

---

Darn, I always get that one wrong.

And yes, modifying the stack-pointer should be pretty fast - probably 5x faster than malloc.

--
Mats

What's a near-leaf function?

A leaf function is one that calls no other functions. A near-leaf function was an on-the-spot word creation meaning a function that calls only leaf functions.

Quote:

---

Originally Posted by robwhit

What's a near-leaf function?

---

It's a function that is near the "end" of a call-tree (main being the "root" of such a tree - at leas from the general programmers, as we don't normally consider the code that calls main() as part of the program, even if it technically is). A leaf-funciton is one that doesn't call any other functions, and a "near leaf" is one that only calls leaf-functions in itself. [Although, I suppose that the std::string::string(const char *) constructor is likely to call at least the new operator.

--
Mats

Here's a version that doesn't depend on strdup or the POSIX call:

Code:

---

std::string dirname(const std::string & path)
{
    std::string dir = path.substr(0, path.find_last_of("/"));

    return ((dir != path) ? dir : std::string("."));
}

---

Quote:

---

Originally Posted by medievalelks

Here's a version that doesn't depend on strdup or the POSIX call:

Code:

---

std::string dirname(const std::string & path)
{
    std::string dir = path.substr(0, path.find_last_of("/"));

    return ((dir != path) ? dir : std::string("."));
}

---

---

Although that wouldn't work on a system that has different directory separators than '/', e.g a Windows system which uses : and \, or one of them systems that uses : only.

But the idea is good, and it's definitely simple.

--
Mats

Here's a more portable version:

Code:

---

std::string dirname(const std::string & path, const std::string & sep)
{
    std::string dir = path.substr(0, path.find_last_of(sep));

    return ((dir != path) ? dir : std::string("."));
}

---