Mailing List Archive: as

as_t value

Mar 6, 2004, 3:32 PM

Post #1 of 3 (566 views)

I'm looking into some more compiler warnings, specifically on Mac OS X
right now. In particular, I've been looking at how AS values are
represented. The BGP RFC states that an AS can have valid values
between 1 and 65535. Obviously, 65535 is the largest number that a
16-bit integer can represent. The as_t value is mapped to a u_int16_t.
This means that the maximum number that can be stored in as_t is
65535. So, throughout the code, there are comparisons between a given
AS and certain "max" values:

bgp_aspath.c (line 608):
if (ntohs (assegment->asval[i]) < BGP_PRIVATE_AS_MIN
|| ntohs (assegment->asval[i]) > BGP_PRIVATE_AS_MAX)

In this example, the Mac OS X compiler throws a warning because
assegment->asval[i] > BGP_PRIVATE_AS_MAX can never happen (obviously
because as_t cannot hold a value greater than 65535). This comparison
is, therefore, irrelevant, but I'll get to that in a minute.

This example is more important:

bgp_routemap.c (line 2941):
as = strtoul (argv[0], &endptr, 10);
if (as == 0 || as == ULONG_MAX || *endptr != '\0')

First of all, "as" is of type as_t. The function strtoul will return
0, ULONG_MAX, or some value in between. In error cases, it will also
return 0 or ULONG_MAX. ULONG_MAX (on PPC) is defined as:

#define ULONG_MAX 0xffffffff /* max value for an unsigned
long */

So, by storing the strtoul value in as_t, we're automatically
typecasting a 32-bit integer to a 16-bit integer, and yet we're trying
to compare the 16-bit integer to ULONG_MAX (a 32-bit integer).

I guess what I'm getting at is the lack of consistency. Since the RFC
lists that AS numbers can only be 16-bit numbers, it makes sense that
*all* of the comparisons and strto* functions should reference 16-bit
values. However, my question is, we're almost to the half-way point in
AS number registrations (I know someone who got one a couple of weeks
ago that was around 32000. So, where will BGP take us? Will there be
a migration to 32-bit AS numbers? Should we ensure that all as_t types
(and similar values) are 32-bit instead of 16-bit so we're ready in the
future?

In summary, I'm looking for some advice before I submit changes to fix
consistency issues. If we should move to 32-bit numbers, then it makes
sense to change the as_t type and friends. If we're staying at 16-bit
numbers, then it makes sense to explicitly check the return value from
strtoul before saving it to the as_t (16-bit) value.

Comments?

--
PC Drew
Manager, Dominet

IBSN
1600 Broadway, Suite 400
Denver, CO 80202

Phone: 303-984-4727 x107
Cell: 720-841-4543
Fax: 303-984-4730
Email: drewpc@ibsncentral.com

Re: as_t value [ In reply to ]

gilad.arnold at terayon

Mar 7, 2004, 1:17 AM

Post #2 of 3 (562 views)

Permalink

PC Drew wrote:

> bgp_aspath.c (line 608):
> if (ntohs (assegment->asval[i]) < BGP_PRIVATE_AS_MIN
> || ntohs (assegment->asval[i]) > BGP_PRIVATE_AS_MAX)
>
> In this example, the Mac OS X compiler throws a warning because
> assegment->asval[i] > BGP_PRIVATE_AS_MAX can never happen (obviously
> because as_t cannot hold a value greater than 65535). This comparison
> is, therefore, irrelevant, but I'll get to that in a minute.

IMO, removing the second comparison would be a mistake: since from an
abstract (ie, the programming language) point of view,
BGP_PRIVATE_AS_MAX may be any 16-bit value (which just happened to be
USHRT_MAX), the conditional expression that's aimed to verify range
constraints must contain the upper bound. Namely, in he unlikely case
that BGP_PRIVATE_AS_MAX is changed to 65530, you would want the code to
keep working correctly. On the other hand, the warning is justified, in
which case I believe there are two appropriate options: first, is to
live with it, knowing that either way the optimizer would remove the
redundant part of the conditional; second, is using the C preprocessor
to avoid this "warning-and-fix" cycle by the compiler. I would suggest
something of this form:

#define BGP_AS_MAX 65535

...

if (ntohs (assegment->asval[i]) < BGP_PRIVATE_AS_MIN
#if (BGP_PRIVATE_AS_MAX < BGP_AS_MAX)
|| ntohs (assegment->asval[i]) > BGP_PRIVATE_AS_MAX
#endif
)

(Note that I wouldn't use USHRT_MAX for BGP_AS_MAX, as the former is
architecture dependant.)

I opt for the second option, of course.

> This example is more important:
>
> bgp_routemap.c (line 2941):
> as = strtoul (argv[0], &endptr, 10);
> if (as == 0 || as == ULONG_MAX || *endptr != '\0')
>
> First of all, "as" is of type as_t. The function strtoul will return 0,
> ULONG_MAX, or some value in between. In error cases, it will also
> return 0 or ULONG_MAX. ULONG_MAX (on PPC) is defined as:
>
> #define ULONG_MAX 0xffffffff /* max value for an unsigned
> long */
>
> So, by storing the strtoul value in as_t, we're automatically
> typecasting a 32-bit integer to a 16-bit integer, and yet we're trying
> to compare the 16-bit integer to ULONG_MAX (a 32-bit integer).

This looks like bad coding:

#1, generally speaking, you can't guarantee any consistency when
checking return values ranges after they are cast to some foreign type.
So I'd say the strtoul should be dumped and verified using an 'unsigned
long' temporary storage, before being converted into as_t.

#2, regarding strtoul() for this case of verifying an enumerator token,
I'd say that the correct error checking should only verify that *endptr
is '\0', and then that range is not exceeded. Nonetheless, following #1
above, it should be applied to the temporary 'unsigned long' variable.

#3, on the other hand, I suspect that this whole test is completely
redundant: isn't zebra's VTY designed to verify integer value ranges
whatsoever? (see for instance the use of a distance value in
zebra/zebra_vty.c, lines 287 and 129-132) Hence, seems like a simple
atoi() should do here, isn't it?

> I guess what I'm getting at is the lack of consistency. Since the RFC
> lists that AS numbers can only be 16-bit numbers, it makes sense that
> *all* of the comparisons and strto* functions should reference 16-bit
> values. However, my question is, we're almost to the half-way point in
> AS number registrations (I know someone who got one a couple of weeks
> ago that was around 32000. So, where will BGP take us? Will there be a
> migration to 32-bit AS numbers? Should we ensure that all as_t types
> (and similar values) are 32-bit instead of 16-bit so we're ready in the
> future?

??

Seems like way speculative question to me... ;->

> In summary, I'm looking for some advice before I submit changes to fix
> consistency issues. If we should move to 32-bit numbers, then it makes
> sense to change the as_t type and friends. If we're staying at 16-bit
> numbers, then it makes sense to explicitly check the return value from
> strtoul before saving it to the as_t (16-bit) value.

I'd say the latter is the reasonable option.

Gilad

Re: as_t value [ In reply to ]

gdt at ir

Mar 8, 2004, 5:52 AM

Post #3 of 3 (562 views)

Permalink

It might make sense to hvae an internal representation of as_t as
u_int32_t. There is talk of a larger space. But, this wouldn't fix
strtoul, since long is sometimes larger than u_int32_t. So arguably
strtoul results should be assigned to a long and checked for as
validity before assignment.

--
Greg Troxel <gdt@ir.bbn.com>