Wireshark-bugs: [Wireshark-bugs] [Bug 9579] Clang ASAN : global-buffer-overflow SNMP : dissect_b

Date: Tue, 06 May 2014 14:21:00 +0000

Comment # 8 on bug 9579 from
(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.