Skip to content

bgpd: fix crash in aspath remove/replace chain, add a test for it #19575

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

bgpd: fix crash in aspath remove/replace chain, add a test for it #19575

wants to merge 2 commits into from

Conversation

@fdumontet6WIND
Copy link
Contributor

crash occurs with a combinaison of

as path replace route maps and remove-private-AS all
commands

    route-map OLDCORE-INTERNET-IN-IPv4 permit 5
     set as-path replace any 65398

     route-map OLDCORE-INTERNET-OUT-IPv4 permit 5
      set as-path replace any 65399

 neighbor 203.0.113.0 remove-private-AS all
     neighbor 203.0.113.0 soft-reconfiguration inbound
     neighbor 203.0.113.0 route-map OLDCORE-INTERNET-IN-IPv4 in
     neighbor 203.0.113.0 route-map OLDCORE-INTERNET-OUT-IPv4 out

issue is dur to a lack of test in "aspath_get_first_as" function.

fix add a test on as path segment length.

@fdumontet6WIND fdumontet6WIND changed the title As path crash bgpd: As path crash Sep 15, 2025
@ton31337
Copy link
Member

@Mergifyio backport stable/10.4 stable/10.3 stable/10.2

@mergify
Copy link

mergify bot commented Sep 15, 2025

backport stable/10.4 stable/10.3 stable/10.2

🟠 Waiting for conditions to match

  • merged [📌 backport requirement]

donaldsharp
donaldsharp reviewed Sep 15, 2025
View reviewed changes
@fdumontet6WIND
Copy link
Contributor Author

fdumontet6WIND commented Sep 15, 2025 •
edited
Loading

all AS had been removed from as-path by remove-private-AS all
since AS had been set to a private value by a previous route-map rule.

the configuration is, to say the least, not working well. but the end user did it
what we want is that the crash is not occuring.

@donaldsharp
Copy link
Member

If we didn't receive the AS path at all would the segment->len be 0? Do we properly handle 0 in all the other places that might be? It seems like we hsould have one way of representing no aspath at all and it should be consistent in the code base.

@fdumontet6WIND
Copy link
Contributor Author

in fact we receive an full as path,
in out the local AS will be added.

in code there are many tests on segment->length

@fdumontet6WIND
bgpd: fix crash in aspath remove/replace chain
9fb2180 
we encounter the following crash stack:
    rpki_curr_state=RPKI_NOT_BEING_USED, json_paths=0x5b4df1ae71c0, pattr=0x5b4df1bab350) at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_route.c:10945
    pathtype=BGP_PATH_SHOW_ALL, display=0x7ffe45d984c8, rpki_target_state=RPKI_NOT_BEING_USED, attr=0x5b4df1bab350) at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_route.c:12314
    json=0x5b4df1bdbb40, json_ar=0x5b4df1b52330, show_flags=513, header1=0x7ffe45d98810, header2=0x7ffe45d98814, rd_str=0x7ffe45d98870 "", match=0x0, output_count=0x7ffe45d98828,
    filtered_count=0x7ffe45d98830) at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_route.c:14612
    at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_route.c:14798
    argv=0x5b4df1bf9a50, viewvrfname=0x5b4df1ab0d30 "INTERNET", all=0x0, neighbors=0x5b4df1bba0a0 "198.18.253.15", route_map=0x0, prefix=0x7ffe45d9aa20, prefix_str=0x0,
    detail=0x5b4df1c34a50 "detail", uj=0x5b4df1b9d390 "json", wide=0x0) at /build/make-pkg/output/_packages/cp-routing/src/bgpd/bgp_route.c:14968
    argv=0x5b4df1bf9a50) at ./bgpd/bgp_route_clippy.c:796
    at /build/make-pkg/output/_packages/cp-routing/src/lib/command.c:1209
    at /build/make-pkg/output/_packages/cp-routing/src/lib/vty.c:627

crash occurs with a combinaison of

as path replace route maps and remove-private-AS all
commands

        route-map OLDCORE-INTERNET-IN-IPv4 permit 5
         set as-path replace any 65398

         route-map OLDCORE-INTERNET-OUT-IPv4 permit 5
          set as-path replace any 65399

	 neighbor 203.0.113.0 remove-private-AS all
         neighbor 203.0.113.0 soft-reconfiguration inbound
         neighbor 203.0.113.0 route-map OLDCORE-INTERNET-IN-IPv4 in
         neighbor 203.0.113.0 route-map OLDCORE-INTERNET-OUT-IPv4 out

issue is dur to a lack of test in "aspath_get_first_as" function.

fix add a test on as path segment length.

Signed-off-by: Francois Dumontet <[email protected]>
@fdumontet6WIND
test: add a configuration providing crasho to "bgp_remove_private_as"
d082034 
we add a configuration:

            configure terminal
             route-map OLDCORE-INTERNET-IN-IPv4 permit 5
              set as-path replace any 65398
             exit
             route-map OLDCORE-INTERNET-OUT-IPv4 permit 5
              set as-path replace any 65399
             exit
            router bgp 65002
             bgp deterministic-med
             address-family ipv4 unicast
              neighbor 203.0.113.0 remove-private-AS all
              neighbor 203.0.113.0 soft-reconfiguration inbound
              neighbor 203.0.113.0 route-map OLDCORE-INTERNET-IN-IPv4 in
              neighbor 203.0.113.0 route-map OLDCORE-INTERNET-OUT-IPv4 out
             exit-address-family

that leads to a crash.

Signed-off-by: Francois Dumontet <[email protected]>
ton31337
ton31337 reviewed Sep 16, 2025
View reviewed changes
tests/topotests/bgp_remove_private_as/test_bgp_remove_private_as.py
_crash_scenario()


exit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftovers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up

ton31337
ton31337 reviewed Sep 16, 2025
View reviewed changes
bgpd/bgp_aspath.c
unsigned int aspath_get_first_as(struct aspath *aspath)
{
if (aspath == NULL || aspath->segments == NULL)
if (aspath == NULL || aspath->segments == NULL || aspath->segments->length == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we should release aspath->segments if the length is 0 (if we use replace as / remove as)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that point is pertinent. i will study the impact.

riw777
riw777 reviewed Sep 16, 2025
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good ... waiting on @ton31337 's comments

@Jafaral Jafaral changed the title bgpd: As path crash bgpd: fix crash in aspath remove/replace chain, add a test for it Sep 16, 2025
@Jafaral Jafaral added this to the 10.5 milestone Sep 24, 2025
@ton31337
Copy link
Member

@fdumontet6WIND any updates?

@fdumontet6WIND
Copy link
Contributor Author

my concern is that if we free the aspath->segments, we will need to allocate it (shortly) after. Since many rules are applied.

@riw777
Copy link
Member

riw777 commented Oct 28, 2025

my concern is that if we free the aspath->segments, we will need to allocate it (shortly) after. Since many rules are applied.

otoh, we might not ... the only way to solve this is to set a timer to clear the memory at some point in the future, as we don't want a leak ... that seems like a lot of work, though (?)

@fdumontet6WIND
Copy link
Contributor Author

Hi Russ,
I don't undurstand your comment.
aspath has a segment on wich we made some operations ( add remove, change).
when aspath is freed , segment is also freed.
an aspath is modified when received, treated, sent.
at one point its segment may be reduced to null size and after some values added.

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

Reviewers

@riw777 riw777 riw777 left review comments

@ton31337 ton31337 ton31337 left review comments

@donaldsharp donaldsharp donaldsharp left review comments

Assignees

No one assigned

Labels

backport bgp bugfix master size/M

Projects

None yet

Milestone

10.5

Development

Successfully merging this pull request may close these issues.

5 participants

@fdumontet6WIND @ton31337 @donaldsharp @riw777 @Jafaral