Re: Adam Roach's No Objection on draft-ietf-rtgwg-routing-types-16: (with COMMENT)

["Acee Lindem (acee)" <[e-mail]>]

Hi Adam, 

On 10/12/17, 12:12 PM, "Adam Roach" <[e-mail]> wrote:

>On 10/11/17 20:16, Acee Lindem (acee) wrote:
>>
>>> ____
>>>
>>> There are several patterns in the YANG definition that perform
>>>significant
>>> restriction of numbers (e.g., to ensure they don't fall outside the
>>>range
>>> that
>>> can be stored in 16 or 32 bits). In many cases, these patterns include
>>>the
>>> ability to zero-prefix some (but not all) decimal values. For example,
>>>the
>>> production for route-origin would allow leading zeros in "2:0100:0555"
>>> but not
>>> in "2:04294967295:065535" (even though "2:4294967295:65535" is okay). I
>>> don't
>>> know offhand whether it makes sense to allow leading zeros in these
>>> fields, but
>>> I would argue that the production should be consistent in allowing or
>>> disallowing them. This issue arises in various forms in route-target,
>>> ipv6-route-target, route-origin, and ipv6-route-origin.
>> We’ll look at this and get back to you - a lot of time has already gone
>> into formulating and testing these patterns.
>
>
>Yes, and it would be a shame if that work resulted in publishing
>patterns with known issues.
>
>This flaw arises in three formulations (each of which appear multiple
>times), and would be quite easy to fix. These fixes should be obvious by
>inspection.
>
>32 bits (0-4,294,967,295)
>   Replace: [0-3]?[0-9]{0,8}[0-9]
>   With:    [1-3][0-9]{9}|[1-9][0-9]{0,8}|0
>
>16 bits (0-65535)
>   Replace: [0-5]?[0-9]{0,3}[0-9]
>   With:    [1-5][0-9]{4}|[1-9][0-9]{0,3}|0
>
>8 bits (0-255)
>   Replace: [01]?[0-9]?[0-9]
>   With:    1[0-9]{2}|[1-9]?[0-9]

Yes - this doesn’t appear to be a complicate fix at all. We’re going to
get more eyes on it and do some tests with https://yangcatalog.org/yangre/
 but we should be able to fix this.

>
>____
>
>As an aside: replacing "[0-9]" with "\d" everywhere would make these
>patterns easier to read in general, but this is merely a readability
>improvement rather than a bug fix. Compare:
>
>          + '(2:(429496729[0-5]|42949672[0-8][0-9]|'
>          +     '4294967[01][0-9]{2}|'
>          +     '429496[0-6][0-9]{3}|42949[0-5][0-9]{4}|'
>          +     '4294[0-8][0-9]{5}|'
>          + '429[0-3][0-9]{6}|42[0-8][0-9]{7}|4[01][0-9]{8}|'
>          +     '[1-3][0-9]{9}|[1-9][0-9]{0,8}|0):'
>          +     '(6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}|'
>          +     '6[0-4][0-9]{3}|'
>          +     '[1-5][0-9]{4}|[1-9][0-9]{0,3}|0))|'
>
>Becomes:
>
>          + '(2:(429496729[0-5]|42949672[0-8]\d|'
>          +     '4294967[01]\d{2}|'
>          +     '429496[0-6]\d{3}|42949[0-5]\d{4}|'
>          +     '4294[0-8]\d{5}|'
>          +     '429[0-3]\d{6}|42[0-8]\d{7}|4[01]\d{8}|'
>          +     '[1-3]\d{9}|[1-9]\d{0,8}|0):'
>          +     '(6553[0-5]|655[0-2]\d|65[0-4]\d{2}|'
>          +     '6[0-4]\d{3}|'
>          +     '[1-5]\d{4}|[1-9]\d{0,3}|0))|'


Although this is a somewhat controversial subject, we used “[0-9]" for
portability for implementations using non-standard regular expression
parsers. 

Thanks,
Acee 

>
>
>/a