isisd: fix crash in display srv6 sid structure in json #16267

louis-6wind · 2024-06-21T09:47:41Z

Fix a crash when doing "show isis database detail json" in isis_srv6_topo1 topotest.
Rename a variable that was incorrectly named, leading to confusion and causing the crash bug
fix a JSON key

Fixes: 2e670cd ("isisd: fix display of srv6 subsubtlvs")
Fixes: 648a158 ("isisd: Add SRv6 End.X SID to Sub-TLV format func")

ton31337 · 2024-06-21T15:02:57Z

Can this f912eb0 commit be in a separate PR (to backport it to 9.1, 10.0)? Or are you planning to do a backports manually?

louis-6wind · 2024-06-21T15:03:58Z

I will do it manually

Fix a crash when doing "show isis database detail json" in isis_srv6_topo1 topotest. > #0 raise (sig=<optimized out>) at ../sysdeps/unix/sysv/linux/raise.c:50 > #1 0x00007fad89524e2c in core_handler (signo=6, siginfo=0x7ffe86a4b8b0, context=0x7ffe86a4b780) at lib/sigevent.c:258 > #2 <signal handler called> > #3 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > FRRouting#4 0x00007fad8904e537 in __GI_abort () at abort.c:79 > FRRouting#5 0x00007fad8904e40f in __assert_fail_base (fmt=0x7fad891c5688 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x7fad8a3e70e8 "json_object_get_type(jso) == json_type_object", > file=0x7fad8a3e7064 "./json_object.c", line=590, function=<optimized out>) at assert.c:92 > FRRouting#6 0x00007fad8905d662 in __GI___assert_fail (assertion=0x7fad8a3e70e8 "json_object_get_type(jso) == json_type_object", file=0x7fad8a3e7064 "./json_object.c", line=590, > function=0x7fad8a3e7440 "json_object_object_add_ex") at assert.c:101 > FRRouting#7 0x00007fad8a3dfe93 in json_object_object_add_ex () from /lib/x86_64-linux-gnu/libjson-c.so.5 > FRRouting#8 0x000055708e3f8f7f in format_subsubtlv_srv6_sid_structure (sid_struct=0x602000172b70, buf=0x0, json=0x6040000a21d0, indent=6) at isisd/isis_tlvs.c:2880 > FRRouting#9 0x000055708e3f9acb in isis_format_subsubtlvs (subsubtlvs=0x602000172b50, buf=0x0, json=0x6040000a21d0, indent=6) at isisd/isis_tlvs.c:3022 > FRRouting#10 0x000055708e3eefb0 in format_item_ext_subtlvs (exts=0x614000047440, buf=0x0, json=0x6040000a2190, indent=2, mtid=2) at isisd/isis_tlvs.c:1313 > FRRouting#11 0x000055708e3fd599 in format_item_extended_reach (mtid=2, i=0x60300015aed0, buf=0x0, json=0x6040000a1bd0, indent=0) at isisd/isis_tlvs.c:3763 > FRRouting#12 0x000055708e40d46a in format_item (mtid=2, context=ISIS_CONTEXT_LSP, type=ISIS_TLV_MT_REACH, i=0x60300015aed0, buf=0x0, json=0x6040000a1bd0, indent=0) at isisd/isis_tlvs.c:6789 > FRRouting#13 0x000055708e40d4fc in format_items_ (mtid=2, context=ISIS_CONTEXT_LSP, type=ISIS_TLV_MT_REACH, items=0x60600021d160, buf=0x0, json=0x6040000a1bd0, indent=0) at isisd/isis_tlvs.c:6804 > FRRouting#14 0x000055708e40edbc in format_mt_items (context=ISIS_CONTEXT_LSP, type=ISIS_TLV_MT_REACH, m=0x6180000845d8, buf=0x0, json=0x6040000a1bd0, indent=0) at isisd/isis_tlvs.c:7147 > FRRouting#15 0x000055708e4111e9 in format_tlvs (tlvs=0x618000084480, buf=0x0, json=0x6040000a1bd0, indent=0) at isisd/isis_tlvs.c:7572 > FRRouting#16 0x000055708e4114ce in isis_format_tlvs (tlvs=0x618000084480, json=0x6040000a1bd0) at isisd/isis_tlvs.c:7613 > FRRouting#17 0x000055708e36f167 in lsp_print_detail (lsp=0x612000058b40, vty=0x0, json=0x6040000a1bd0, dynhost=1 '\001', isis=0x60d00001f800) at isisd/isis_lsp.c:785 > FRRouting#18 0x000055708e36f31f in lsp_print_all (vty=0x0, json=0x6040000a0490, head=0x61f000005488, detail=1 '\001', dynhost=1 '\001', isis=0x60d00001f800) at isisd/isis_lsp.c:820 > FRRouting#19 0x000055708e4379fc in show_isis_database_lspdb_json (json=0x6040000a0450, area=0x61f000005480, level=0, lspdb=0x61f000005488, sysid_str=0x0, ui_level=1) at isisd/isisd.c:2683 > FRRouting#20 0x000055708e437ef9 in show_isis_database_json (json=0x6040000a0310, sysid_str=0x0, ui_level=1, isis=0x60d00001f800) at isisd/isisd.c:2754 > FRRouting#21 0x000055708e438357 in show_isis_database_common (vty=0x62e000060400, json=0x6040000a0310, sysid_str=0x0, ui_level=1, isis=0x60d00001f800) at isisd/isisd.c:2788 > FRRouting#22 0x000055708e438591 in show_isis_database (vty=0x62e000060400, json=0x6040000a0310, sysid_str=0x0, ui_level=1, vrf_name=0x7fad89806300 <vrf_default_name> "default", all_vrf=false) > at isisd/isisd.c:2825 > FRRouting#23 0x000055708e43891d in show_database (self=0x55708e5519c0 <show_database_cmd>, vty=0x62e000060400, argc=5, argv=0x6040000a02d0) at isisd/isisd.c:2855 > FRRouting#24 0x00007fad893a9767 in cmd_execute_command_real (vline=0x60300015f220, vty=0x62e000060400, cmd=0x0, up_level=0) at lib/command.c:1002 > FRRouting#25 0x00007fad893a9adc in cmd_execute_command (vline=0x60300015f220, vty=0x62e000060400, cmd=0x0, vtysh=0) at lib/command.c:1061 > FRRouting#26 0x00007fad893aa728 in cmd_execute (vty=0x62e000060400, cmd=0x621000025900 "show isis database detail json ", matched=0x0, vtysh=0) at lib/command.c:1227 Note that prior to 2e670cd, there was no crash but only the last "srv6-sid-structure" was displayed. A "srv6-sid-structure" should be displayed for each "sid". This commit also fix this. Was: > "srv6-lan-endx-sid": [ > { > "sid": "fc00:0:1:1::", > "weight": 0, > "algorithm": "SPF", > "neighbor-id": "0000.0000.0002" > }, > { > "sid": "fc00:0:1:2::", > "weight": 0, > "algorithm": "SPF", > "neighbor-id": "0000.0000.0003" > } > ], > "srv6-sid-structure": { > "loc-block-len": 32, > "loc-node-len": 16, > "func-len": 16, > "arg-len": 0 > }, Now (srv6-sid-structure are identical but they are not always): > "srv6-lan-endx-sid": [ > { > "sid": "fc00:0:1:1::", > "algorithm": "SPF", > "neighbor-id": "0000.0000.0002", > "srv6-sid-structure": { > "loc-block-len": 32, > "loc-node-len": 16, > "func-len": 8, > "arg-len": 0 > }, > }, > { > "sid": "fc00:0:1:2::", > "algorithm": "SPF", > "neighbor-id": "0000.0000.0003", > "srv6-sid-structure": { > "loc-block-len": 32, > "loc-node-len": 16, > "func-len": 16, > "arg-len": 0 > }, > } > ], Fixes: 2e670cd ("isisd: fix display of srv6 subsubtlvs") Fixes: 648a158 ("isisd: Add SRv6 End.X SID to Sub-TLV format func") Signed-off-by: Louis Scalbert <[email protected]>

louis-6wind · 2024-06-21T16:09:31Z

Can this f912eb0 commit be in a separate PR (to backport it to 9.1, 10.0)? Or are you planning to do a backports manually?

What I said in the commit log was incorrect, there is no crash in 10.0 etc. But the issue in previous versions is that only last key is displayed. The key should in the list element. I will not fix 10.0 because it has the same issue at many places in "show database detail json"

However, I have found a crash fix that was not backported in 10.0. Done in #16268

cscarpitta · 2024-06-24T07:51:45Z

isisd/isis_tlvs.c

@@ -1010,7 +1010,7 @@ static void format_item_ext_subtlvs(struct isis_ext_subtlvs *exts,
 		struct isis_adj_sid *adj;

 		if (json) {
-			struct json_object *arr_adj_json, *flags_json;
+			struct json_object *arr_adj_json, *srv6_end_sid_json;


Here we are showing the Adjacency SIDs of SR-MPLS (not SRv6).

Shouldn't it be something like adj_sid_json instead of srv6_end_sid_json?

done. I've set adj_sid_json

cscarpitta · 2024-06-24T08:02:32Z

isisd/isis_tlvs.c

@@ -1268,7 +1286,7 @@ static void format_item_ext_subtlvs(struct isis_ext_subtlvs *exts,
 		struct isis_srv6_endx_sid_subtlv *adj;

 		if (json) {
-			struct json_object *arr_adj_json, *flags_json;
+			struct json_object *arr_adj_json, *srv6_end_sid_json;


Can we change the name to srv6_endx_sid_json or srv6_end_x_sid_json?

srv6_end_sid_json could create confusion.

According to the RFC terminology,
SRv6 End SID => Prefix SID
SRv6 End.X SID => Adjacency SID

Here we are showing the Adjacency SIDs (SRv6 End.X SIDs).

done. I've set srv6_endx_sid_json

cscarpitta · 2024-06-24T08:04:55Z

isisd/isis_tlvs.c

@@ -1382,7 +1408,7 @@ static void format_item_ext_subtlvs(struct isis_ext_subtlvs *exts,
 	if (IS_SUBTLV(exts, EXT_SRV6_LAN_ENDX_SID)) {
 		struct isis_srv6_lan_endx_sid_subtlv *lan;
 		if (json) {
-			struct json_object *arr_adj_json, *flags_json;
+			struct json_object *arr_adj_json, *srv6_end_sid_json;


Ditto

Change to srv6_endx_sid_json or srv6_end_x_sid_json?

Done. I've set srv6_lan_endx_sid_json

cscarpitta · 2024-06-24T08:09:51Z

isisd/isis_tlvs.c

@@ -2613,24 +2647,24 @@ static void format_item_prefix_sid(uint16_t mtid, struct isis_item *i,
 			((sid->flags & ISIS_PREFIX_SID_LOCAL) ? "yes" : ""));
 		/* end deprecated keys (no booleans) */

-		struct json_object *flags_json;
+		struct json_object *srv6_end_sid_json;


Here we are showing Prefix SIDs of SR-MPLS (not SRv6).

Yes, it's the prefix SID, but this time it's the correct name. It is displaying the flags
I have kept flags_json

The variable flags_json was incorrectly named, leading to confusion and causing the bug fixed in the previous commit. Rename the variable to refer to SRv6 End SID instead. Cosmetic change. Signed-off-by: Louis Scalbert <[email protected]>

d587926 ("isisd: fix show database json format") renamed JSON keys to a standard format but forgot to rename the neighbor-id key. Fixes: d587926 ("isisd: fix show database json format") Signed-off-by: Louis Scalbert <[email protected]>

srv6EndSID is actually srv6EndXSID. Fixes: d587926 ("isisd: fix show database json format") Signed-off-by: Louis Scalbert <[email protected]>

ton31337 · 2024-06-24T12:12:56Z

Can this f912eb0 commit be in a separate PR (to backport it to 9.1, 10.0)? Or are you planning to do a backports manually?

What I said in the commit log was incorrect, there is no crash in 10.0 etc. But the issue in previous versions is that only last key is displayed. The key should in the list element. I will not fix 10.0 because it has the same issue at many places in "show database detail json"

However, I have found a crash fix that was not backported in 10.0. Done in #16268

c06fb90 this needs to be backported for 9.1 too, right?

louis-6wind · 2024-06-24T12:14:15Z

c06fb90 this needs to be backported for 9.1 too, right?

No c06fb90 is for 10.1 branch and master.

For previous versions, 1c39794 needs to backported. It is done in #16268 for 10.0. The same commit needs to backported in 9.1 and 9.0 as well.

cscarpitta

Thanks for the changes.

LGTM

riw777

looks good

github-actions bot added size/L master labels Jun 21, 2024

louis-6wind changed the title ~~isisd: fix display srv6 sid structure in json~~ isisd: fix crash in display srv6 sid structure in json Jun 21, 2024

louis-6wind force-pushed the fix-isis-srv6-json branch from a8517cc to 6fd9720 Compare June 21, 2024 16:06

cscarpitta reviewed Jun 24, 2024

View reviewed changes

louis-6wind force-pushed the fix-isis-srv6-json branch from 6fd9720 to 52cf168 Compare June 24, 2024 11:40

louis-6wind added 3 commits June 24, 2024 13:48

isisd: remane flags_json variable

c554b5d

The variable flags_json was incorrectly named, leading to confusion and causing the bug fixed in the previous commit. Rename the variable to refer to SRv6 End SID instead. Cosmetic change. Signed-off-by: Louis Scalbert <[email protected]>

isisd: fix neighbor id json key

3e82cf2

d587926 ("isisd: fix show database json format") renamed JSON keys to a standard format but forgot to rename the neighbor-id key. Fixes: d587926 ("isisd: fix show database json format") Signed-off-by: Louis Scalbert <[email protected]>

isisd: fix srv6 endx sid key name

cd2e872

srv6EndSID is actually srv6EndXSID. Fixes: d587926 ("isisd: fix show database json format") Signed-off-by: Louis Scalbert <[email protected]>

louis-6wind force-pushed the fix-isis-srv6-json branch from 52cf168 to cd2e872 Compare June 24, 2024 11:49

cscarpitta approved these changes Jun 24, 2024

View reviewed changes

riw777 approved these changes Jun 24, 2024

View reviewed changes

riw777 merged commit 3178350 into FRRouting:master Jun 24, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

isisd: fix crash in display srv6 sid structure in json #16267

isisd: fix crash in display srv6 sid structure in json #16267

louis-6wind commented Jun 21, 2024

ton31337 commented Jun 21, 2024

louis-6wind commented Jun 21, 2024

louis-6wind commented Jun 21, 2024 •

edited

Loading

cscarpitta Jun 24, 2024

louis-6wind Jun 24, 2024

cscarpitta Jun 24, 2024

louis-6wind Jun 24, 2024

cscarpitta Jun 24, 2024

louis-6wind Jun 24, 2024 •

edited

Loading

cscarpitta Jun 24, 2024

louis-6wind Jun 24, 2024

ton31337 commented Jun 24, 2024

louis-6wind commented Jun 24, 2024 •

edited

Loading

cscarpitta left a comment

riw777 left a comment

isisd: fix crash in display srv6 sid structure in json #16267

isisd: fix crash in display srv6 sid structure in json #16267

Conversation

louis-6wind commented Jun 21, 2024

ton31337 commented Jun 21, 2024

louis-6wind commented Jun 21, 2024

louis-6wind commented Jun 21, 2024 • edited Loading

cscarpitta Jun 24, 2024

Choose a reason for hiding this comment

louis-6wind Jun 24, 2024

Choose a reason for hiding this comment

cscarpitta Jun 24, 2024

Choose a reason for hiding this comment

louis-6wind Jun 24, 2024

Choose a reason for hiding this comment

cscarpitta Jun 24, 2024

Choose a reason for hiding this comment

louis-6wind Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

cscarpitta Jun 24, 2024

Choose a reason for hiding this comment

louis-6wind Jun 24, 2024

Choose a reason for hiding this comment

ton31337 commented Jun 24, 2024

louis-6wind commented Jun 24, 2024 • edited Loading

cscarpitta left a comment

Choose a reason for hiding this comment

riw777 left a comment

Choose a reason for hiding this comment

louis-6wind commented Jun 21, 2024 •

edited

Loading

louis-6wind Jun 24, 2024 •

edited

Loading

louis-6wind commented Jun 24, 2024 •

edited

Loading