#10 OpenJPEG crashing

[Brian Robb]

The OpenJPEG function in corona/src/OpenJPEG.cpp crashes when you have low memory...

The following two lines are at fault:

byte* pixels = new byte[width * height * 3];
memset(pixels, 0, width * height * 3);

So I changed it to:

byte* pixels = new byte[width * height * 3];
if (!pixels)
{
jpeg_finish_decompress(&cinfo);
jpeg_destroy_decompress(&cinfo);
return 0;
}

memset(pixels, 0, width * height * 3);

Fairly obvious why it crashes :)
(pixels is returned as null, then you memset null...)

- Flik

[Chad Austin]

Logged In: YES
user_id=7212
Originator: NO

operator new only returns NULL in older, noncomformant compilers, so the right fix is to catch std::bad_alloc, imo.

[Brian Robb]

Logged In: YES
user_id=387078
Originator: YES

So is it going to be fixed?

There's no try / catch for std::bad_alloc anywhere...
corona.dll crashes when you open an image in sphere's editor at the moment...

[Chad Austin]

Logged In: YES
user_id=7212
Originator: NO

I don't have time to fix it, but you could make that change if you want. Also, in the past, I've done stuff like this to get the correct std::bad_alloc behavior in VC6:

void* operator new(size_t size) {
void* p = malloc(size);
if (p) {
return p;
} else {
throw std::bad_alloc();
}
}

void operator delete(void* p) {
free(p);
}

Then you can just catch std::bad_alloc at the top of the OpenImage call stack.

[Chad Austin]

Logged In: YES
user_id=7212
Originator: NO

Also, hey Flik! How's it going?!

(I meant to write that in my last message, but forgot!)

[Brian Robb]

Logged In: YES
user_id=387078
Originator: YES

Hrm, but what's the point in catching the bad_alloc at the top of the stack?

///////

void MainFunction() {
try {
SubFunction();
}
catch (std::bad_alloc)
{

}
}

void SubFunction()
{
byte* data1 = new byte[50000];

// if bad_alloc gets thrown here,
// what happens to data1?
byte* data2 = new byte[100000];

delete[] data2;
delete[] data1;
}

///////

So unless you can explain how to make sure data1 gets cleaned up if SubFunction fails when the allocation for data2 fails, I can't really bring myself to do it.

The memory allocated (since we're dealing with images) is normally going to be quite large, so what would you think of catching it straight away?

e.g.

byte* pixels = NULL;

try {
pixels = new byte[size];
}
catch (std::bad_alloc)
{
pixels = NULL;
}

Which is a bit too much code to have on each allocation attempt,
maybe a DEFINE which looks like TRY_CATCH(pixels = new byte[size]); ?

The other idea would be to change using new/delete to malloc/free,
then we wouldn't need any exceptions at all...

What do you think?

[Chad Austin]

Logged In: YES
user_id=7212
Originator: NO

Catching the error right away is okay, but requires changes everywhere that allocation is done. What's better is a technique called RAII (resource acquisition is initialization):

http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

You'd need a class like ScopedArray ( http://empyrean.cvs.sourceforge.net/empyrean/empyrean/src/base/ScopedArray.h?revision=1.2&view=markup ), but with a release() method that sets the internal pointer to 0 (releasing ownership) and then returns it.

[Brian Robb]

Patch for added allocation checks

[Brian Robb]

Logged In: YES
user_id=387078
Originator: YES

ScopedArray - There's a class called auto_array in corona...

I've changed all the allocation attempts to catch for failures.
And tested it as much as I can, so I'm happy to use the corona.dll it compiles to.

I've added a patch file: added-allocation-checks.patch

I haven't added try/catch's for std::bad_alloc
(There's no point since msvc++ v6 doesn't throw those errors)

You can close the bugs "OpenPNG crashing" and "ConvertPixels crashing" if you like, since they're basically the same low-memory null pointer style crash...

(This patch fixes all the low-memory null pointer style crashes, for me at least.)

[Chad Austin]

Logged In: YES
user_id=7212
Originator: NO

I don't like having to put additional error checking in tons of places across the library, especially since it's for an ancient, broken compiler. Exceptions are definitely better, and you can "fix" VC6 by defining custom operator new and delete:

void* operator new(size_t size) {
void* p = malloc(size);
if (p) {
return p;
} else {
throw std::bad_alloc();
}
}

void operator delete(void* p) {
free(p);
}

Here's a patch that does that. Can you put a catch (std::bad_alloc) in Open/SaveImage/etc. and see if it's a more localized change?

Also, I'm happy to get you access to VC7 or 8 if you'd like.

Oh yeah, and I changed Corona to use Subversion instead of CVS today. Just so you know.

[Chad Austin]

trick's VC6's operator new and delete into throwing std::bad_alloc