pppd: Provide error() implementation in pppoe-discovery
PATCH v2 1/1 pppd: Provide error() implementation in pppoe-discovery
From: Stefan Nickl <[email protected]>
The pppoe-discovery program calls error() from the CHECK_ROOM macro defined in pppoe.h. Since pppoe-discovery is a standalone program not linked with the rest of pppd, the only way this could build is by linking to glibc's proprietary error(3) function instead of the function of the same name (but with different arguments) defined in pppd/utils.c.
So with glibc and uClibc this builds, but will probably crash when the assertion is triggered. As the assertion is unlikely to fail, nobody has noticed. The build however fails with musl libc since it doesn't provide the doppelganger.
package/pppd/0002-rp-pppoe-error.patch new file +27 -0
@@ -0,0 +1,27 @@ +diff --git a/pppd/plugins/rp-pppoe/pppoe-discovery.c b/pppd/plugins/rp-pppoe/pppoe-discovery.c +index 3d3bf4e..55037df 100644 +--- a/pppd/plugins/rp-pppoe/pppoe-discovery.c ++++ b/pppd/plugins/rp-pppoe/pppoe-discovery.c +@@ -9,6 +9,7 @@ + * + */ + ++#include <stdarg.h> + #include <stdio.h> + #include <stdlib.h> + #include <unistd.h> +@@ -55,6 +56,14 @@ void die(int status) + exit(status); + } + ++void error(char *fmt, ...) ++{ ++ va_list pvar; ++ va_start(pvar, fmt); ++ vfprintf(stderr, fmt, pvar); ++ va_end(pvar); ++} ++ + /* Initialize frame types to RFC 2516 values. Some broken peers apparently + use different frame types... sigh... */ +
Signature · 1 line
2.7.4
PATCH v2 1/1 pppd: Provide error() implementation in pppoe-discovery
Hello,
Quoted 8 lines · stefan.nickl at gmail.com
From: Stefan Nickl <[email protected]>
The pppoe-discovery program calls
error()from theCHECK_ROOMmacro defined inpppoe.h. Since pppoe-discovery is a standalone program not linked with the rest of pppd, the only way this could build is by linking to glibc's proprietary error(3) function instead of the function of the same name (but with different arguments) defined inpppd/utils.c.
Quoted 4 lines
So with glibc and uClibc this builds, but will probably crash when the assertion is triggered. As the assertion is unlikely to fail, nobody has noticed. The build however fails with musl libc since it doesn't provide the doppelganger.
What do you mean by doppelganger?
package/pppd/0002-rp-pppoe-error.patch line 4
Quoted 10 lines
package/pppd/0002-rp-pppoe-error.patch new file +4
-0@@ -0,0 +1,27 @@ +diff --git a/pppd/plugins/rp-pppoe/pppoe-discovery.c b/pppd/plugins/rp-pppoe/pppoe-discovery.c +index 3d3bf4e..55037df 100644 +--- a/pppd/plugins/rp-pppoe/pppoe-discovery.c ++++ b/pppd/plugins/rp-pppoe/pppoe-discovery.c
The patch lacks a description + Signed-off-by.
Quoted 9 lines
package/pppd/0002-rp-pppoe-error.patch new file +9
-0+@@ -9,6 +9,7 @@ + * + */ + ++#include <stdarg.h> + #include <stdio.h> + #include <stdlib.h> + #include <unistd.h> +@@ -55,6 +56,14 @@ void die(int status)
package/pppd/0002-rp-pppoe-error.patch line 23
Quoted 10 lines
package/pppd/0002-rp-pppoe-error.patch new file +10
-0+ exit(status); + } + ++void error(char *fmt, ...) ++{ ++ va_list pvar; ++ va_start(pvar, fmt); ++ vfprintf(stderr, fmt, pvar); ++ va_end(pvar); ++}
This really doesn't seem like the right approach, for several reasons:
- There is already a function with the same name provided in the C library, which means static linking scenarios will no longer work, as you'll have clashing symbols.
- The rest of the pppoe-discovery code is using perror(), which is provided by the C library. So I believe the better fix is to change the CHECK_ROOM() macro to use the perror() function instead.
Could you rework your patch accordingly?
Thanks a lot,
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH v2 1/1 pppd: Provide error() implementation in pppoe-discovery
Hi Thomas,
Quoted 1 line · Thomas Petazzoni
What do you mean by doppelganger?
If you do "man 3 error" on a desktop Linux box, you'll get this function, which is provided by glibc and uclibc, but not by musl:
void error(int status, int errnum, const char *format, ...)
If you look at ppp/pppd/utils.c, you'll find a function with this prototype, looking like the one in my patch:
void error(char *fmt, ...)
The libc version is actually defined as a weak symbol:
# nm -D /lib/libc.so.6 |grep -w error 000f5990 W error
When rp-pppoe.so is built from pppd/plugins/rp-pppoe/common.c (among others), CHECK_ROOM expands into an error() call that matches the second signature, as is custom within ppp. When the plugin is dynamically loaded into pppd, that call to error() uses the function from utils.c, overriding the weak symbol in the libc.
But for the standalone pppoe-discovery, there is no error() in the program, and the linker just silently uses the unrelated/wrong weak symbol from libc instead.
It does not really matter because at most it makes the utility terminate cleanly instead of crashing if it detects some internal error, which may never even happen. I only minded because Arnout asked about it; it does matter for musl, but there it only solves one out of several issues.
Quoted 6 lines
This really doesn't seem like the right approach, for several reasons:
- There is already a function with the same name provided in the C library, which means static linking scenarios will no longer work, as you'll have clashing symbols.
As mentioned, the very idea is to provide a correct alternative so the weak/wrong error() from libc is not used.
Quoted 4 lines
- The rest of the pppoe-discovery code is using
perror(), which is provided by the C library. So I believe the better fix is to change theCHECK_ROOM()macro to use theperror()function instead.
Translation units that go into the rp-pppoe.so plugin also use this macro, and in this context, "void error(char *fmt, ...)" is certainly the correct choice. Also, perror(3) is for translating errno, which is not applicable for CHECK_ROOM. But there is also a lot of fprintf(stderr...) going on in pppoe-discovery.c.
Signature · 4 lines
Stefan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20161101/5f16bd4e/attachment.html>