Wireshark · Wireshark-dev: Re: [Wireshark-dev] const'ness of value_string_ext

[Evan Huus <eapache@xxxxxxxxx>]

On Thu, Jul 24, 2014 at 4:35 PM, Kevin Cox <kevincox@xxxxxxxxxxx> wrote:

Hello All,

While working on the new Ceph dissector I made a mistake using
value_string_ext (herein evs) where I declared them 'const' which was
causing an error when they were put in a read-only segment of the
executable.

Thankfully Bill Meier was able to figure out why it was crashing on his
system but it made me curious why this error was possible without
compiler errors or warnings.

After looking into epan/value_string.{h,c} I noticed that all of the evs
methods took 'const value_string_ext*' even though many of them may end
up writing to the evs.  This all boils down to the "virtual" method
'_vs_match2' which may contain '_try_val_to_str_ext_init' which casts
away the const'ness and writes to the evs.

There is a comment inside this function explaining why the const is
casted away however I believe that this isn't the proper solution.  The
comment discusses casting inside the function versus casting the
function pointer when assigning it to '_vs_match2'.  I would argue that
the correct solution is actually to change the prototype of '_vs_match2'
from:

const value_string *(*)(const guint32, const value_string_ext*)
to
const value_string *(*)(const guint32, value_string_ext*)

then change all of the evs functions to take a non-const evs.  This
would convey the actual signature of the functions and prevent this type
of error in the future.

However, the problem with this approach is that when declaring hf's and
probably other places around the source 'const void*'s are used to
handle evs.  So changing these functions would cause large changes to
the code.  There a a number of solutions of varying completeness.

0: Leave as is.
Pros:
- Easy.
Cons:
- The API lies and can invoke undefined behaviour unless you know.
- No compiler checking.

1: Change all of the evs functions and add the const-cast further up the
stack.
Pros:
- The evs functions don't lie and people will get errors if they use an
evs directly.
- The evs API is now correct.
- Can slowly start to move the rest of the code to be const-correct.
Cons:
- There is still an issue in a different part of the code.
- Not full type safety.

2: Change everything.
Pros:
- Full compiler error checking and type safety.
Cons:
- Hard
- May change dissector API.

I was wondering what everyone else thought and what should be done to
improve the safety of this code.

Cheers,
Kevin

We are usually not afraid of breaking API changes (whether that's a good thing or not is debatable, but it's true) so probably solution 2 if somebody wants to tackle it :)