Hi, pidl gurus!
Coverity has discovered a class of flaws in the generated
NDR marshalling code.
If you look at gen_ndr/ndr_echo.c:1166 we dereference
r->in.foo1 but in line 1174 we check if that's NULL. So
either the check in 1174 is unnecessary or should come
before 1166. Foo1 in this case is a ref pointer, but a
broken user of this API should not segfault I think, so I
would tend to move the check to line 1166.
But this goes a bit beyond my pidl knowledge.
Comments?
Volker
PGP SIGNATURE
Version: GnuPG v1.4.2 (GNU/Linux)
Q18E4gtNwlc3uCWiCblftV0=
=/JN1
PGP SIGNATURE

Luke,

put some assertion checks in?

time for my bi-yearly rant about assertions on NULL dereference :)

Unix and unix-like systems provide a highly efficient, zero code
assertion system. It interfaces to debuggers, it correctly propogates
to parent processes and it is even implemented in hardware on nearly
every system in existance.

It's called the segmentation fault. It's wonderful. Use it :-)

Cheers, Tridge

Wed, 25, 2006 at 12:35:56PM +1000, tridge (AT) samba (DOT) org wrote:
Unix and unix-like systems provide a highly efficient, zero code
assertion system. It interfaces to debuggers, it correctly propogates
to parent processes and it is even implemented in hardware on nearly
every system in existance.

It's called the segmentation fault. It's wonderful. Use it :-)

But then the check for NULL is bogus. Should we remove it or
shouldn't we? I could certainly mark all these Coverity
errors as false positives, but I do think a NULL check after
having dereferenced a pointer is kind of void.

Volker

PGP SIGNATURE
Version: GnuPG v1.4.2 (GNU/Linux)

Kd+xojCuylnxR1Pbwjqa+AE=
=kJHU
PGP SIGNATURE

Volker,

But then the check for NULL is bogus. Should we remove it or
shouldn't we? I could certainly mark all these Coverity
errors as false positives, but I do think a NULL check after
having dereferenced a pointer is kind of void.

In my attempt to be humorous I left off the obvious exception -
anything that could be an allocation failure obviously does need to be
checked. It's not K to segv on allocation failure (or failing to
mmap, failing to open a file etc etc). So I think we do need one check
when we allocate a new attribute, but if the attribute does get added
K then we don't need to check the case that its null afterwards.

I just don't like the use of assert() calls for NULL checks, when the
assert call causes a panic or similar shutdown. That's pointless,
makes the code larger and slower, and is generally a bad thing ;-)

btw, does coverity do inter-module checking? ie. does it notice that
a function passing in ldb_msg.c implies that a different function in a
different module can't fail? I know the ibm checker does some of that
(tho not perfectly) but I don't know if coverity does.

Cheers, Tridge

Wed, 25, 2006 at 08:20:54PM +1000, tridge (AT) samba (DOT) org wrote:
In my attempt to be humorous I left off the obvious exception -
anything that could be an allocation failure obviously does need to be
checked. It's not K to segv on allocation failure (or failing to
mmap, failing to open a file etc etc). So I think we do need one check
when we allocate a new attribute, but if the attribute does get added
K then we don't need to check the case that its null afterwards.

I was not talking about ldb, I was talking about generated
NDR marshalling code. In ndr_echo.c starting with line 1162
we have the following code fragment:

ndr_print_echo_Enum1(ndr, "foo1", *r->in.foo1);
ndr->depth--;
ndr_print_ptr(ndr, "foo2", r->in.foo2);
ndr->depth++;
ndr_print_echo_Enum2(ndr, "foo2", r->in.foo2);
ndr->depth--;
ndr_print_ptr(ndr, "foo3", r->in.foo3);
ndr->depth++;
if (r->in.foo1 == NULL) return;

I think the last line is bogus, as in the first line we
already segfaulted. should I mark these as wontfix in
coverity?

Volker

Volker,

I was not talking about ldb, I was talking about generated
NDR marshalling code. In ndr_echo.c starting with line 1162
we have the following code fragment:

oh, sorry!

ndr_print_echo_Enum1(ndr, "foo1", *r->in.foo1);
ndr->depth--;
ndr_print_ptr(ndr, "foo2", r->in.foo2);
ndr->depth++;
ndr_print_echo_Enum2(ndr, "foo2", r->in.foo2);
ndr->depth--;
ndr_print_ptr(ndr, "foo3", r->in.foo3);
ndr->depth++;
if (r->in.foo1 == NULL) return;

I think the last line is bogus, as in the first line we
already segfaulted. should I mark these as wontfix in
coverity?

yes, last line is bogus. It should be fixable in pidl. I'll leave it
to Jelmer to see just how painful the fix is (I don't think it will be
bad - just need to check for a ref ptr and avoid generating that NULL
check).

Is it ok to leave this in coverity until Jelmer has a chance to have a
look at a fix?

Cheers, Tridge

Wed, 25, 2006 at 09:56:00PM +1000, tridge (AT) samba (DOT) org wrote:
Is it ok to leave this in coverity until Jelmer has a chance to have a
look at a fix?
Sure.
Volker

David,
It should be seriously easy to write an equivalent one for Linux.
it is :-)
see and
http://junkcode.samba.org/#segv_handler
Cheers, Tridge

Hi Volker,

Tue, 24, 2006 at 10:43:24PM +0200, Volker Lendecke wrote:
Coverity has discovered a class of flaws in the generated
NDR marshalling code.

If you look at gen_ndr/ndr_echo.c:1166 we dereference
r->in.foo1 but in line 1174 we check if that's NULL. So
either the check in 1174 is unnecessary or should come
before 1166. Foo1 in this case is a ref pointer, but a
broken user of this API should not segfault I think, so I
would tend to move the check to line 1166.

But this goes a bit beyond my pidl knowledge.

Comments?
I've just fixed this by simply removing the check completely. This
code would've only been triggered in case of invalid API usage and in
that case would've silently returned without any errors.

Cheers,

Jelmer

PGP SIGNATURE
Version: GnuPG v1.4.5 (GNU/Linux)

vX12FYzdSc+AK/HSCZa1Skk=
=henN
PGP SIGNATURE

Fri, Nov 03, 2006 at 08:58:37PM +0100, Jelmer Vernooij wrote:
But this goes a bit beyond my pidl knowledge.

Comments?
I've just fixed this by simply removing the check completely. This
code would've only been triggered in case of invalid API usage and in
that case would've silently returned without any errors.

Thanks!

Would you mind to re-generate the Samba3 part and check it
in?

Volker

PGP SIGNATURE
Version: GnuPG v1.4.2 (GNU/Linux)

VqyTnIc75K9G84kLMATCMi8=
=rLCX
PGP SIGNATURE