Re: [cp-patches] FYI: Remove (bogus) asserts in java-net.

[Mark Wielaard]

Hi Roman,

On Wed, 2006-01-18 at 12:20 +0100, Roman Kennke wrote:
> Am Donnerstag, den 12.01.2006, 10:36 +0100 schrieb Mark Wielaard:
> > Hi,
> > 
> > The crashes we were seeing with the net code in Mauve were caused by the
> > following bogus asserts in VMPlainDatagramSocketImpl_nativeReceive():
> > 
> >   assert((*env)->GetArrayLength(env, receivedFromAddress) > 4);
> >   assert((*env)->GetArrayLength(env, receivedFromPort   ) > 1);
> >   assert((*env)->GetArrayLength(env, receivedLength     ) > 1);
> > 
> > Removing these makes most net tests work again. I am working on the
> > others. I also took the oppertunity to remove all other asserts() in
> > this code since they don't really add anything. Checking whether the
> > JNIEnv is not NULL isn't really that much help since if that is the case
> > lots of things will just break since it means the runtime is completely
> > broken.
> 
> AFAICS, it is always helpful to assert parameters in function calls,
> even the env parameter. I spoke to Torsten about that and he put
> together a nice piece of example code that demonstrates the problem,
> even when the runtime is not broken. The assert statements are very
> helpful to find such errors early:
> [...]
> Without the asserts you would get a segfault in f2() in the line with
> the printf() statement and most likely don't immediately see what is
> wrong there. With the asserts on you would see that env is NULL in f2()
> but not in f1(). Obviously we have trashed the stack somewhere in
> f1() ;-)

I find the example a bit contrived. You will get a crash anyway in f2()
since you trashed env and are using it. Whether the crash is because of
the assert (which only checks for null) or because of actual usage
(which will catch much more cases) doesn't really matter that much imho.

> It is generally a good idea to check arguments with asserts. It doesn't
> hurt (except in the cases mentioned by you that made the Mauve tests
> fail, where we better look why the asserts fail, maybe they are wrong
> and need to be adjusted, and not remove the asserts) and makes the code
> safer (and more secure, think of buffer overflow attacks and stuff like
> that).
> 
> I would vote for taking the asserts back in and investigating why the
> specific VMPlainDatagramSocketImpl_nativeReceive() asserts make the
> Mauve tests crash.

The asserts failed because they were testing the wrong condition (they
test for > instead of ==). This code is so tightly bound to the private
VM method that I don't feel argument assert checking buys us anything.

Cheers,

Mark

signature.asc
Description: This is a digitally signed message part