Resolve strncmp warnings

[Andreas Hasenack]

[Impact]

* Users of frr reported warnings while building frr 8.1 with -Wstringop-overread.

* This crash is caused because the size parameter should be the size of the specific struct member that is being compared, alias and community, and not the whole struct. The fix is to get rid of sizeof(struct community_alias) in bgp_ca_alias_hash_key and bgp_ca_community_hash_cmp functions.

[Test Plan]

$ lxc launch ubuntu:22.04 jammy-frr
$ lxc shell jammy-frr

1. Install frr:
# apt install frr

2. Download frr 8.1 and unzip it.
3. Enter frr 8.1 after unzipping it.
4. Install dependencies:
# apt install -y autoconf automake libtool make gawk libreadline-dev texinfo pkg-config libpam0g-dev libjson-c-dev bison flex python3-pytest protobuf-c-compiler libprotobuf-c-dev libelf-dev pkg-config bison flex libc-ares-dev python3-dev python3-sphinx install-info build-essential libsnmp-dev perl libcap-dev python2 libunwind-dev libyang2 libyang2-dev

5. Add FRR user and groups:

# groupadd -r -g 92 frr
# groupadd -r -g 85 frrvty
# adduser --system --ingroup frr --home /var/run/frr/ --gecos "FRR suite" --shell /sbin/nologin frr
# usermod -a -G frrvty frr

6. #./bootstrap.sh
7. #./configure
8. # make -Wstringop-overread

Example of failed output:

  CC bgpd/bgp_community_alias.o
bgpd/bgp_community_alias.c: In function ‘bgp_ca_alias_hash_cmp’:
bgpd/bgp_community_alias.c:60:17: warning: ‘strncmp’ specified bound 8228 exceeds source size 8192 [-Wstringop-overread]
   60 | return (strncmp(ca1->alias, ca2->alias, sizeof(struct community_alias))
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bgpd/bgp_community_alias.c: In function ‘bgp_ca_community_hash_cmp’:
bgpd/bgp_community_alias.c:43:17: warning: ‘strncmp’ specified bound 8228 exceeds source size 36 [-Wstringop-overread]
   43 | return (strncmp(ca1->community, ca2->community,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   44 | sizeof(struct community_alias))
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Example of successful output:

No warnings for strncmp usage during building frr 8.1.

[Where problems could occur]

* The patch itself modifies only the bgp_community_alias.c and BGP community aliases are useful to quickly identify what communities are set for a specific prefix in a human-readable format, so regressions should be limited to that.

---------------------------------original report--------------------------

This was flagged in the MIR[1] review for frr.

There is an incorrect strncmp() usage in the code that was flagged by gcc:

bgpd/bgp_community_alias.c: In function ‘bgp_ca_alias_hash_cmp’:
bgpd/bgp_community_alias.c:60:17: warning: ‘strncmp’ specified bound 8228 exceeds source size 8192 [-Wstringop-overread]
   60 | return (strncmp(ca1->alias, ca2->alias, sizeof(struct community_alias))
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
bgpd/bgp_community_alias.c: In function ‘bgp_ca_community_hash_cmp’:
bgpd/bgp_community_alias.c:43:17: warning: ‘strncmp’ specified bound 8228 exceeds source size 36 [-Wstringop-overread]
   43 | return (strncmp(ca1->community, ca2->community,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   44 | sizeof(struct community_alias))
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I filed an upstream bug[2], and it was fixed[3].

1. https://bugs.launchpad.net/ubuntu/+source/frr/+bug/1951834
2. https://github.com/FRRouting/frr/issues/10484
3. https://github.com/FRRouting/frr/pull/10485

[Miriam España Acebal (mirespace)]

Fix was merged on dev/8.2 un upstream [1].

Mantic and Lunar are in 8.4, so the fix is there (and the build logs are clean too).

[1] https://github.com/FRRouting/frr/pull/10485

[Michał Małoszewski (michal-maloszewski99)]

Assigning to myself Jammy task.

[Steve Langasek (vorlon)]

The bug description says 'this crash'. But a crash is not demonstrated, only a compiler warning. The code is definitely wrong, as described; and the compiler warning is correct; but this bug report has not demonstrated that this results in a crash.

In fact, the upstream fix that has been proposed for backporting addresses the compiler warning by REMOVING the use of strncmp() in favor of strcmp(), which is less safe, and relying on null termination of the strings!

So this definitely doesn't look to me like something that should be SRUed.

[Michał Małoszewski (michal-maloszewski99)]

I set the bug status to Won't Fix and unassigned myself unless __at least__ there is a good patch that does not have any contraindications. Moreover, it should land to block-proposed, so ideally, it would be uploaded together with other changes then. Now, I confirm it's not SRUable.