New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

packet/bgp: use strconv.ParseUint instead of strconv.Atoi() #1557

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@vincentbernat
Contributor

vincentbernat commented Jan 11, 2018

Atoi() returns a signed int. On a 32-bit platform, this is not big
enough to fit an unsigned 32-bit int. Replace all occurrences of
Atoi() to ParseUint() with the appropriate size as a parameter.

This fix this failure:

--- FAIL: Test_ParseEthernetSegmentIdentifier (0.00s)
        Error Trace:    bgp_test.go:1181
        Error:          Expected nil, but got: &errors.errorString{s:"invalid esi values for type ESI_AS: [2864434397 287454020]"}

        Error Trace:    bgp_test.go:1182
        Error:          Not equal: bgp.EthernetSegmentIdentifier{Type:0x5, Value:[]uint8{0xaa, 0xbb, 0xcc, 0xdd, 0x11, 0x22, 0x33, 0x44, 0x0}} (expected)
                                != bgp.EthernetSegmentIdentifier{Type:0x5, Value:[]uint8{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}} (actual)

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,2 +1,2 @@
                        -(bgp.EthernetSegmentIdentifier) ESI_AS | as 2864434397, local discriminator 287454020
                        +(bgp.EthernetSegmentIdentifier) ESI_AS | as 0, local discriminator 0

FAIL
FAIL    github.com/osrg/gobgp/packet/bgp        0.003s

There are other occurrences of Atoi() that seems suspicious when parsing command-line. However, I didn't have time to look at them. Please pay attention to fst and snd. I believe they are 16 bits, right?

packet/bgp: use strconv.ParseUint instead of strconv.Atoi()
Atoi() returns a signed int. On a 32-bit platform, this is not big
enough to fit an unsigned 32-bit int. Replace all occurrences of
Atoi() to ParseUint() with the appropriate size as a parameter.

This fix this failure:

```
--- FAIL: Test_ParseEthernetSegmentIdentifier (0.00s)
        Error Trace:    bgp_test.go:1181
        Error:          Expected nil, but got: &errors.errorString{s:"invalid esi values for type ESI_AS: [2864434397 287454020]"}

        Error Trace:    bgp_test.go:1182
        Error:          Not equal: bgp.EthernetSegmentIdentifier{Type:0x5, Value:[]uint8{0xaa, 0xbb, 0xcc, 0xdd, 0x11, 0x22, 0x33, 0x44, 0x0}} (expected)
                                != bgp.EthernetSegmentIdentifier{Type:0x5, Value:[]uint8{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}} (actual)

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,2 +1,2 @@
                        -(bgp.EthernetSegmentIdentifier) ESI_AS | as 2864434397, local discriminator 287454020
                        +(bgp.EthernetSegmentIdentifier) ESI_AS | as 0, local discriminator 0

FAIL
FAIL    github.com/osrg/gobgp/packet/bgp        0.003s
```
@iwaseyusuke

This comment has been minimized.

Show comment
Hide comment
@iwaseyusuke

iwaseyusuke Jan 11, 2018

Contributor

It looks good to me! Thanks!
I will replace the remaining strconv.Atoi() after this PR will be merged.

Please pay attention to fst and snd. I believe they are 16 bits, right?

Yes, right. fsn is the upper 16 bits of asn and snd is lower 16 bits.

Contributor

iwaseyusuke commented Jan 11, 2018

It looks good to me! Thanks!
I will replace the remaining strconv.Atoi() after this PR will be merged.

Please pay attention to fst and snd. I believe they are 16 bits, right?

Yes, right. fsn is the upper 16 bits of asn and snd is lower 16 bits.

@iwaseyusuke

This comment has been minimized.

Show comment
Hide comment
@iwaseyusuke

iwaseyusuke Jan 11, 2018

Contributor

Also I think better to prohibit the use of strconv.Atoi() and to checkout them in Travis-CI.
It might be overdoing, but be safer for the 32-bit platform...

Contributor

iwaseyusuke commented Jan 11, 2018

Also I think better to prohibit the use of strconv.Atoi() and to checkout them in Travis-CI.
It might be overdoing, but be safer for the 32-bit platform...

@vincentbernat

This comment has been minimized.

Show comment
Hide comment
@vincentbernat

vincentbernat Jan 11, 2018

Contributor

You can also run tests with GOARCH=386, that would catch the ones I fixed.

Contributor

vincentbernat commented Jan 11, 2018

You can also run tests with GOARCH=386, that would catch the ones I fixed.

@iwaseyusuke

This comment has been minimized.

Show comment
Hide comment
@iwaseyusuke

iwaseyusuke Jan 11, 2018

Contributor

@vincentbernat Thanks, I will try it!

Contributor

iwaseyusuke commented Jan 11, 2018

@vincentbernat Thanks, I will try it!

@fujita

This comment has been minimized.

Show comment
Hide comment
@fujita

fujita Jan 12, 2018

Member

nice, thanks.

Member

fujita commented Jan 12, 2018

nice, thanks.

@fujita fujita closed this Jan 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment