On Thu, Jul 24, 2014 at 4:35 PM, Kevin Cox <[email protected]> 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 :)
___________________________________________________________________________
Sent via: Wireshark-dev mailing list <[email protected]>
Archives: http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:[email protected]?subject=unsubscribe
