Wireshark-dev: Re: [Wireshark-dev] Possible bug in dissect_pipe_dcerpc() (packet-smb-pipe.c)

From: Pascal Quantin <pascal.quantin@xxxxxxxxx>
Date: Sat, 28 Jul 2012 00:32:19 +0200
2012/7/28 Evan Huus <eapache@xxxxxxxxx>
On Fri, Jul 27, 2012 at 6:14 PM, Pascal Quantin
<pascal.quantin@xxxxxxxxx> wrote:
> Hi all,
>
> while working on fixing a few Clang warnings in packet-smb-pipe.c, I
> discovered that the hash_key variable in dissect_pipe_dcerpc() function is
> never used.
> According to the nice comment Guy put in revision 7777 above the variable
> assignment, I have the feeling that it is actually useful in case of
> reassembly in both directions and that the following patch should be
> applied:
>
> Index: packet-smb-pipe.c
> ===================================================================
> --- packet-smb-pipe.c   (révision 44082)
> +++ packet-smb-pipe.c   (copie de travail)
> @@ -3348,7 +3348,7 @@
>                  * in this direction, by searching for its reassembly
>                  * structure.
>                  */
> -               fd_head=fragment_get(pinfo, fid, dcerpc_fragment_table);
> +               fd_head=fragment_get(pinfo, hash_key,
> dcerpc_fragment_table);
>                 if(!fd_head){
>                         /* No reassembly, so this is a new pdu. check if the
>                            dissector wants us to reassemble it or if we
> @@ -3370,11 +3370,11 @@
>                            more data ?
>                         */
>                         if(pinfo->desegment_len){
> -                               fragment_add_check(d_tvb, 0, pinfo, fid,
> +                               fragment_add_check(d_tvb, 0, pinfo,
> hash_key,
>                                         dcerpc_fragment_table,
>                                         dcerpc_reassembled_table,
>                                         0, reported_len, TRUE);
> -                               fragment_set_tot_len(pinfo, fid,
> +                               fragment_set_tot_len(pinfo, hash_key,
>                                         dcerpc_fragment_table,
>                                         pinfo->desegment_len+reported_len);
>                         }
> @@ -3392,7 +3392,7 @@
>                 while(fd_head->next){
>                         fd_head=fd_head->next;
>                 }
> -               fd_head=fragment_add_check(d_tvb, 0, pinfo, fid,
> +               fd_head=fragment_add_check(d_tvb, 0, pinfo, hash_key,
>                         dcerpc_fragment_table, dcerpc_reassembled_table,
>                         fd_head->offset+fd_head->len,
>                         reported_len, TRUE);
> @@ -3426,7 +3426,7 @@
>          * up so that we don't have to distinguish between the first
>          * pass and subsequent passes?
>          */
> -       fd_head=fragment_add_check(d_tvb, 0, pinfo, fid,
> dcerpc_fragment_table,
> +       fd_head=fragment_add_check(d_tvb, 0, pinfo, hash_key,
> dcerpc_fragment_table,
>             dcerpc_reassembled_table, 0, 0, TRUE);
>         if(!fd_head){
>                 /* we didnt find it, try any of the heuristic dissectors
>
> Am I right or should we remove the hash_key variable?
>
> Regards,
> Pascal.

I'm not sure what the correct course of action is, but there's already
a bug for it. I found the same issue with CppCheck a few weeks ago :)

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7451
 
Hi Evan,

thanks for the reminder: I missed this bug ;)
I guess Guy would be the best able to answer this question (if he remembers the code he wrote in 2003!).

Regards,
Pascal.