Wireshark · Ethereal-dev: Re: [Ethereal-dev] New postgresql dissector

[Edwin Calo <calo@xxxxxxxxxxxx>]

I made changes on reading the string and took out the excess
buff_remaining call.

I saw what you refer on epan/tvbuff.h in terms of 'tvb_get_ptr'.

I change it using 'tvb_memcpy' and also fixed the readonly warning. 

This is how I'm processing the string: 
(counter is the size of the array)

/* Reading the string from the packet */
string = g_malloc( counter+1 );
tvb_memcpy(tvb,string,offset,counter);
string[counter]='\0';   /* Forcing end of string */
/* Printing the data */
proto_tree_add_string(tree,hf_postgresql_string,tvb,offset,counter,string);
if (check_col (pinfo->cinfo, COL_INFO)) { col_append_fstr (pinfo->cinfo,
COL_INFO, " %s", string ); }
g_free(string);  /* Freeing up string */
string='\0';

I'm attaching the new packet-postgresql.c with the changes.
Thanks for the feedback.

On Sun, 2004-02-15 at 01:36, Joerg Mayer wrote:
> On Sat, Feb 14, 2004 at 02:23:46PM -0600, Edwin Calo wrote:
> > Thanks for the feedback and I made some changes that I think 
> > will guarantee the loop will always terminate.
> 
> Just let me rephrase that: When I looked at your code the first time,
> I couldn't immediately decide whether the loop terminates or not, and
> I don't think that your code has changed anything regarding that, so
> I will work with your original patch, not the second one.
> 
> > (Before I was just looking at the buff_remaining...)
> 
> When I first looked at the code, I saw that offset was sometimes being
> increased, sometimes being decreased. Now that I have taken another
> look at it, I can see that the decrease is always undone. Also, offset
> is increased at the very beginning of the loop and buff_remaining is
> calculated at the end of loop, so it should terminate.
> 
> 
> ...
> > #include <epan/strutil.h>
> > #include "packet-rpc.h"
> > #include "plugins/plugin_api.h"
> 
> These aren't needed.
> ...
> 
> >   buff_remaining = tvb_length_remaining (tvb, offset);
> ...
> >     buff_remaining = tvb_length_remaining (tvb, offset);
> >     /* Used to print the initial buff remaining */
> >     /* if (check_col (pinfo->cinfo, COL_INFO)) { col_append_fstr (pinfo->cinfo, COL_INFO, " BuffRemainig: %d", buff_remaining ); } */
> 
> Looks like there is one call to buff_remaining too much
> 
> >                   proto_tree_add_string (tree,hf_postgresql_string,tvb, offset,counter, tvb_get_ptr(tvb, offset, counter));
> >                   string = tvb_get_ptr (tvb, offset, counter);
> >		   /* Forcing end to string */
> >                   string[counter]='\0'; 
> 
> I'm not really sure that this does what you are trying to accomplish:
> a) According to doc/README.tvbuff, tvb_get_ptr might create a copy of the
>    data, so running tvb_get_ptr twice wouldn't acomplish anything. In
>    case no copy is made, things are even worse, because you are changing
>    the original data. Also, it seems that you are writing one byte
>    *past* the end of the buffer.
> b) gcc complains about "assignment of read-only loaction"
> 
> An example of how this can be done can be found in packet-http.c,
> ft the end of unction is_http_request_or_reply (request_method).
> Hmm, maybe just using tvb_memdup should be ok (and fixing that off
> by 1 problem).
> 
>  Ciao
>     Jörg
> 
> PS: epan/tvbuff.h contains an even stricter warning on the use of
>     tvb_get_ptr.

/* packet-postgresql.c
 * Routines for postgresql packet disassembly
 *
 * Copyright 2004, Edwin Calo <calo@xxxxxxxxxxxx>
 *
 * $Id: packet-loadl31-master.c,v 1.4 2004/01/20 03:41:56 jafour1 Exp $
 *
 * Ethereal - Network traffic analyzer
 * By Gerald Combs <gerald@xxxxxxxxxxxx>
 * Copyright 1998 Gerald Combs
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
 */

#ifdef HAVE_CONFIG_H
#include "config.h"
#endif

#include <glib.h>
#include <epan/packet.h>

static int proto_postgresql = -1;
static int hf_postgresql_response = -1;
static int hf_postgresql_request = -1;
static int hf_postgresql_length = -1;
static int hf_postgresql_string_size = -1;
static int hf_postgresql_string = -1;
static int hf_postgresql_total_length = -1;
static int hf_postgresql_bitone = -1;
static int hf_postgresql_buff_remaining = -1;
static int hf_postgresql_opcode = -1;
static int hf_postgresql_idone = -1;
static gint ett_postgresql = -1;

#define TCP_PORT_POSTGRESQL	5432


static void
dissect_postgresql (tvbuff_t * tvb, packet_info * pinfo, proto_tree * tree)
{
  proto_tree *postgresql_tree;
  proto_item *ti;
  gint offset = 0;
  gint buff_remaining = 0;

  guint8 *string;
  guint8 bitone;
  gint flag = 0;
  gint counter = 0;




  if (check_col (pinfo->cinfo, COL_PROTOCOL))
    col_set_str (pinfo->cinfo, COL_PROTOCOL, "POSTGRESQL");

  ti = proto_tree_add_item (tree, proto_postgresql, tvb, offset, -1, FALSE);
  postgresql_tree = proto_item_add_subtree (ti, ett_postgresql);

  buff_remaining = tvb_length_remaining (tvb, offset);

  if (check_col (pinfo->cinfo, COL_INFO))
    {
      col_add_str (pinfo->cinfo, COL_INFO,
		   (pinfo->match_port ==
		    pinfo->destport) ? " Request" : " Response");
    }

    counter=0;
    flag=0;
    while ( buff_remaining > 1 )
    {
         bitone = tvb_get_ntohs (tvb, offset);
         offset += 1;

         if(bitone > 0x7f || (bitone > 0x0 && bitone < 0x20) ) 
         {
	    if(counter > 3)
	    {
	      if(offset > counter)
	      {
                   offset -= counter;

                    /* Reading the string from the packet */
		    string = g_malloc( counter+1 );
		    tvb_memcpy(tvb,string,offset,counter);
                    string[counter]='\0';   /* Forcing end of string */
                    /* Printing the data */
                    proto_tree_add_string (tree,hf_postgresql_string,tvb, offset,counter, string );
                    if (check_col (pinfo->cinfo, COL_INFO)) { col_append_fstr (pinfo->cinfo, COL_INFO, " %s", string ); }
		    g_free(string);  /* Freeing up string */
                    string='\0';

                   offset += counter;
	           counter=0;
	      }
	      else
	      {
	       counter=0;
	       offset+=1;
	      }
	    }
	    else
	    {
	     counter=0;
	     offset+=1;
	    }
         }

         if( bitone == 0 )
         {
                if(counter != 0)
                { 
                  if(offset > counter)
                  {
                   offset -= counter;
		   if( counter > 1)
		   {
                    /* Reading the string from the packet */
		    string = g_malloc( counter+1 );
		    tvb_memcpy(tvb,string,offset,counter);
                    string[counter]='\0';   /* Forcing end of string */
                    /* Printing the data */
                    proto_tree_add_string (tree,hf_postgresql_string,tvb, offset,counter, string );
                    if (check_col (pinfo->cinfo, COL_INFO)) { col_append_fstr (pinfo->cinfo, COL_INFO, " %s", string ); }
		    g_free(string);  /* Freeing up string */
                    string='\0';

                   }
		   offset += counter;
                  }
                  counter = 0;
                }
                counter=0;
         }
         else
         {
             counter += 1;
         }

         buff_remaining = tvb_length_remaining (tvb, offset);
     }
} 


void proto_register_postgresql (void)
{

  static hf_register_info hf[] = {
    {&hf_postgresql_response,
     {"Response", "postgresql.response",
      FT_BOOLEAN, BASE_NONE, NULL, 0x0,
      "TRUE if postgresql response", HFILL}},
    {&hf_postgresql_request,
     {"Request", "postgresql.request",
      FT_BOOLEAN, BASE_NONE, NULL, 0x0,
      "TRUE if postgresql request", HFILL}},
    {&hf_postgresql_string, {"String", "hf_postgresql_string", FT_STRING, BASE_NONE, NULL, 0x0, "", HFILL}},
    {&hf_postgresql_length, {"Length", "hf_postgresql_length", FT_UINT32, BASE_DEC, NULL, 0x0, "", HFILL}},
    {&hf_postgresql_string_size, {"Size", "hf_postgresql_string_size", FT_UINT32, BASE_DEC, NULL, 0x0, "", HFILL}},
    {&hf_postgresql_total_length, {"TotalLength", "hf_postgresql_total_length", FT_UINT16, BASE_DEC, NULL, 0x0, "", HFILL}},
    {&hf_postgresql_buff_remaining, {"Buffer Remaining", "hf_postgresql_buff_remaining", FT_UINT8, BASE_DEC, NULL, 0x0, "", HFILL}},
    {&hf_postgresql_opcode, {"Op Code", "hf_postgresql_opcode", FT_UINT32, BASE_HEX, NULL, 0x0, "", HFILL}},
    {&hf_postgresql_bitone, {"Bitone", "hf_postgresql_bitone", FT_UINT8, BASE_HEX, NULL, 0x0, "", HFILL}},
    {&hf_postgresql_idone, {"idone", "hf_postgresql_idone", FT_UINT8, BASE_HEX, NULL, 0x0, "", HFILL}},
 


  };

  static gint *ett[] = {
    &ett_postgresql,
  };

  proto_postgresql =
    proto_register_protocol ("POSTGRESQL", "POSTGRESQL", "postgresql");
  proto_register_field_array (proto_postgresql, hf, array_length (hf));
  proto_register_subtree_array (ett, array_length (ett));
}

void
proto_reg_handoff_postgresql (void)
{
  dissector_handle_t postgresql_handle;

  postgresql_handle =
    create_dissector_handle (dissect_postgresql, proto_postgresql);
  dissector_add ("tcp.port", TCP_PORT_POSTGRESQL, postgresql_handle);
}