stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Farid Zaripov" <Far...@kyiv.vdiweb.com>
Subject RE: [PATCH] stdlib patch
Date Fri, 29 Sep 2006 12:53:45 GMT
> -----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.

Mime
View raw message