Evan Huus
changed
bug 8239
Comment # 10
on bug 8239
from Evan Huus
Thanks for the patch and sample captures. It already looks pretty solid, I just
have a few questions about the dissector.
- Why do you have static variables in dissect_sstp? I have nothing against
static variables in general, but sstp_numattrib and tvb_next don't need to be
static at all that I can see, and ppp_handle should probably be global
(find_dissector calls are typically put in the proto_reg_handoff_ function so
that they're only run once instead of once per packet).
- Is the hashproto assignment on line 316 necessary? I don't see it used
anywhere after that.
- When using hf_sstp_data_unknown you don't have to call tvb_length_remaining()
again for the length - you can just pass -1 and it will use the rest of the
buffer automatically. Tangentially, you probably meant to use
tvb_reported_length_remaining on line 328 so that it still works with truncated
packet captures?
- I believe there is a possibility of underflow in the attrib_length variable
if the packet is corrupt and has a value in that field that is too small?
- There is a crash bug on lines 239 and 253 where you directly access a
value_string array with data from the packet. If the packet is corrupt and
contains an index higher than the largest entry in the value_string then the
lookup will use invalid memory. The val_to_str() utility function should be
used instead - it takes the index, the value string, and a printf-style format
string to use in case the requested index is not valid in the value_string.
Cheers,
Evan
You are receiving this mail because:
- You are watching all bug changes.