orbit-python-list

[Orbit-python-list] Changes in marshalling code between 0.5.3 and 0.5.6?

[Roland M. <ma...@ec...>]

  Hello people,

[Cc: to the ORBit-Python developers list]

  As a developer for ORBit-Python, a Python binding for ORBit, I'm
encountering a problem.  You could even call that a bug, but I'm not
casting the blame to anyone (yet >:-] ).

  Basically, I get warnings like
** WARNING **: incomplete message received
in my client, which results in a segmentation fault in the client.
The server seems to run OK (no segfault).

  But, where this becomes interesting is that this behaviour appears
with ORBit 0.5.6, I seem to remember it also did with 0.5.5, and it
does not happen with 0.5.3 (I kept that one because I knew it worked).
Compile and run with 0.5.3 OK, compile and run with 0.5.6 -> segfault.
The same ORBit-Python source, the same test case.

  The test-case is the following:

typedef sequence<long> seq ;
interface Instance {
[...]
  seq gimme_list ();
};

  The test server picks a random number n, say n=4, and returns the
sequence (0, 1, 2, 3).  When n=0 (empty list), the call returns OK,
result is an empty list as expected.  When n=1 (list with only one
element, the long 0), the call returns OK, the result is the list
containing one element, that is 0, as expected.  When n>1, shit
happens with 0.5.6, not with 0.5.3.

  Let's change seq to be a sequence<short>.  The limit is now 3: n<=3
OK, n>3 NOK.  long long?  n=0 OK, n>0 NOK.  float?  n<=1 OK, n>1 NOK.
For strings, the problem seems a bit different, and the limit depends
on the length of the strings that the server returns.

  So, I'd be interested in knowing what changes were made in the
marshalling code.  What I'd love most of all would be someone standing
up and saying "Yea, I plead guilty, silly bug, here's the two-line
patch to ORBit", but I won't hold my breath for it.  The size
limitation induces me into thinking that there's some buffer size
problem somewhere...  I will once again review the marshalling code in
ORBit-Python, but I'd be inclined to think it is correct (after all,
it works with an older ORBit, doesn't it?).

  Thanks for your attention,

Roland.
-- 
Roland Mas

Mou ichido !  Hayaku !  Ookii koede !
  -- Atsuko Sasaki

[Orbit-python-list] Re: Changes in marshalling code between 0.5.3 and 0.5.6?

[Roland M. <ma...@ec...>]

Roland Mas (2001-01-26 23:02:51 +0100) :

>   As a developer for ORBit-Python, a Python binding for ORBit, I'm
> encountering a problem.  You could even call that a bug, but I'm not
> casting the blame to anyone (yet >:-] ).

  Well, after a bit of hacking, it appears that given a
GIOPRecvBuffer *buf, the attribute
GIOP_MESSAGE_BUFFER(buf)->message_header.message_size is too small by
twelve bytes.  "Coincidentally", twelve is sizeof(GIOPMessageHeader)
too.

  So, we patched ORBit-Python to add this sizeof() to the message_size
when checking for availability of bytes in the buffer, but it still
feels uncomfortable.  So I'd like a confirmation that what we did was
The Right Thing, and not a dirty hack to fix a broken behaviour in
ORBit.  Should this be a broken behaviour, please keep me informed
when it is fixed, so that we can clean the hack.

  Have a nice day,
Roland.
-- 
Roland Mas

Depuis 1977.

[Orbit-python-list] Re: Changes in marshalling code between 0.5.3 and 0.5.6?

[Owen T. <ot...@re...>]

Roland Mas <ma...@ec...> writes:

> Roland Mas (2001-01-26 23:02:51 +0100) :
>=20
> >   As a developer for ORBit-Python, a Python binding for ORBit, I'm
> > encountering a problem.  You could even call that a bug, but I'm not
> > casting the blame to anyone (yet >:-] ).
>=20
>   Well, after a bit of hacking, it appears that given a
> GIOPRecvBuffer=A0*buf, the attribute
> GIOP_MESSAGE_BUFFER(buf)->message_header.message_size is too small by
> twelve bytes.  "Coincidentally", twelve is sizeof(GIOPMessageHeader)
> too.
>=20
>   So, we patched ORBit-Python to add this sizeof() to the message_size
> when checking for availability of bytes in the buffer, but it still
> feels uncomfortable.  So I'd like a confirmation that what we did was
> The Right Thing, and not a dirty hack to fix a broken behaviour in
> ORBit.  Should this be a broken behaviour, please keep me informed
> when it is fixed, so that we can clean the hack.

Well, to shed a little more light on the issue, the root cause
of the problem was Elliot's change of Nov 9:

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
revision 1.117.4.3
date: 2000/11/09 20:14:05;  author: sopwith;  state: Exp;  lines: +10 -11

src/IIOP/giop-msg-buffer.c: Avoid a memory corruption problem on the Alpha =
by doing
some magic voodoo that makes the symptoms go away. No clue what the actual =
cause is.

diff -u -r1.117.4.2 -r1.117.4.4
--- giop-msg-buffer.c   2000/10/27 19:40:52     1.117.4.2
+++ giop-msg-buffer.c   2000/11/15 02:10:36     1.117.4.4
@@ -56,6 +56,8 @@
   GIOP_octet flags;
 } PACKED;
=20
+#include <stdlib.h>
+
 /* functions */
 static gint giop_recv_decode_message(GIOPRecvBuffer *buf);
 static gboolean num_on_list(GIOP_unsigned_long num,
@@ -754,13 +756,11 @@
   if (buffer =3D=3D NULL)
     return;
=20
-  if(buffer->message_body) {
-    buffer->message_body =3D ((guchar *)buffer->message_body)
-      - sizeof(GIOPMessageHeader);
-=20=20=20=20
-    g_free(buffer->message_body);
-    buffer->message_body =3D NULL;
-  }
+  if(buffer->message_body)
+    {
+      g_free(buffer->message_body);
+      buffer->message_body =3D NULL;
+    }
=20
   if(GIOP_MESSAGE_BUFFER(buffer)->connection->incoming_msg =3D=3D buffer)
     GIOP_MESSAGE_BUFFER(buffer)->connection->incoming_msg =3D NULL;
@@ -888,11 +888,10 @@
          goto errout;
        }
=20
-       retval->message_body =3D g_malloc(message_size+sizeof(GIOPMessageHe=
ader));
+       retval->message_body =3D g_malloc(message_size+sizeof(GIOPMessageHe=
ader)+4);
        /* XXX1 This is a lame hack to work with the fact that
           alignment is relative to the MessageHeader, not the RequestHeade=
r */
-       retval->message_body =3D ((guchar *)retval->message_body) + sizeof(=
GIOPMessageHeader);
-       retval->cur =3D retval->message_body;
+       retval->cur =3D retval->message_body + 12;
        retval->state =3D GIOP_MSG_READING_BODY;
        retval->left_to_read =3D message_size;
        break;
@@ -1131,7 +1130,7 @@
 if(!( (( ((guchar*)GIOP_RECV_BUFFER(buf)->cur) \
                        + (requested_increment) ) \
                       <=3D ( ((guchar *)GIOP_RECV_BUFFER(buf)->message_bod=
y) \
-                          + GIOP_MESSAGE_BUFFER(buf)->message_header.messa=
ge_size)) \
+                          + GIOP_MESSAGE_BUFFER(buf)->message_header.messa=
ge_size) + 12) \
                      && ( ( ((guchar*)GIOP_RECV_BUFFER(buf)->cur) \
                            + (requested_increment) ) \
                          >=3D ((guchar*)GIOP_RECV_BUFFER(buf)->cur) ))) go=
to out
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

This is indeed magic voodoo, and I don't think changing the interpretation
of retval->message_body was necessary -- the reason why I don't think
it is necessary, is that the only places that used it were the
places that broke: orbit-perl, orbit-python, and ORBit. Yes, ORBit is
currently broken. (See code below)

The two possibilities of things that might have actually been fixed here
were:

1) Allocating message_body bigger (though that could only have covered
   up bugs elsewhere.)

2) Not counting on sizeof(GIOPMessageHeader) =3D=3D 12. Though if that were=
n't
   the case, the breakage would not be subtle.

Though perhaps changing message_body back is worse than the just leaving it
as it is now.

=3D=3D=3D=3D=3D=3D=3D
void ORBit_decode_CORBA_TypeCode(CORBA_TypeCode* t, GIOPRecvBuffer* buf)
{
        CDR_Codec codec_d;
        CDR_Codec* codec =3D &codec_d;
        TCDecodeContext ctx;
        GSList* l;

        CDR_codec_init_static(codec);
        codec->buffer=3Dbuf->cur;
        codec->release_buffer=3DCORBA_FALSE;
        codec->readonly=3DCORBA_TRUE;
        codec->buf_len =3D                /* hope this is correct */
          ((guchar *)buf->message_body) +
          GIOP_MESSAGE_BUFFER(buf)->message_header.message_size
          - ((guchar *)buf->cur);
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20
        codec->data_endian=3DGIOP_MESSAGE_BUFFER(buf)->message_header.flags=
 & 1;

        ctx.current_idx=3D0;
        ctx.prior_tcs=3DNULL;
        tc_dec(t, codec, &ctx);
        for(l=3Dctx.prior_tcs;l;l=3Dl->next)
                g_free(l->data);
        g_slist_free(ctx.prior_tcs);
        buf->cur =3D ((guchar *)buf->cur) + codec->rptr;
}
=3D=3D=3D=3D=3D=3D

That hope is now pretty forlorn.

Regards,
                                        Owen