Comment # 8
on bug 9579
from Evan Huus
(In reply to comment #7)
> The above was a simplification of the issue, but (skipping disabled code)
> there are still parts I don't fully understand (first_pass, branch_taken).
I don't necessarily understand it fully either but I think:
- branch_taken is the index of the choice used to satisfy the decoding
- the loop is supposed to go through the array once looking for an exact match,
and if it can't find one it goes through the array again looking for a more
general match
> Replacing the goto by a continue has as side-effect that *branch_taken gets
> incremented and possibly reset to -1 if !ch->func.
Based on what I think the code is trying to do, I believe that this is OK. I
think there is already a general bug in that choice_index is off-by-one on the
second pass (it gets set to -1 instead of 0 for the first element of the second
pass, and is subsequently always behind).
> This BER thing seems very complex, can't branch_taken just be a boolean?
> (looking at epan/dissectors/packet-mms.c:7032 for example).
No, take a look at packet-cmp.c:1101
> Replacing goto by continue (and then the choice_try_again label can be
> removed too) fixes the overflow issue and assuming that branch_taken is a
> boolean (-1 false, others true), then I think it is safe to make the change.
>
> I have not tested it since it will "obviously" fix the ASAN bug, but will
> possibly modify the output and I cannot tell whether the new output will be
> correct or not.
>
> Remember to compile with -DCMAKE_C_FLAGS='-fsanitize=address'
> -DCMAKE_CXX_FLAGS='-fsanitize=address' (or similar CFLAGS, CXXFLAGS and
> possibly LDFLAGS for autotools) to enable ASAN.
I need to rework my build env at some point to support multiple out-of-tree
builds and cmake and ASAN and... it's on my todo list :)
Potential fix(es) at: https://code.wireshark.org/review/1530
You are receiving this mail because:
- You are watching all bug changes.
