Wireshark · Ethereal-dev: [Ethereal-dev] Re: rev 15660: /trunk/plugins/asn1/: packet-asn1.c /trunk/epan/: prefs.c range.c /tru

[ronnie sahlberg <ronniesahlberg@xxxxxxxxx>]

On 9/1/05, Guy Harris <gharris@xxxxxxxxx> wrote:
> Ulf Lamping wrote:
> > sahlberg@xxxxxxxxxxxx wrote:
> > g_snprintf(str, 128-(str-string), "%s%u", prepend_comma?",":"",
> >
> > The expression "128-(str-string)" is exactly which should be avoided!!!
> >
> > This way of coding is *very* error intensive, at least when changes are
> > made to the sources later.
> 
> Yes, it's a somewhat awkward idiom, possibly leaving more room for errors.

Yes you are right. It is suboptimal.


> 
> It also means the dissector imposes a limit on the length of the string.
> 
> > Maybe copying the GString code from GLib and replace the g_malloc calls
> > by ep_malloc (well and change the prefix g_string_ to something like
> > ep_string_ or course)?
> 
> Or, alternatively, implementing an emem-based asprintf-style API, perhaps
> using g_printf_string_upper_bound()?
> 
> Another possibility is to use proto_item_append_text() - put the initial
> information into an item created with a proto_tree_add_ routine, and then
> append to it.
> 

Good point.

There are only very very few dissectors thus far that do this kind of
string manipulation.
Most dissectors dont do any string manipulation at all.



I think that in a future audit phase one should go through all string
processing and convert as many as possible into
standard value_strings (some dissectors still use switch-statements to
pick out static strings) and append the strings to col_info and the
proto_item one component at a time instead of first building these
temporary strings.


My aim was to first get rid of all sprintf()   which was done a week
ago, and then all GString and g_string_...
and standardize on g_snprintf() for this phase.


I also audited that all value_strings were terminated properly.


Next phase i would like to eliminate all strcat and strcpy.



In a future phase i would like to eliminate all character arrays that
are either global variables or stack variables to become either sp or
ep emem allocated memory.
In particular i would like to move every one of them off the stack 
since this would reduce the danger of buffer overflows.
eliminating all use of arrays that live on the stack would virtually
eliminate the danger of buffer overflows as an attack vector.
eliminating all global/static arrays would reduce the runtime
requirement of memory for ethereal slightly.


After that i think it would be nice to go over all loops and verify
that offset is always increased in each iteration or that the loop
terminates with an exception.


After that i think we should be in pretty decent shape,  at least
better shape than we were in in 0.10.11