[prev in list] [next in list] [prev in thread] [next in thread]
List: wireshark-dev
Subject: Re: [Wireshark-dev] range_string checking
From: Martin Mathieson via Wireshark-dev <wireshark-dev () wireshark ! org>
Date: 2020-04-06 9:10:26
Message-ID: CAC803ueS2O=OOYCihNJq_r+7v7SywzvEEwO+tjmyrBgvPTuQ_Q () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
After committing the main change for this (minus the ISUP part), I did play
around some more with this check. This included:
- checking for non-inclusive overlap (i.e. some overlapping, some not)
- check to see if collectively all of the earlier entries hide a later one
I didn't find any definite bugs, but was surprised to see the use of -ve
values being used (value_min and value_max are guint32) with an FT_INT16
field.
e.g.
{ &hf_mq_tsh_ccsid , {"CCSID.....", "mq.tsh.ccsid", FT_INT16,
BASE_DEC | BASE_RANGE_STRING, RVALS(GET_VALRV(ccsid)), 0x0, "TSH CCSID",
HFILL }},
So the field is being dissected as a 16-bit signed value.
The value_string array itself is defined (using nested macros) as:
DEF_VALRB(ccsid)
/* -4*/ DEF_VALR1(MQCCSI_AS_PUBLISHED),
/* -3*/ DEF_VALR1(MQCCSI_APPL),
/* -2*/ DEF_VALR1(MQCCSI_INHERIT),
/* -1*/ DEF_VALR1(MQCCSI_EMBEDDED),
/* 0*/ DEF_VALR3(MQCCSI_UNDEFINED, MQCCSI_UNDEFINED,
"UNDEFINED/DEFAULT/Q_MGR"),
/* >=1*/ DEF_VALR3(MQCCSI_1, MQCCSI_65535, ">=1"),
DEF_VALRE;
If you cast (gint16)(-4) to a guint32 and back again, it at least gave me
the same answer, so I believe it'll work for single values. What wouldn't
work is if you had e.g.
( -2, 2, "Something" }
As the min field will be cast to something much bigger than 2, so the (min
<= val <= max) test could never pass. However, this isn't happening, or
my existing check for max being <= min would have flagged it.
Martin
On Sat, Apr 4, 2020 at 10:26 PM Martin Mathieson <
martin.r.mathieson@googlemail.com> wrote:
> In the previous message I said " I suspect having a more complicated test
> probably find many more issues." I meant to say that I doubted it would.
>
> Have uploaded https://code.wireshark.org/review/#/c/36708/ for the
> remaining issues and the checking code itself. Only spec I couldn't find
> for was ISUP - if someone who does have the spec could look it up that'd be
> great.
> Thanks,
> Martin
>
> On Sat, Apr 4, 2020 at 9:24 PM Martin Mathieson <
> martin.r.mathieson@googlemail.com> wrote:
>
>> Yes, I'm sure I've seen an example where there is a catch-all at the end
>> of each of a number of ranges, then a catch-all covering all ranges after
>> that.
>>
>> I am still only complaining if a later item is completely hidden by a
>> single, earlier one. If I understand what you said, I suppose I could
>> check whether collectively all of the earlier items hide a later one. I
>> suspect having a more complicated test probably find many more issues.
>>
>> My plan is to fix the remaining handful of issues and check it in as
>> it is, then I'll experiment with seeing if I can find any others.
>>
>> The only other automated check along these lines I tried recently was for
>> enumerated preferences (i.e. looking for duplicated IDs or strings), but it
>> sadly(?) didn't find anything...
>>
>> Martin
>>
>> On Sat, Apr 4, 2020 at 2:53 PM Jaap Keuter <jaap.keuter@xs4all.nl> wrote:
>>
>>>
>>>
>>> On 2 Apr 2020, at 23:08, Martin Mathieson via Wireshark-dev <
>>> wireshark-dev@wireshark.org> wrote:
>>>
>>> It is common to have a 'catch-all' case for parts or all of the range,
>>> which is Ok if it comes after more specific entries. I'm wondering if its
>>> worth complaining if *part* of an entry is hidden by an earlier one?
>>> Current output from master is as below. I will try to fix them up where I
>>> can access the relevant specs, but wanted to check my understanding of how
>>> they work and how fussy we should be? I will most likely update
>>> README.dissector to make sure it is clear how it is evaluated in order.
>>>
>>>
>>> Cool stuff.
>>> I can definitely see use for catch-all-in-certain-range, opposite of
>>> filling every gap with their specifics, which is maintenance heavy. This
>>> matches the val_to_string() default string used when no match is found, but
>>> then in a higher dimension. I would say let the ranges decide, if their
>>> union is the same as the catch-all then it's okay, otherwise mark it.
>>>
>>> just my €0.02
>>> Jaap
>>>
>>>
>>> ___________________________________________________________________________
>>> Sent via: Wireshark-dev mailing list <wireshark-dev@wireshark.org>
>>> Archives: https://www.wireshark.org/lists/wireshark-dev
>>> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>>> mailto:wireshark-dev-request@wireshark.org
>>> ?subject=unsubscribe
>>
>>
[Attachment #5 (text/html)]
<div dir="ltr">After committing the main change for this (minus the ISUP part), I \
did play around some more with this check. This included:<div>- checking for \
non-inclusive overlap (i.e. some overlapping, some not)</div><div>- check to see if \
collectively all of the earlier entries hide a later one</div><div>I didn't find \
any definite bugs, but was surprised to see the use of -ve values being used \
(value_min and value_max are guint32) with an FT_INT16 \
field.</div><div><br></div><div>e.g.</div><div> { &hf_mq_tsh_ccsid \
, {"CCSID.....", "mq.tsh.ccsid", FT_INT16, BASE_DEC | \
BASE_RANGE_STRING, RVALS(GET_VALRV(ccsid)), 0x0, "TSH CCSID", HFILL \
}},<br></div><div><br></div><div>So the field is being dissected as a 16-bit signed \
value.</div><div><br></div><div>The value_string array itself is defined (using \
nested macros) as:</div><div><br></div><div>DEF_VALRB(ccsid)<br>/* -4*/ \
DEF_VALR1(MQCCSI_AS_PUBLISHED),<br>/* -3*/ DEF_VALR1(MQCCSI_APPL),<br>/* -2*/ \
DEF_VALR1(MQCCSI_INHERIT),<br>/* -1*/ DEF_VALR1(MQCCSI_EMBEDDED),<br>/* 0*/ \
DEF_VALR3(MQCCSI_UNDEFINED, MQCCSI_UNDEFINED, \
"UNDEFINED/DEFAULT/Q_MGR"),<br>/* >=1*/ DEF_VALR3(MQCCSI_1, \
MQCCSI_65535, ">=1"),<br>DEF_VALRE;<br></div><div><br></div><div>If you \
cast (gint16)(-4) to a guint32 and back again, it at least gave me the same answer, \
so I believe it'll work for single values. What wouldn't work is if you had \
e.g.</div><div>( -2, 2, "Something" }</div><div>As the min field will be \
cast to something much bigger than 2, so the (min <= val <= max) test could \
never pass. However, this isn't happening, or my existing check for max being \
<= min would have flagged \
it.</div><div><br></div><div>Martin</div><div><br></div><div><br></div><div><br></div> \
<div><br></div><div><br></div><div><br></div><div><br></div><div><br></div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Apr 4, 2020 at 10:26 PM \
Martin Mathieson <<a \
href="mailto:martin.r.mathieson@googlemail.com">martin.r.mathieson@googlemail.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">In the \
previous message I said "
I suspect having a more complicated test probably find many more issues." I \
meant to say that I doubted it would.<div><br></div><div>Have uploaded <a \
href="https://code.wireshark.org/review/#/c/36708/" \
target="_blank">https://code.wireshark.org/review/#/c/36708/</a> for the remaining \
issues and the checking code itself. Only spec I couldn't find for was ISUP - \
if someone who does have the spec could look it up that'd be \
great.</div><div>Thanks,</div><div>Martin</div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Apr 4, 2020 at 9:24 PM \
Martin Mathieson <<a href="mailto:martin.r.mathieson@googlemail.com" \
target="_blank">martin.r.mathieson@googlemail.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr">Yes, I'm sure I've seen an \
example where there is a catch-all at the end of each of a number of ranges, then a \
catch-all covering all ranges after that.<div><br></div><div>I am still only \
complaining if a later item is completely hidden by a single, earlier one. If I \
understand what you said, I suppose I could check whether collectively all of the \
earlier items hide a later one. I suspect having a more complicated test probably \
find many more issues.</div><div><br></div><div>My plan is to fix the remaining \
handful of issues and check it in as it is, then I'll experiment with seeing if \
I can find any others.</div><div><br></div><div>The only other automated check along \
these lines I tried recently was for enumerated preferences (i.e. looking for \
duplicated IDs or strings), but it sadly(?) didn't find \
anything...</div><div><br></div><div>Martin</div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Apr 4, 2020 at 2:53 PM \
Jaap Keuter <<a href="mailto:jaap.keuter@xs4all.nl" \
target="_blank">jaap.keuter@xs4all.nl</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div><br><div><br><blockquote type="cite"><div>On \
2 Apr 2020, at 23:08, Martin Mathieson via Wireshark-dev <<a \
href="mailto:wireshark-dev@wireshark.org" \
target="_blank">wireshark-dev@wireshark.org</a>> wrote:</div><br><div><span \
style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal \
;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transf \
orm:none;white-space:normal;word-spacing:0px;text-decoration:none;float:none;display:inline">It \
is common to have a 'catch-all' case for parts or all of the range, which is \
Ok if it comes after more specific entries. I'm wondering if its worth \
complaining if *part* of an entry is hidden by an earlier one? Current output from \
master is as below. I will try to fix them up where I can access the relevant \
specs, but wanted to check my understanding of how they work and how fussy we should \
be? I will most likely update README.dissector to make sure it is clear how it is \
evaluated in order.</span></div></blockquote></div><br><div>Cool stuff. </div><div>I \
can definitely see use for catch-all-in-certain-range, opposite of filling every gap \
with their specifics, which is maintenance heavy. This matches the val_to_string() \
default string used when no match is found, but then in a higher dimension. I would \
say let the ranges decide, if their union is the same as the catch-all then it's \
okay, otherwise mark it.</div><div><br></div><div>just my \
€0.02</div><div>Jaap</div><div><br></div></div>___________________________________________________________________________<br>
Sent via: Wireshark-dev mailing list <<a \
href="mailto:wireshark-dev@wireshark.org" \
target="_blank">wireshark-dev@wireshark.org</a>><br>
Archives: <a href="https://www.wireshark.org/lists/wireshark-dev" \
rel="noreferrer" target="_blank">https://www.wireshark.org/lists/wireshark-dev</a><br>
Unsubscribe: <a href="https://www.wireshark.org/mailman/options/wireshark-dev" \
rel="noreferrer" target="_blank">https://www.wireshark.org/mailman/options/wireshark-dev</a><br>
mailto:<a href="mailto:wireshark-dev-request@wireshark.org" \
target="_blank">wireshark-dev-request@wireshark.org</a>?subject=unsubscribe</blockquote></div>
</blockquote></div>
</blockquote></div>
[Attachment #6 (text/plain)]
___________________________________________________________________________
Sent via: Wireshark-dev mailing list <wireshark-dev@wireshark.org>
Archives: https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-request@wireshark.org?subject=unsubscribe
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic