[omniORB] CORBA::Any::NP_marshalDataOnly assertion failure

[Peter S. Housel]

The following test program seems like it ought to work, but (in the
current CVS omni4_1_develop sources) fails with an assertion failure:

#include <omniORB4/CORBA.h>

int main(int argc, char *argv[]) {
CORBA::Any a;
a <<= CORBA::Object::_nil();
cdrMemoryStream s;
a.NP_marshalDataOnly(s);

return 0;
}

The assertion failure is:

omniORB: Assertion failed. This indicates a bug in the application
using omniORB, or maybe in omniORB itself.

file: /home/housel/sourceforge/omniorb/omni/src/lib/omniORB/dynamic/any.cc
line: 433
info: kind == CORBA::tk_void || kind == CORBA::tk_null
terminate called after throwing an instance of 'omniORB::fatalException'
Abort (core dumped)

The code in NP_marshalDataOnly seems to be asserting that only tk_null
and tk_void should be without a pd_data or pd_mbuf field, but this seems
to also be the case for nil object references.

(In my real code, CORBA::Any::NP_marshalDataOnly() is being called from
within RequestImpl::marshalArgs().)

[Duncan Grisby]

On Tuesday 20 November, "Peter S. Housel" wrote:

[...]

Post by Peter S. Housel
omniORB: Assertion failed. This indicates a bug in the application
using omniORB, or maybe in omniORB itself.
file: /home/housel/sourceforge/omniorb/omni/src/lib/omniORB/dynamic/any.cc
line: 433
info: kind == CORBA::tk_void || kind == CORBA::tk_null
terminate called after throwing an instance of 'omniORB::fatalException'
Abort (core dumped)
The code in NP_marshalDataOnly seems to be asserting that only tk_null
and tk_void should be without a pd_data or pd_mbuf field, but this seems
to also be the case for nil object references.

That's a silly bug. The code just before the assertion that's triggering
should accept tk_objref to cope with nil object references. I've fixed
it in CVS.

Cheers,

Duncan.

[Peter Housel]

Post by Duncan Grisby
[...]

Post by Peter S. Housel
omniORB: Assertion failed. This indicates a bug in the application
using omniORB, or maybe in omniORB itself.
file: /home/housel/sourceforge/omniorb/omni/src/lib/omniORB/dynamic/any.cc
line: 433
info: kind == CORBA::tk_void || kind == CORBA::tk_null
terminate called after throwing an instance of 'omniORB::fatalException'
Abort (core dumped)
The code in NP_marshalDataOnly seems to be asserting that only tk_null
and tk_void should be without a pd_data or pd_mbuf field, but this seems
to also be the case for nil object references.

That's a silly bug. The code just before the assertion that's triggering
should accept tk_objref to cope with nil object references. I've fixed
it in CVS.

That doesn't quite do it, because operator=() and the copy constructor
don't deal properly with nil object references either. Changing the
test code to:

#include <omniORB4/CORBA.h>

int main(int argc, char *argv[]) {
CORBA::Any a;
a <<= CORBA::Object::_nil();
cdrMemoryStream s;
a.NP_marshalDataOnly(s);

CORBA::Any b(a);
b.NP_marshalDataOnly(s);

CORBA::Any c = a;
c.NP_marshalDataOnly(s);

return 0;
}

yields the following failure:

omniORB: Assertion failed. This indicates a bug in the application
using omniORB, or maybe in omniORB itself.

file: /home/housel/sourceforge/omniorb/omni/src/lib/omniORB/dynamic/any.cc
line: 434
info: pd_marshal
terminate called after throwing an instance of 'omniORB::fatalException'
Abort (core dumped)

Would copying pd_marshal in the else { ... } cases of the copy
constructor operator=() be the right thing to do? Alternatively, maybe
the second branch should be "else if (a.pd_marshal) {" rather than
"else if (a.pd_data) {".

Mightn't it be a good idea to have a "pd_duplicate" member so that the
pd_data optimization could be maintained across a copy?

Also, it looks like "pd_mbuf = 0;" within operator=() is redundant,
since the call to PR_cleardata() does this already.

-Peter-

[Peter S. Housel]

Post by Peter Housel
Alternatively, maybe
the second branch should be "else if (a.pd_marshal) {" rather than
"else if (a.pd_data) {".

The patch below works for me:

Index: any.cc
===================================================================
RCS file: /cvsroot/omniorb/omni/src/lib/omniORB/dynamic/Attic/any.cc,v
retrieving revision 1.21.2.9
diff -u -u -r1.21.2.9 any.cc
--- any.cc 26 Nov 2007 16:14:35 -0000 1.21.2.9
+++ any.cc 27 Nov 2007 22:23:37 -0000
@@ -267,13 +267,11 @@
if (a.pd_mbuf) {
pd_mbuf = new cdrAnyMemoryStream(*a.pd_mbuf);
}
- else if (a.pd_data) {
+ else if (a.pd_marshal) {
// Existing Any has data in its void* pointer. Rather than trying
// to copy that (which would require a copy function to be
// registered along with the marshal and destructor functions), we
// marshal the data into a memory buffer.
- OMNIORB_ASSERT(a.pd_marshal);
-
pd_mbuf = new cdrAnyMemoryStream;
a.pd_marshal(*pd_mbuf, a.pd_data);
}
@@ -294,14 +292,10 @@
if (a.pd_mbuf) {
pd_mbuf = new cdrAnyMemoryStream(*a.pd_mbuf);
}
- else if (a.pd_data) {
- OMNIORB_ASSERT(a.pd_marshal);
+ else if (a.pd_marshal) {
pd_mbuf = new cdrAnyMemoryStream;
a.pd_marshal(*pd_mbuf, a.pd_data);
}
- else {
- pd_mbuf = 0;
- }
}
return *this;
}

[Duncan Grisby]

Post by Peter Housel
Alternatively, maybe
the second branch should be "else if (a.pd_marshal) {" rather than
"else if (a.pd_data) {".

Yes, your fix is the right thing to do, and I've checked it in. Sorry
about the continued problems with nil object references.

Cheers,

Duncan.