Re: double free or corruption - it works on older machine

[Ulrich Eckhardt]

alan.larkin@gmail.com wrote:

> Hi Ulrich.
> 
> Thanks for your interest.
> 
> Ulrich Eckhardt wrote:
>> alan.larkin@gmail.com wrote:
>> > I have software that works on an old machine:
>> > Linux 2.4.20-8smp #1 SMP i686 i686 i386 GNU/Linux
>> > gcc (GCC) 3.2.2 20030222 (Red Hat Linux 3.2.2-5)
>> >
>> > But not on a new one:
>> > Linux 2.6.15-26-amd64-generic #1 SMP PREEMPT x86_64 GNU/Linux
>> > gcc (GCC) 4.0.3 (Ubuntu 4.0.3-1ubuntu5)
>> > (also has gcc 3.3 and 3.4)
>>
>> Those numbers are mostly irrelevant, the question rather is which
>> malloc/free implementations you use. You might be able to reproduce the
>> error on the older system by upgrading the glibc.
> 
> Im afraid I have restricted access on the old machine.
> 
>> > I get a "*** glibc detected *** double free or corruption (!prev):
>> > 0x00002aaabc610420 ***" error which IMHO is a scurrilous lie ... I
dont
>> > see anything wrong with the code which has been working for a long
>> > time.
>>
>> Hahahaa. Believe me, we all had cases where we would have sworn that the
>> code is okay but it wasn't. Show the code first. Also, does it compile
>> without warnings and does it follow guidelines for safe, modern C++?
> 
> I get the following warnings from the class which I believe might be
> the source of the problem (see below) but I do not believe them to be
> significant (the j-types are from Javas jni.h):
> 
> In file included from MTWM.cpp:33:
> MTWM.h: In static member function `static int MTWM::encodeJFloat(float,
> char*)
>    ':
> MTWM.h:251: warning: converting to `unsigned int' from `jfloat'
> MTWM.h: In static member function `static int
> MTWM::encodeJDouble(double,
>    char*)':
> MTWM.h:307: warning: converting to `long int' from `jdouble'
> 
> Otherwise, I have tried to program well, but who am I to say that I
> have succeeded.
> 
>> > Things are greatly complicated by the fact that the new machine is
>> > AMD64, this software is a plugin loaded dynamically by (Sun) java so
>> > gdb isnt an option I dont think, and that I am stuck with java version
>> > 1.5.
>>
>> Why not? Of course you can attach GDB to a process while it's running.
BTW,
>> just to name an example, in C you can get away without a malloc()
>> declaration and still use it as long as a pointer has the same size as
an
>> int, while this is sure to break on e.g. 64 bit machines. Similar
>> assumptions about the size of types have proven fatal to other programs,
>> too which then broke as soon as someone put them on a 64 bit system.
>>
> So I can launch a java process ($java -XrunMYLIB ...) and attach gdb to
> the MYLIB stuff!? Any chance you know how?

Well, I think you could even run java in gdb, you might need the feature
that gdb stops execution on library load though. Otherwise, use 'ps' to
get the process-ID (a number) and use gdb's 'attach' command. The tricky
part might be to make the program not just run and finish, put a delay
loop somewhere.


>   char* encoding;
>   int length = FORREST(m_protocol)->classLoad(encoding, envID, 
>               name, srcName, numInterfaces, numMethods, methods,
>               numStatics, statics, numInstances, instances, 
>               classID, requested);
>   threadGetLocalData(envID)->getOstream()->write(encoding, length);
>   FORREST(m_protocol)->fre(encoding);

Hmmm, apart from 13 parameters being passed to classLoad(), what strikes me
most here is that there are lots of pointers being used but none are ever
checked if they aren't zero. If they can't be zero, make them references
instead.
Other than that, encoding/length form some kind of buffer, why not return a
vector<char> as parameter instead? This would rid you of the the call to
fre() (that also looks like a typo...).

> int
> MTWM::classLoad(char*& c, JNIEnv* const envID, const char* const name,
> const char* const srcName, const int numInterfaces, const int
> numMethods, const JVMPI_Method* const methods, const int numStatics,
> const JVMPI_Field* const statics, const int numInstances, const
> JVMPI_Field* const instances, const jobjectID classID, const int
> requested)
> {
>   int size = [...]

Consider using size_t. 'int' is only 32 bit even on 64 bit platforms.

>   c = new char[size];
>   if(!c)
>   {
>     /* Out of memory */
>     error("Out of memory.");
>     /* Never get here */

With a conforming C++ compiler, you would never even get to the line above
because new throws a bad_alloc exception if it fails!

[...]
>   return size;
> }
> 
> <in MTWM.h>
> public virtual void
> fre(void* arg)
> {
>     free(arg);
> }

Okay, here's your problem: you are using new[] to allocate storage and
using free() to release it. Also, even if you changed this to delete[], it
wouldn't work completely correct because calling delete[] also means
calling the destructors, but which destructors when you don't know the
type? For a char-buffer, that will be meaningless in most cases, but for
real types this will usually fail.

What I'd do first is to think about whether I need this wrapper around heap
management and if it must be overridable. If that comes out with a "yes",
I'd create two virtual functions 
  void* allocate(size_t)
  void release(void*)
that work complementary and use those functions consistently. Using them
consistently is probably easiest when temporary buffers are wrapped into a
class

class buffer
{
   MTWM* owner;
   void* ptr;
   size_t size;
public:
   [...]
};

Now, for the interface of that buffer, I'd consider making it one that
resembles that of std::auto_ptr, i.e. exclusive ownership of the resource
with a transferring copy-constructor (the copy own the buffer afterwards
while the original is reset). This allows passing this to/from a function
without having to perform expensive and usually useless copying.

Uli

-- 
http://gcc.gnu.org/faq.html
http://parashift.com/c++-faq-lite/