Re: [Wireshark-dev] Should we check value_strings for NULL termination while registering?
ronnie sahlberg wrote:
That would be an awful lot of work since we would need to modify (add
registration of) all value_strings.
I only thought of the registration of the hf_ arrays, so this would
probably a single place to change.
I think this would already catch the vast majority of problems.
Barring we could convince the coverity guys to add a check make sure
all arrays of the type value_string are terminated with {0,NULL}
but that wouldnt help out of tree dissectors nor can we rely on
coverity being available to us forever.
Not an option in my eyes. I'm thinking about helping (newbie) developers
not needing to hunt down bugs hard to find.
If coverity comes in place, this is much too late for this purpose.
BTW: I couldn't find Ethereal/Wireshark on their project list recently?!?
If we are making these changes we might also at the same time try to
address the linear search property for the existing struct.
While most value_strings are small there are some very very large
ones that are used very frequently such as the SMB NT status code
array.
Binary trees have both good and bad properties. They would be good for
managing these few very large value_strings efficiantly but since
most of the value_strings are very small there would be a massive
memory overhead for these small value_strings where a linear search
would be quite sufficient.
(we could change the accessor/search function for value strings to
still keep it as a linear search array but everytime something has
been looked up, we swap some pointers and let the found entry
bubble upwards in the array to get it to automatically make sure at
runtime that the frequently looked up entries are always near the
start of the array and thus quick to find with a linear search)
I'm not sure on this, but I suppose the places where we have huge
value_strings are relatively rare.
Using a completely different (new) mechanism might be the way to go.
Well an easy way to make sure all of them are properly terminated
with a {0,NULL} is by using variadic macros.
Unfortunately not standard until c99 but been part of GCC forever.
#define VALUE_STRING(name, ...) \
value_string_t name[] = { \
__VA_ARGS__ \
{0, NULL} \
};
VALUE_STRING(vs,
{1, foo},
{2, bar},
)
Does VC6 support variadic macros?
Sounds like a good approach.
I don't know if this is possible on VC6 (and other compilers we support)
and my current VC6 setup is out of business (I'm currently trying to
compile using VC 2005).
Maybe someone else can have a try?
Regards, ULFL
___
Wireshark-dev mailing list
Wireshark-dev@wireshark.org
http://www.wireshark.org/mailman/listinfo/wireshark-dev
Re: [Wireshark-dev] Should we check value_strings for NULL termination while registering?
Gilbert Ramirez wrote: I believe we do this in the build-bot testing, by doing: tshark -G values Since that operation iterates across all the value_string arrays, a non-terminated array will result in an error or at least it should. Is that enough testing? Unsure. It can drive you mad if you develop a dissector and crash for that reason as there's no hint into that direction. It can take you hours of development time to find the cause. So it's basically a tradeoff between ease of development (crash if the value_string isn't zero terminated) and program startup time (as this check obviously will require some time to be done). The build-bot tests are (hopefully) done long after the dissector is developed and seems to work perfectly. So yes, it helps to make sure no security related bugs are introduced but no, it won't help much during development time of an (unexperienced) developer. Regards, ULFL ___ Wireshark-dev mailing list Wireshark-dev@wireshark.org http://www.wireshark.org/mailman/listinfo/wireshark-dev
Re: [Wireshark-dev] Should we check value_strings for NULL termination while registering?
I believe we do this in the build-bot testing, by doing: tshark -G values Since that operation iterates across all the value_string arrays, a non-terminated array will result in an error or at least it should. Is that enough testing? --gilbert On 9/20/06, Ulf Lamping [EMAIL PROTECTED] wrote: Hi List! It seems to be a common mistake to forget the terminating zero entry in a value_string, I've done this myself before and it's hard to track it down if you don't have a clue what's going wrong. Even worse, this mistake might not make any problems for a long time as usually the values rushing in will be in the value_string (I've found my mistake by fuzz-testing). We could automatically check the termination of the value_string (at least for the registered values) while registering the hf_ values. This will indicate the problem a lot earlier and at the place where it's caused (and not much, much later). Of cause this has a drawback: Iterating trough all value_strings at registration will take a little bit of time at startup (didn't tried so don't know how long this will take). What do others think: Is the added time overhead worth adding some extra robustness or are there other mechanisms possible to detect these kind of errors (e.g. static code analysis)? Regards, ULFL _ Der WEB.DE SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen! http://smartsurfer.web.de/?mc=100071distributionid=0066 ___ Wireshark-dev mailing list Wireshark-dev@wireshark.org http://www.wireshark.org/mailman/listinfo/wireshark-dev ___ Wireshark-dev mailing list Wireshark-dev@wireshark.org http://www.wireshark.org/mailman/listinfo/wireshark-dev
