Re: Possible bug in dissect_pipe_dcerpc() (packet-smb-pipe.c)

[Pascal Quantin <pascal.quantin () gmail com>]

2012/7/28 Evan Huus <eapache () gmail com>

> On Fri, Jul 27, 2012 at 6:14 PM, Pascal Quantin
> <pascal.quantin () gmail com> 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.

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe