> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Thursday, September 28, 2006 7:51 PM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: [PATCH] stdlib patch
>
> > * sstream.cc (basic_stringbuf<>::str): Added check before
> deallocate
> > the old buffer
>
> I'm wondering about this change. Does it fix a bug?
>
> If there's a bug in the library we need an issue in Jira with
> a test case that reproduces it.
This is a bug, but it's not present in 4.1.3 release. The bug was
introduced
here: http://svn.apache.org/viewvc?view=rev&revision=380995
The fix for the bug should
> then be committed separately from any other changes. We
> should also have a test that exercises the bug. If no such
> test exists, we need to write one :)
>
> If there is no bug, can you explain how the change is beneficial?
If current capacity is enough to hold a new string, the next code
executing:
---------------- sstream.cc; line 118
else if (0 < __bufsize) {
// requested capacity is the same or less than the current one
__buf = this->_C_buffer;
__bufsize = this->_C_bufsize;
}
----------------
After that the new string copied to the current buffer, then current
buffer deallocated:
---------------- sstream.cc; line 134
if (__s != __buf) {
// copy the provided string to buffer
traits_type::copy (__buf, __s, __slen);
// here this->_C_buffer == __buf and this->_C_bufsize ==
__bufsize
if (this->_C_buffer && this->_C_own_buf ())
__alloc.deallocate (this->_C_buffer, this->_C_bufsize);
this->_C_buffer = __buf;
this->_C_bufsize = __bufsize;
}
----------------
The simple test:
#include <sstream>
#include <cassert>
int main (int, char**)
{
std::stringbuf buf;
buf.str ("123");
assert (buf.str() == "123");
buf.str ("456");
assert (buf.str() == "456");
return 0;
}
The second assert fill fail in debug configurations because the buffer
filled by debug content
after __alloc.deallocate ().
>
> > (__rw_free_what_buf): New function to free buffer for what message
>
> This function should probably be inline for efficiency, don't
> you think?
Agreed.
> > (__rw_throw_exception [!_RWSTD_NO_EXCEPTIONS]): Don't free buffer
> > before return from function
>
> Doesn't this cause a memory leak?
No, because of if __rw_throw_exception() returns the what buffer
will be deallocated in __rw_throw():
// throw_proc takes ownership of allocated string
__rw_throw_proc (id, what);
// if throw_proc returns, delete allocated what string
__rw_free_what_buf (what);
Maybe it would be better deallocate the buffer in
__rw_throw_exception()
and remove deallocation from __rw_throw() after __rw_throw_proc()
returns
to not break the sense of the comment "// throw_proc takes ownership of
allocated string"?
Also maybe __rw_free_what_buf() should be extern _RWSTD_EXPORT,
to be accessible by user in overridden __rw_throw_proc()?
---------------------
// throws an exception identified by first argument with the second
// argument containing the exception object's what() string, which
// if non-0, is dynamically allocated and must be delete[]'d
// may be assigned to a user-defined handler (e.g., to prevent
// the library from throwing exceptions or to implement logging)
extern void _RWSTD_EXPORT (*__rw_throw_proc)(int, char*);
---------------------
> FYI, I believe we have a problem here when a program sets up
> its own exception function (__rw_throw_proc) and tries to
> delete the character string that's passed to it by the
> library the delete will fail when the string is in the thread
> local buffer. I think this causes the example program
> rwexcept.cpp to crash on some platforms.
Yes, you're right.
> Does your change have anything to do with this?
No. The rwexcept.cpp test crashed due to delete[] what in
exception_handler():
-------------- rwexcept.cpp, line 33
// function called from within the library to throw an exception
void exception_handler (int id, char *what)
{
std::cerr << "exception #" << id << ": " << what << '\n';
//
if (id >= _RWSTD_ERROR_BAD_CAST)
delete[] what;
// a real program would call abort() here to prevent the potentially
// dangerous destruction of objects with static storage duration
// abort ();
// introduce exit into the current scope if it's declared in
namespace
// std (as it should be) to allow referring to the function without
// qualification when <cstdlib> incorrectly declares it at file
scope
using namespace std;
// successfully exit the process
exit (0);
// return at your own risk
}
--------------
> In any case, if it fixes a bug we need to go through the process I
outlined above
First we should decide the exception handler function is responsible
for deallocating
the what buffer or not?
Farid.
|