Re: Changes in marshalling code between 0.5.3 and 0.5.6?

[Owen Taylor <otaylor redhat com>]

Roland Mas <mas echo fr> writes:

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

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

========================
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;
 
+#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 == NULL)
     return;
 
-  if(buffer->message_body) {
-    buffer->message_body = ((guchar *)buffer->message_body)
-      - sizeof(GIOPMessageHeader);
-    
-    g_free(buffer->message_body);
-    buffer->message_body = NULL;
-  }
+  if(buffer->message_body)
+    {
+      g_free(buffer->message_body);
+      buffer->message_body = NULL;
+    }
 
   if(GIOP_MESSAGE_BUFFER(buffer)->connection->incoming_msg == buffer)
     GIOP_MESSAGE_BUFFER(buffer)->connection->incoming_msg = NULL;
@@ -888,11 +888,10 @@
          goto errout;
        }
 
-       retval->message_body = g_malloc(message_size+sizeof(GIOPMessageHeader));
+       retval->message_body = g_malloc(message_size+sizeof(GIOPMessageHeader)+4);
        /* XXX1 This is a lame hack to work with the fact that
           alignment is relative to the MessageHeader, not the RequestHeader */
-       retval->message_body = ((guchar *)retval->message_body) + sizeof(GIOPMessageHeader);
-       retval->cur = retval->message_body;
+       retval->cur = retval->message_body + 12;
        retval->state = GIOP_MSG_READING_BODY;
        retval->left_to_read = message_size;
        break;
@@ -1131,7 +1130,7 @@
 if(!( (( ((guchar*)GIOP_RECV_BUFFER(buf)->cur) \
                        + (requested_increment) ) \
                       <= ( ((guchar *)GIOP_RECV_BUFFER(buf)->message_body) \
-                          + GIOP_MESSAGE_BUFFER(buf)->message_header.message_size)) \
+                          + GIOP_MESSAGE_BUFFER(buf)->message_header.message_size) + 12) \
                      && ( ( ((guchar*)GIOP_RECV_BUFFER(buf)->cur) \
                            + (requested_increment) ) \
                          >= ((guchar*)GIOP_RECV_BUFFER(buf)->cur) ))) goto out
=================

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) == 12. Though if that weren'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.

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

        CDR_codec_init_static(codec);
        codec->buffer=buf->cur;
        codec->release_buffer=CORBA_FALSE;
        codec->readonly=CORBA_TRUE;
        codec->buf_len =                /* hope this is correct */
          ((guchar *)buf->message_body) +
          GIOP_MESSAGE_BUFFER(buf)->message_header.message_size
          - ((guchar *)buf->cur);
                
        codec->data_endian=GIOP_MESSAGE_BUFFER(buf)->message_header.flags & 1;

        ctx.current_idx=0;
        ctx.prior_tcs=NULL;
        tc_dec(t, codec, &ctx);
        for(l=ctx.prior_tcs;l;l=l->next)
                g_free(l->data);
        g_slist_free(ctx.prior_tcs);
        buf->cur = ((guchar *)buf->cur) + codec->rptr;
}
======

That hope is now pretty forlorn.

Regards,
                                        Owen