lib/if: fix interface name comparison #11146

NicolasDichtel · 2022-05-04T12:35:10Z

Using strtol() to compare two strings is a bad idea.
Before the patch, if_cmp_name_func() may confuse foo001 and foo1.

Fixes: 106d2fd ("2003-08-01 Cougar [email protected]")
Signed-off-by: Nicolas Dichtel [email protected]
Acked-by: Philippe Guibert [email protected]

NicolasDichtel · 2022-05-04T12:57:25Z

Here are some tests to compare the result of the functions
if_cmp_name_func() before my patch / if_cmp_name_func() after my patch / strcmp()

$ ./sandbox
foo vs bar: 4 / 4 / 4
foo-001 vs bar-001: 4 / 4 / 4
foo-001 vs foo-1: 0 / -1 / -1
foo-100 vs foo-1: 1 / 1 / 48
foo-1_bar-01 vs foo-1_bar-1: 0 / -1 / -1
foo-1_bar-01 vs foo-1_bar-10: -1 / -1 / -1
foo-1_bar-1 vs foo-1_bar-10: -1 / -1 / -48
foo-1_bar-1 vs foo-1_bar-1: 0 / 0 / 0
foo1 vs foo2: -1 / -1 / -1
foo2 vs foo10: -1 / -1 / 1
foo10 vs foo11: -1 / -1 / -1
123foo vs 123bar: 4 / 4 / 4
007 vs 7: 0 / -7 / -7
007 vs 0: 1 / 1 / 48
007-foo vs 7-foo: 0 / -7 / -7

It seems this function was implemented to classify names like 'foo2' before 'foo10'. Not sure why this is needed.

NetDEF-CI · 2022-05-04T16:45:57Z

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5206/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests debian 10 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9DEB10AMD64-5206/test

Topology Tests failed for Topotests debian 10 amd64 part 9
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5206/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt

Successful on other platforms/tests

Topotests Ubuntu 18.04 arm8 part 7
Addresssanitizer topotests part 9
Topotests debian 10 amd64 part 0
IPv4 ldp protocol on Ubuntu 18.04
Topotests debian 10 amd64 part 5
Topotests Ubuntu 18.04 arm8 part 2
Ubuntu 16.04 deb pkg check
Topotests Ubuntu 18.04 i386 part 7
Addresssanitizer topotests part 7
Topotests Ubuntu 18.04 arm8 part 8
Topotests Ubuntu 18.04 i386 part 6
Topotests Ubuntu 18.04 i386 part 1
Topotests Ubuntu 18.04 arm8 part 6
Topotests Ubuntu 18.04 amd64 part 5
Topotests Ubuntu 18.04 i386 part 2
Topotests Ubuntu 18.04 arm8 part 1
Topotests Ubuntu 18.04 amd64 part 7
Addresssanitizer topotests part 3
Fedora 29 rpm pkg check
Topotests Ubuntu 18.04 i386 part 5
Topotests Ubuntu 18.04 i386 part 0
Topotests Ubuntu 18.04 i386 part 3
Topotests Ubuntu 18.04 i386 part 8
Topotests Ubuntu 18.04 amd64 part 2
IPv6 protocols on Ubuntu 18.04
Topotests debian 10 amd64 part 8
Topotests Ubuntu 18.04 arm8 part 4
Topotests Ubuntu 18.04 amd64 part 4
CentOS 7 rpm pkg check
Topotests Ubuntu 18.04 amd64 part 3
Topotests Ubuntu 18.04 amd64 part 0
Topotests Ubuntu 18.04 arm8 part 9
Addresssanitizer topotests part 2
Topotests Ubuntu 18.04 i386 part 4
IPv4 protocols on Ubuntu 18.04
Static analyzer (clang)
Topotests Ubuntu 18.04 amd64 part 1
Topotests debian 10 amd64 part 7
Topotests Ubuntu 18.04 amd64 part 9
Topotests Ubuntu 18.04 arm8 part 0
Debian 9 deb pkg check
Topotests debian 10 amd64 part 2
Addresssanitizer topotests part 8
Topotests Ubuntu 18.04 amd64 part 8
Debian 10 deb pkg check
Topotests Ubuntu 18.04 arm8 part 5
Topotests Ubuntu 18.04 amd64 part 6
Ubuntu 18.04 deb pkg check
Addresssanitizer topotests part 6
Ubuntu 20.04 deb pkg check
Topotests debian 10 amd64 part 6
Addresssanitizer topotests part 5
Topotests debian 10 amd64 part 1
Addresssanitizer topotests part 4
Addresssanitizer topotests part 0
Topotests debian 10 amd64 part 3
Topotests debian 10 amd64 part 4
Topotests Ubuntu 18.04 arm8 part 3
Topotests Ubuntu 18.04 i386 part 9
Addresssanitizer topotests part 1

Warnings Generated during build:

Checkout code: Successful with additional warnings

Topotests debian 10 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9DEB10AMD64-5206/test

Topology Tests failed for Topotests debian 10 amd64 part 9
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5206/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt

Report for if.c | 2 issues
===============================================
< WARNING: line over 80 characters
< #125: FILE: /tmp/f1-6791/if.c:125:

NetDEF-CI · 2022-05-04T17:09:24Z

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5207/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

CentOS 7 rpm pkg check: Failed (click for details)

CentOS 7 rpm pkg check: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5207/artifact/CENTOS7RPM/ErrorLog/log_package_start.txt CentOS 7 rpm pkg check: No useful log found

Topotests Ubuntu 18.04 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-5207/test

Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5207/artifact/TOPO9U18AMD64/ErrorLog/log_topotests.txt

Successful on other platforms/tests

Topotests Ubuntu 18.04 arm8 part 7
Addresssanitizer topotests part 9
Topotests debian 10 amd64 part 0
IPv4 ldp protocol on Ubuntu 18.04
Ubuntu 16.04 deb pkg check
Topotests debian 10 amd64 part 5
Topotests Ubuntu 18.04 arm8 part 2
Topotests Ubuntu 18.04 i386 part 1
Topotests Ubuntu 18.04 i386 part 7
Topotests Ubuntu 18.04 i386 part 6
Addresssanitizer topotests part 7
Topotests Ubuntu 18.04 arm8 part 8
Topotests Ubuntu 18.04 arm8 part 6
Topotests Ubuntu 18.04 i386 part 2
Topotests Ubuntu 18.04 amd64 part 5
Addresssanitizer topotests part 3
Fedora 29 rpm pkg check
Topotests Ubuntu 18.04 amd64 part 7
Topotests Ubuntu 18.04 i386 part 5
Topotests Ubuntu 18.04 i386 part 0
Topotests Ubuntu 18.04 i386 part 3
Topotests Ubuntu 18.04 i386 part 8
Topotests Ubuntu 18.04 amd64 part 2
IPv6 protocols on Ubuntu 18.04
Topotests debian 10 amd64 part 8
Addresssanitizer topotests part 2
Topotests Ubuntu 18.04 arm8 part 4
Topotests Ubuntu 18.04 amd64 part 4
Topotests Ubuntu 18.04 arm8 part 9
Topotests Ubuntu 18.04 amd64 part 0
Topotests Ubuntu 18.04 amd64 part 3
Topotests debian 10 amd64 part 9
Topotests Ubuntu 18.04 i386 part 4
Static analyzer (clang)
IPv4 protocols on Ubuntu 18.04
Topotests debian 10 amd64 part 7
Topotests Ubuntu 18.04 arm8 part 0
Debian 9 deb pkg check
Addresssanitizer topotests part 8
Topotests debian 10 amd64 part 2
Topotests Ubuntu 18.04 amd64 part 8
Debian 10 deb pkg check
Topotests Ubuntu 18.04 arm8 part 5
Topotests Ubuntu 18.04 amd64 part 6
Addresssanitizer topotests part 6
Topotests Ubuntu 18.04 amd64 part 1
Ubuntu 18.04 deb pkg check
Ubuntu 20.04 deb pkg check
Topotests debian 10 amd64 part 1
Addresssanitizer topotests part 5
Topotests debian 10 amd64 part 6
Topotests Ubuntu 18.04 arm8 part 1
Addresssanitizer topotests part 4
Addresssanitizer topotests part 0
Topotests debian 10 amd64 part 3
Topotests debian 10 amd64 part 4
Topotests Ubuntu 18.04 arm8 part 3
Topotests Ubuntu 18.04 i386 part 9
Addresssanitizer topotests part 1

idryzhov

Two comments, otherwise looks reasonable.

lib/if.c

vjardin

LGTM

lib/if.c

vjardin · 2022-05-04T22:09:41Z

@NicolasDichtel : you should add to FRRouting tests your sandbox testing in order to avoid another regression for the next 19y ;)

NicolasDichtel · 2022-05-06T07:16:11Z

@NicolasDichtel : you should add to FRRouting tests your sandbox testing in order to avoid another regression for the next 19y ;)

Yes, this is probably the oldest bug I've ever spotted :)
I copy and paste the function to a single .c file, to check the result before/after my patch. It should be reworked if we want to integrate it in frr.

NetDEF-CI · 2022-05-09T15:53:44Z

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Failed

OpenBSD 7 amd64 build: Failed (click for details)

Make failed for OpenBSD 7 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI011BUILD/ErrorLog/log_make.txt)

/home/ci/cibuild.5282/frr-source/doc/user/zebra.rst:721: WARNING: duplicate clicmd description of locator NAME, other instance in bgp
/home/ci/cibuild.5282/frr-source/doc/user/zebra.rst:1410: WARNING: duplicate clicmd description of debug zebra pbr, other instance in pbr
lib/if.c:80:9: error: implicit declaration of function 'strverscmp' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
1 error generated.
gmake[1]: *** [Makefile:10319: lib/if.lo] Error 1
gmake[1]: Target 'all-am' not remade because of errors.
gmake[1]: Leaving directory '/home/ci/cibuild.5282/frr-source'
gmake: *** [Makefile:6341: all] Error 2

OpenBSD 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI011BUILD/config.status/config.status
OpenBSD 7 amd64 build: Unknown Log <config.log.gz>
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI011BUILD/config.log/config.log.gz

FreeBSD 11 amd64 build: Failed (click for details)

Make failed for FreeBSD 11 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI009BUILD/ErrorLog/log_make.txt)

/usr/home/ci/cibuild.5282/frr-source/doc/user/zebra.rst:1410: WARNING: duplicate clicmd description of debug zebra pbr, other instance in pbr
lib/if.c: In function 'if_cmp_name_func':
lib/if.c:80:9: error: implicit declaration of function 'strverscmp'; did you mean 'strncmp'? [-Werror=implicit-function-declaration]
cc1: all warnings being treated as errors
gmake[1]: *** [Makefile:10319: lib/if.lo] Error 1
gmake[1]: Leaving directory '/usr/home/ci/cibuild.5282/frr-source'
gmake[1]: Target 'all-am' not remade because of errors.
gmake: *** [Makefile:6341: all] Error 2

FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI009BUILD/config.status/config.status
FreeBSD 11 amd64 build: Unknown Log <config.log.gz>
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI009BUILD/config.log/config.log.gz

NetBSD 9 amd64 build: Failed (click for details)

NetBSD 9 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI012BUILD/config.log/config.log.gz

Make failed for NetBSD 9 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI012BUILD/ErrorLog/log_make.txt)

doc/user/_build/texinfo/frr.texi:31508: warning: @image file `frr-figures/fig-vnc-redundant-route-reflectors.txt' (for text) unreadable: No such file or directory.
lib/if.c: In function 'if_cmp_name_func':
lib/if.c:80:9: error: implicit declaration of function 'strverscmp'; did you mean 'stresep'? [-Werror=implicit-function-declaration]
cc1: all warnings being treated as errors
gmake[1]: *** [Makefile:10320: lib/if.lo] Error 1
gmake[1]: Leaving directory '/home/ci/cibuild.5282/frr-source'
gmake[1]: Target 'all-am' not remade because of errors.
gmake: *** [Makefile:6342: all] Error 2

NetBSD 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/CI012BUILD/config.status/config.status

FreeBSD 12 amd64 build: Failed (click for details)

Make failed for FreeBSD 12 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/FBSD12AMD64/ErrorLog/log_make.txt)

gmake[1]: Entering directory '/usr/home/ci/cibuild.5282/frr-source'
lib/if.c: In function 'if_cmp_name_func':
lib/if.c:80:9: error: implicit declaration of function 'strverscmp'; did you mean 'strncmp'? [-Werror=implicit-function-declaration]
cc1: all warnings being treated as errors
gmake[1]: *** [Makefile:10316: lib/if.lo] Error 1
gmake[1]: Target 'all-am' not remade because of errors.
gmake[1]: Leaving directory '/usr/home/ci/cibuild.5282/frr-source'
gmake: *** [Makefile:6338: all] Error 2

FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/FBSD12AMD64/config.status/config.status
FreeBSD 12 amd64 build: Unknown Log <config.log.gz>
URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5282/artifact/FBSD12AMD64/config.log/config.log.gz

Successful on other platforms/tests

Fedora 29 amd64 build
Ubuntu 20.04 amd64 build
Ubuntu 18.04 arm8 build
Redhat 8 amd64 build
Debian 9 amd64 build
Debian 10 amd64 build
Ubuntu 18.04 ppc64le build
Ubuntu 16.04 arm7 build
Ubuntu 22.04 amd64 build
Ubuntu 16.04 arm8 build
Ubuntu 18.04 amd64 build
Debian 11 amd64 build
Ubuntu 18.04 i386 build
Ubuntu 16.04 amd64 build
Ubuntu 18.04 arm7 build
CentOS 7 amd64 build
Ubuntu 16.04 i386 build

NicolasDichtel · 2022-05-24T07:54:08Z

Philippe has proposed another version: #11213

Using strtol() to compare two strings is a bad idea. Before the patch, if_cmp_name_func() may confuse foo001 and foo1. PR=79407 Fixes: 106d2fd ("2003-08-01 Cougar <[email protected]>") Signed-off-by: Nicolas Dichtel <[email protected]> Tested-by: Aurélien Degeorges <[email protected]> Acked-by: Philippe Guibert <[email protected]>

NetDEF-CI · 2022-05-30T15:22:03Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5690/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings

Report for if.c | 2 issues
===============================================
< WARNING: line over 80 characters
< #125: FILE: /tmp/f1-1319/if.c:125:

pguibert6WIND · 2022-06-15T16:01:39Z

@idryzhov , please review this pull request

frrbot bot added the bugfix label May 4, 2022

NicolasDichtel force-pushed the master branch from db29a31 to 74c97dc Compare May 4, 2022 12:44

idryzhov requested changes May 4, 2022

View reviewed changes

lib/if.c Show resolved Hide resolved

lib/if.c Show resolved Hide resolved

vjardin approved these changes May 4, 2022

View reviewed changes

lib/if.c Show resolved Hide resolved

lib/if.c Show resolved Hide resolved

NicolasDichtel force-pushed the master branch from 74c97dc to 75105b1 Compare May 9, 2022 15:14

NicolasDichtel closed this May 24, 2022

idryzhov mentioned this pull request May 30, 2022

lib/if: fix interface name comparison #11213

Closed

NicolasDichtel reopened this May 30, 2022

github-actions bot added the master label May 30, 2022

NicolasDichtel force-pushed the master branch from 75105b1 to f33abb8 Compare May 30, 2022 12:30

idryzhov approved these changes Jun 20, 2022

View reviewed changes

idryzhov merged commit 2af4827 into FRRouting:master Jun 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/if: fix interface name comparison #11146

lib/if: fix interface name comparison #11146

NicolasDichtel commented May 4, 2022

NicolasDichtel commented May 4, 2022 •

edited

Loading

NetDEF-CI commented May 4, 2022 •

edited

Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Warnings Generated during build:

NetDEF-CI commented May 4, 2022 •

edited

Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

idryzhov left a comment

vjardin left a comment

vjardin commented May 4, 2022

NicolasDichtel commented May 6, 2022

NetDEF-CI commented May 9, 2022 •

edited

Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Failed

NicolasDichtel commented May 24, 2022

NetDEF-CI commented May 30, 2022

pguibert6WIND commented Jun 15, 2022

lib/if: fix interface name comparison #11146

lib/if: fix interface name comparison #11146

Conversation

NicolasDichtel commented May 4, 2022

NicolasDichtel commented May 4, 2022 • edited Loading

NetDEF-CI commented May 4, 2022 • edited Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Warnings Generated during build:

NetDEF-CI commented May 4, 2022 • edited Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

idryzhov left a comment

Choose a reason for hiding this comment

vjardin left a comment

Choose a reason for hiding this comment

vjardin commented May 4, 2022

NicolasDichtel commented May 6, 2022

NetDEF-CI commented May 9, 2022 • edited Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Failed

NicolasDichtel commented May 24, 2022

NetDEF-CI commented May 30, 2022

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

pguibert6WIND commented Jun 15, 2022

NicolasDichtel commented May 4, 2022 •

edited

Loading

NetDEF-CI commented May 4, 2022 •

edited

Loading

NetDEF-CI commented May 4, 2022 •

edited

Loading

NetDEF-CI commented May 9, 2022 •

edited

Loading