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

[Roman Kennke]

Hi Mark,

Am Donnerstag, den 19.01.2006, 02:12 +0100 schrieb 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.

Granted, the example is a little constructed. The point is not if you
get the crash in f2() anyway. The point is that the asserts help you
find the actual bug. In a real world environment you may trash the stack
anywhere, not only one function call up. The asserts help you find out,
'Oh, there's something wrong in f2() but in f1() it was ok, so the error
must be in f1()'. Imagine such a scenario when there are more function
calls in between etc and you quickly see that the asserts actually make
sense.

OTOH, when you say the asserts don't make sense for argument checking,
can you give a good example of a use case for asserts? Or are you
opposed to using asserts in general?


> > 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.

Indeed, that was the problem and IMO it should have been fixed that way.
I don't really get why you feel that the asserts don't by us anything.
Isn't it good to make sure that the arrays have the correct size before
accessing them? Sure, if we don't check we crash anyway, as we would
with the asserts in, but with the asserts enabled (which only happens in
debug mode anyway AFAIK) you have a better chance to find the actual
bug. In my experience the usage of asserts is extremely helpful and I
feel that if anything, we should put more asserts in the code instead of
removing them.

Roman

signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil