Wireshark · Wireshark-dev: Re: [Wireshark-dev] [PATCH] LIBNDR_FLAG_NOALIGN support in wireshark and PIDL

[Julien Kerihuel <j.kerihuel@xxxxxxxxxxxxxx>]

On Wed, 2010-01-20 at 08:50 +1100, ronnie sahlberg wrote:
> Can you send me your new di->no_align patch and Ill check it in right
> now.
> 
> I started applying it yesterday but modified it (to ensure we always
> initialize di->no_align in get_next_di(), but will reverse these local
> changes
> so they wont collide with your patch.)
> 
> 
> This patch can be applied right now, in anticipation of adding the
> pidl patches later.

Ronnie,

Thanks I was looking for a correct place to initialize the variable.
The new version of the patch is attached to this e-mail.

Cheers,
Julien.

> 
> 
> 
> 
> On Wed, Jan 20, 2010 at 1:58 AM, Julien Kerihuel
> <j.kerihuel@xxxxxxxxxxxxxx> wrote:
>         On Tue, 2010-01-19 at 13:44 +1300, Jelmer Vernooij wrote: 
>         > On Tue, 2010-01-19 at 11:13 +1100, ronnie sahlberg wrote:
>         > > The wireshark patch for this is fine.
>         > > 
>         > > I can apply these two patches to wireshark if you want me to.
>         > 
>         > > Is the pidl patch ok with the upstream pidl maintainer (jelmer?) ?
>         > It's mostly ok, but it should be looking at the alignment information in
>         > the level table rather than looking at IDL properties directly. 
>         Looks OK for me, I'll rework the patch to fetch information
>         there rather than within properties directly. It may however
>         require some time (few days). I'm currently trying to
>         implement other things within wireshark/pidl for string
>         support.
>         
>         FYI, similarly to the nstring/astring patch, I have added a
>         wireshark/pidl implementation for ascstr3
>         [size(16bit)][string] based on the same logic - in this case
>         we look whether we have .*LIBNDR_FLAG_STR_SIZE2.* flag or not
>         associated to the IDL element - type being obviously string.
>         
>         Finally I have found a couple more functions where I forgot to
>         propagate the di->no_align fix in packet-dcerpc.c (including
>         the dissect_nastring one).
>         
>         It tends to be a little difficult to maintain all the patches
>         properly and I'm not very good at svn diff and diff
>         editing ... Anyway will do the necessary changes and come with
>         an updated version here.
>         
>         Cheers,
>         Julien.
>         
>         
>         
>         > > On Tue, Jan 19, 2010 at 1:25 AM, Julien Kerihuel
>         > > <j.kerihuel@xxxxxxxxxxxxxx> wrote:
>         > > > Hi Lists,
>         > > >
>         > > > Prior submitting the wireshark's part of this patch onto the wireshark
>         > > > bugzilla, I thought it might be worthwhile to have feedback from
>         > > > developers first.
>         > > >
>         > > > MAPI content is non-NDR compatible. It can be dissected using the
>         > > > existing NDR layer functions in epan/dissectors/packet-dcerpc-ndr.c but
>         > > > it requires offsets to be left intact prior effective dissection, which
>         > > > means there shouldn't be any offset adjustment when LIBNDR_FLAG_NOALIGN
>         > > > flag is used in PIDL.
>         > > >
>         > > > The following patches implement such behavior:
>         > > > 1. It adds a no_align gboolean variable to dcerpc_info structure
>         > > > (default set to FALSE)
>         > > > 2. when pidl generates the code and LIBNDR_FLAG_NOALIGN flag is used, it
>         > > > sets the no_align gboolean to TRUE which turns offste adjustment off in
>         > > > wireshark.
>         > > >
>         > > > I couldn't come up with a nicer solution so far, but these tiny patches
>         > > > truly improves the overall development effort for the MAPI dissector. It
>         > > > basically prevents from writing hand-written code for most of the MAPI
>         > > > calls. This also means this may help keeping the conformance files - in
>         > > > particular request.cnf.c and response.cnf.c - readable and prevent them
>         > > > from exponentially growing up.
>         > > >
>         > > > Another advantage is that it becomes conceivable to generate code for
>         > > > structures or others some non-dceprc dissectors using pidl. You would
>         > > > only have to describe the structures, specify LIBNDR_FLAG_NOALIGN flag
>         > > > and you would have automatic dissection code generated which you can
>         > > > refer to (or cut and paste).
>         > > >
>         > > > Cheers,
>         > > > Julien.
>         > > >
>         > > > ---
>         > > >
>         > > > Julien Kerihuel
>         > > > j.kerihuel@xxxxxxxxxxxxxx
>         > > > OpenChange Project Manager
>         > > >
>         > > > GPG Fingerprint: 0B55 783D A781 6329 108A  B609 7EF6 FE11 A35F 1F79
>         > > >
>         > > >
>         > > >
>         > 
>         
>         
>         Julien Kerihuel
>         j.kerihuel@xxxxxxxxxxxxxx
>         OpenChange Project Manager
>         
>         GPG Fingerprint: 0B55 783D A781 6329 108A  B609 7EF6 FE11 A35F
>         1F79
>         
>         
>         
> 
-- 
Julien Kerihuel
j.kerihuel@xxxxxxxxxxxxxx
OpenChange Project Manager

GPG Fingerprint: 0B55 783D A781 6329 108A  B609 7EF6 FE11 A35F 1F79

Index: epan/dissectors/packet-dcerpc.c
===================================================================
--- epan/dissectors/packet-dcerpc.c	(revision 31579)
+++ epan/dissectors/packet-dcerpc.c	(working copy)
@@ -1428,7 +1428,7 @@
     buffer_len = size_is * (guint32)len;
 
     /* Adjust offset */
-    if (offset % size_is)
+    if (!di->no_align && (offset % size_is))
         offset += size_is - (offset % size_is);
 
     if (size_is == sizeof(guint16)) {
@@ -1615,7 +1615,7 @@
     buffer_len = size_is * (guint32)len;
 
     /* Adjust offset */
-    if (offset % size_is)
+    if (!di->no_align && (offset % size_is))
         offset += size_is - (offset % size_is);
 
     if (size_is == sizeof(guint16)) {
Index: epan/dissectors/packet-dcerpc.h
===================================================================
--- epan/dissectors/packet-dcerpc.h	(revision 31579)
+++ epan/dissectors/packet-dcerpc.h	(working copy)
@@ -325,6 +325,7 @@
 	guint16 smb_fid;	/* FID for DCERPC over SMB */
 	guint8 ptype;       /* packet type: PDU_REQ, PDU_RESP, ... */
 	gboolean conformant_run;
+	gboolean no_align; /* are data aligned? (default yes) */
 	gint32 conformant_eaten; /* how many bytes did the conformant run eat?*/
 	guint32 array_max_count;	/* max_count for conformant arrays */
 	guint32 array_max_count_offset;
Index: epan/dissectors/packet-dcerpc-ndr.c
===================================================================
--- epan/dissectors/packet-dcerpc-ndr.c	(revision 31579)
+++ epan/dissectors/packet-dcerpc-ndr.c	(working copy)
@@ -129,7 +129,7 @@
     }
 
 
-    if (offset % 2) {
+    if (!di->no_align && (offset % 2)) {
         offset++;
     }
     return dissect_dcerpc_uint16 (tvb, offset, pinfo,
@@ -151,7 +151,7 @@
     }
 
 
-    if (offset % 2) {
+    if (!di->no_align && (offset % 2)) {
         offset++;
     }
     offset=dissect_dcerpc_uint16 (tvb, offset, pinfo,
@@ -207,7 +207,7 @@
     }
 
 
-    if (offset % 4) {
+    if (!di->no_align && (offset % 4)) {
         offset += 4 - (offset % 4);
     }
     return dissect_dcerpc_uint32 (tvb, offset, pinfo,
@@ -277,7 +277,7 @@
     }
 
 
-    if (offset % 4) {
+    if (!di->no_align && (offset % 4)) {
         offset += 4 - (offset % 4);
     }
     offset=dissect_dcerpc_uint32 (tvb, offset, pinfo,
@@ -337,7 +337,7 @@
       return offset;
     }
 
-    if (offset % 4) {
+    if (!di->no_align && (offset % 4)) {
         offset += 4 - (offset % 4);
     }
     return dissect_dcerpc_uint64 (tvb, offset, pinfo,
@@ -360,7 +360,7 @@
       return offset;
     }
 
-    if (offset % 8) {
+    if (!di->no_align && (offset % 8)) {
         offset += 8 - (offset % 8);
     }
     return dissect_dcerpc_uint64 (tvb, offset, pinfo,
@@ -381,7 +381,7 @@
       return offset;
     }
 
-    if (offset % 8) {
+    if (!di->no_align && (offset % 8)) {
         offset += 8 - (offset % 8);
     }
     offset=dissect_dcerpc_uint64 (tvb, offset, pinfo,
@@ -437,7 +437,7 @@
       return offset;
     }
 
-    if (offset % 4) {
+    if (!di->no_align && (offset % 4)) {
         offset += 4 - (offset % 4);
     }
     return dissect_dcerpc_float(tvb, offset, pinfo,
@@ -459,7 +459,7 @@
       return offset;
     }
 
-    if (offset % 8) {
+    if (!di->no_align && (offset % 8)) {
         offset += 8 - (offset % 8);
     }
     return dissect_dcerpc_double(tvb, offset, pinfo,
@@ -481,7 +481,7 @@
     }
 
 
-    if (offset % 4) {
+    if (!di->no_align && (offset % 4)) {
         offset += 4 - (offset % 4);
     }
     return dissect_dcerpc_time_t (tvb, offset, pinfo,
@@ -502,7 +502,7 @@
     }
 
     /* uuid's are aligned to 4 bytes, due to initial uint32 in struct */
-    if (offset % 4) {
+    if (!di->no_align && (offset % 4)) {
         offset += 4 - (offset % 4);
     }
     return dissect_dcerpc_uuid_t (tvb, offset, pinfo,
@@ -532,7 +532,7 @@
       return offset;
     }
 
-    if (offset % 4) {
+    if (!di->no_align && (offset % 4)) {
         offset += 4 - (offset % 4);
     }
     ctx_hnd.attributes = dcerpc_tvb_get_ntohl (tvb, offset, drep);

Attachment: signature.asc
Description: This is a digitally signed message part