[Buildroot] [PATCH v2] pppd: Provide error() implementation in pppoe-discovery

[Buildroot] [PATCH v2] pppd: Provide error() implementation in pppoe-discovery

[stefan.nickl at gmail.com]

From: Stefan Nickl <Stefan.Nickl@gmail.com>

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.

Signed-off-by: Stefan Nickl <Stefan.Nickl@gmail.com>

diff --git a/package/pppd/0002-rp-pppoe-error.patch b/package/pppd/0002-rp-pppoe-error.patch
new file mode 100644
index 0000000..f103f4d
--- /dev/null
+++ b/package/pppd/0002-rp-pppoe-error.patch
@@ -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... */
+ 
-- 
2.7.4

[Buildroot] [PATCH v2] pppd: Provide error() implementation in pppoe-discovery

[Thomas Petazzoni]

Hello,

On Sun, 23 Oct 2016 20:49:58 +0200, stefan.nickl at gmail.com wrote:
> From: Stefan Nickl <Stefan.Nickl@gmail.com>
> 
> 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.

What do you mean by doppelganger?

> diff --git a/package/pppd/0002-rp-pppoe-error.patch b/package/pppd/0002-rp-pppoe-error.patch
> new file mode 100644
> index 0000000..f103f4d
> --- /dev/null
> +++ b/package/pppd/0002-rp-pppoe-error.patch
> @@ -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.

> +@@ -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);
> ++}

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH v2] pppd: Provide error() implementation in pppoe-discovery

[Stefan Nickl]

Hi Thomas,

On Fri, Oct 28, 2016 at 3:24 PM, Thomas Petazzoni <
thomas.petazzoni@free-electrons.com> wrote:

> 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.


> 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.


>  - 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.
>

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.

-- 
Stefan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20161101/5f16bd4e/attachment.html>