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

[Roman Kennke]

Hi Mark,

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:

typedef struct
{
   int counter;
} Tenv;

void f2(Tenv *env, const char *s)
{
   assert(env!=NULL);
   if (env->counter>10) printf(s);
}

void f1(Tenv *env)
{
   char a[20];

   assert(env!=NULL);

   memset(a,0,100);
   strcpy(a,"Hello World");
   f2(env,s);
}


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() ;-)

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.

/Roman

signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil