https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=16335
--- Comment #62 from Martin Mathieson <martin.r.mathie...@googlemail.com> ---
(In reply to Jaap Keuter from comment #60)
> (In reply to Balling from comment #59)
> > /ui/text_import.c#L1049
> > 'info->offset_type == OFFSET_DEC' is always true because offset_type is
> > enum.
> So, defensive programming is discouraged?
I am with Jaap on this one. The code is clearer because OFFSET_DEC is shown
there. I don't want to not see the extra enum type, or have to look up what
fills in info to make sure it will only be a valid member of the enum.
>
> > epan/dissectors/packet-infiniband.c#L2782
> > Copy paste *offset = local_offset; can be removed.
> I assume this should instead update local_offset.
>
I assume that the real problem here is that local_offset should have been
advanced when the fields above were dissected, before re-ssigning to *offset.
But I'd need to look at the spec to know for sure if there was a valid reason
why the offset wasn't advanced.
> > epan/dissectors/packet-ip.c#L1556
> > enum
> I don't understand the suggestion.
>
The test is redundant, because above we know opt is EOOL or NOP. It would be
fine to just make it an else with a comment to note that this is the NOP
path...
> > epan/dissectors/packet-ipdc.c#L739
> > This is impossible because get_ipdc_pdu_len has return raw_len + 4; but
> > raw_len
> > is unsigned, thus it cannot be < 4.
> Again, defensive programming is discouraged?
I am less sure about this one. get_ipdc_pdu_len() would have thrown if 3rd and
4th bytes were available, so really couldn't get here. Maybe a comment to say
that the payload is definitely at least 4 bytes instead?
Wading through these long logs of issues with lots of false positives takes a
lot of time to do properly. Apart from the leaks (which luckily were localised
to one function), I've mostly got rid of redundant assignments and tests (and
even there its sometimes a judgement call as to whether the code reads better
with or without them).
If no-one else does, I'll do the packet-ip.c one later, and look for a reviewer
for the packet-infiniband.c one (in a few hours at the earliest).
--
You are receiving this mail because:
You are watching all bug changes.
___________________________________________________________________________
Sent via: Wireshark-bugs mailing list <wireshark-bugs@wireshark.org>
Archives: https://www.wireshark.org/lists/wireshark-bugs
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-bugs
mailto:wireshark-bugs-requ...@wireshark.org?subject=unsubscribe
