Patchwork [6/9] Add __vsyslog_internal, with same flags as __v*printf_internal.

login
register
mail settings
Submitter Zack Weinberg
Date March 7, 2018, 7:32 p.m.
Message ID <20180307193205.4751-7-zackw@panix.com>
Download mbox | patch
Permalink /patch/26228/
State New
Headers show

Comments

Zack Weinberg - March 7, 2018, 7:32 p.m.
__nldbl___vsyslog_chk will ultimately want to pass PRINTF_LDBL_IS_DBL
down to __vfprintf_internal *as well as* possibly setting PRINTF_FORTIFY.
To make that possible, we need a __vsyslog_internal that takes the
same flags as printf.  The code in misc/syslog.c does also get a
little simpler.

	* misc/syslog.c: Include libioP.h, not iolibio.h.
	(__vsyslog_internal): New function with the former body of
	__vsyslog_chk; takes mode_flags argument same as
	__v*printf_internal.  Call __vfprintf_internal directly.

	(__vsyslog_chk): Now a wrapper around __vsyslog_internal.
	Remove libc_hidden_def.
	(__syslog, __syslog_chk): Use __vsyslog_internal.
	(__vsyslog): Move to just below __syslog.  Use __vsyslog_internal.

	* include/sys/syslog.h: Add multiple inclusion guard.
	Add prototype for __vsyslog_internal.
	Remove libc_hidden_proto for __vsyslog_chk.

	* sysdeps/ieee754/ldbl-opt/nldbl-compat.c (__nldbl___vsyslog_chk):
	Use __vsyslog_internal.
---
 include/sys/syslog.h                    | 15 +++++++++++---
 misc/syslog.c                           | 35 ++++++++++++++++++---------------
 sysdeps/ieee754/ldbl-opt/nldbl-compat.c |  2 +-
 3 files changed, 32 insertions(+), 20 deletions(-)
Florian Weimer - March 13, 2018, 11:59 a.m.
On 03/07/2018 08:32 PM, Zack Weinberg wrote:

> +/* __vsyslog_internal uses the same mode_flags bits as
> +   __v*printf_internal; see libio/libioP.h.  */
> +extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list ap,
> +                                unsigned int mode_flags)
> +     __attribute__ ((__format__ (__printf__, 2, 0)));

I'm surprised that this doesn't need attribute_hidden or 
libc_hidden_proto to avoid new PLT calls.

Rest looks okay.

Thanks,
Florian
Zack Weinberg - March 13, 2018, 12:39 p.m.
On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 03/07/2018 08:32 PM, Zack Weinberg wrote:
>
>> +/* __vsyslog_internal uses the same mode_flags bits as
>> +   __v*printf_internal; see libio/libioP.h.  */
>> +extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list
>> ap,
>> +                                unsigned int mode_flags)
>> +     __attribute__ ((__format__ (__printf__, 2, 0)));
>
>
> I'm surprised that this doesn't need attribute_hidden or libc_hidden_proto
> to avoid new PLT calls.

That's only needed for functions that will be called _both_ from
inside and outside glibc.  This function is only ever called from
inside glibc, so it doesn't appear in any Versions files and it's
hidden by default.

zw
Florian Weimer - March 13, 2018, 12:43 p.m.
On 03/13/2018 01:39 PM, Zack Weinberg wrote:
> On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 03/07/2018 08:32 PM, Zack Weinberg wrote:
>>
>>> +/* __vsyslog_internal uses the same mode_flags bits as
>>> +   __v*printf_internal; see libio/libioP.h.  */
>>> +extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list
>>> ap,
>>> +                                unsigned int mode_flags)
>>> +     __attribute__ ((__format__ (__printf__, 2, 0)));
>>
>>
>> I'm surprised that this doesn't need attribute_hidden or libc_hidden_proto
>> to avoid new PLT calls.
> 
> That's only needed for functions that will be called _both_ from
> inside and outside glibc.  This function is only ever called from
> inside glibc, so it doesn't appear in any Versions files and it's
> hidden by default.

Some architectures will still use indirect calls without 
attribute_hidden, so please add it.  The existing tests do not catch 
this reliably unfortunately.

Thanks,
Florian
Zack Weinberg - March 13, 2018, 1:37 p.m.
On Tue, Mar 13, 2018 at 8:43 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 03/13/2018 01:39 PM, Zack Weinberg wrote:
>>
>> On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com>
>> wrote:
>>>
>>> On 03/07/2018 08:32 PM, Zack Weinberg wrote:
>>>
>>>> +/* __vsyslog_internal uses the same mode_flags bits as
>>>> +   __v*printf_internal; see libio/libioP.h.  */
>>>> +extern void __vsyslog_internal (int pri, const char *fmt,
>>>> __gnuc_va_list
>>>> ap,
>>>> +                                unsigned int mode_flags)
>>>> +     __attribute__ ((__format__ (__printf__, 2, 0)));
>>>
>>>
>>>
>>> I'm surprised that this doesn't need attribute_hidden or
>>> libc_hidden_proto
>>> to avoid new PLT calls.
>>
>>
>> That's only needed for functions that will be called _both_ from
>> inside and outside glibc.  This function is only ever called from
>> inside glibc, so it doesn't appear in any Versions files and it's
>> hidden by default.
>
>
> Some architectures will still use indirect calls without attribute_hidden,
> so please add it.  The existing tests do not catch this reliably
> unfortunately.

Can you be more specific?  This will affect all of the other new
__*_internal functions added in this patchset, so I need to know how
to be sure I got it right.  Also, this seems like something we should
find a way to automate if at all possible.

zw
Florian Weimer - March 13, 2018, 1:50 p.m.
On 03/13/2018 02:37 PM, Zack Weinberg wrote:
> On Tue, Mar 13, 2018 at 8:43 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 03/13/2018 01:39 PM, Zack Weinberg wrote:
>>>
>>> On Tue, Mar 13, 2018 at 7:59 AM, Florian Weimer <fweimer@redhat.com>
>>> wrote:
>>>>
>>>> On 03/07/2018 08:32 PM, Zack Weinberg wrote:
>>>>
>>>>> +/* __vsyslog_internal uses the same mode_flags bits as
>>>>> +   __v*printf_internal; see libio/libioP.h.  */
>>>>> +extern void __vsyslog_internal (int pri, const char *fmt,
>>>>> __gnuc_va_list
>>>>> ap,
>>>>> +                                unsigned int mode_flags)
>>>>> +     __attribute__ ((__format__ (__printf__, 2, 0)));
>>>>
>>>>
>>>>
>>>> I'm surprised that this doesn't need attribute_hidden or
>>>> libc_hidden_proto
>>>> to avoid new PLT calls.
>>>
>>>
>>> That's only needed for functions that will be called _both_ from
>>> inside and outside glibc.  This function is only ever called from
>>> inside glibc, so it doesn't appear in any Versions files and it's
>>> hidden by default.
>>
>>
>> Some architectures will still use indirect calls without attribute_hidden,
>> so please add it.  The existing tests do not catch this reliably
>> unfortunately.
> 
> Can you be more specific?  This will affect all of the other new
> __*_internal functions added in this patchset, so I need to know how
> to be sure I got it right.  Also, this seems like something we should
> find a way to automate if at all possible.

Consider this code:

$ cat call.c
int external (void) ATTR;

int
call (void)
{
   return external () + 1;
}

With default visibility, GCC 7 produces:

$ ppc64-linux-gnu-gcc -m32 -fPIC -c -O2 -DATTR= call.c && 
ppc64-linux-gnu-objdump -d --reloc call.o

call.o:     file format elf32-powerpc


Disassembly of section .text:

00000000 <call>:
    0:	94 21 ff e0 	stwu    r1,-32(r1)
    4:	7c 08 02 a6 	mflr    r0
    8:	42 9f 00 05 	bcl     20,4*cr7+so,c <call+0xc>
    c:	93 c1 00 18 	stw     r30,24(r1)
   10:	7f c8 02 a6 	mflr    r30
   14:	90 01 00 24 	stw     r0,36(r1)
   18:	3f de 00 00 	addis   r30,r30,0
			1a: R_PPC_REL16_HA	.got2+0x800e
   1c:	3b de 00 00 	addi    r30,r30,0
			1e: R_PPC_REL16_LO	.got2+0x8012
   20:	48 00 00 01 	bl      20 <call+0x20>
			20: R_PPC_PLTREL24	external+0x8000
   24:	80 01 00 24 	lwz     r0,36(r1)
   28:	83 c1 00 18 	lwz     r30,24(r1)
   2c:	38 21 00 20 	addi    r1,r1,32
   30:	38 63 00 01 	addi    r3,r3,1
   34:	7c 08 03 a6 	mtlr    r0
   38:	4e 80 00 20 	blr

With hidden visibility, we get instead:

$ ppc64-linux-gnu-gcc -m32 -fPIC -c -O2 -DATTR='__attribute ((visibility 
("hidden")))' call.c && ppc64-linux-gnu-objdump -d --reloc call.o

call.o:     file format elf32-powerpc


Disassembly of section .text:

00000000 <call>:
    0:	94 21 ff e0 	stwu    r1,-32(r1)
    4:	7c 08 02 a6 	mflr    r0
    8:	93 c1 00 18 	stw     r30,24(r1)
    c:	90 01 00 24 	stw     r0,36(r1)
   10:	48 00 00 01 	bl      10 <call+0x10>
			10: R_PPC_LOCAL24PC	external
   14:	80 01 00 24 	lwz     r0,36(r1)
   18:	83 c1 00 18 	lwz     r30,24(r1)
   1c:	38 21 00 20 	addi    r1,r1,32
   20:	38 63 00 01 	addi    r3,r3,1
   24:	7c 08 03 a6 	mtlr    r0
   28:	4e 80 00 20 	blr

The linker may have some optimization to eliminate the PLT indirection 
(blinding the localplt test), but it cannot get rid of the other 
unnecessary instructions.

Does this example help?

Thanks,
Florian
Zack Weinberg - March 13, 2018, 2:11 p.m.
On Tue, Mar 13, 2018 at 9:50 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> Some architectures will still use indirect calls without
>>> attribute_hidden,
>>> so please add it.  The existing tests do not catch this reliably
>>> unfortunately.
>>
>>
>> Can you be more specific?  This will affect all of the other new
>> __*_internal functions added in this patchset, so I need to know how
>> to be sure I got it right.  Also, this seems like something we should
>> find a way to automate if at all possible.
...
> The linker may have some optimization to eliminate the PLT indirection
> (blinding the localplt test), but it cannot get rid of the other unnecessary
> instructions.
>
> Does this example help?

I believe I understand the problem now, thank you.  And we can't/don't
use -fvisibility=hidden because then we would have to annotate all of
the _public_ symbols with visibility default, yes?

zw
Florian Weimer - March 13, 2018, 2:13 p.m.
On 03/13/2018 03:11 PM, Zack Weinberg wrote:
> I believe I understand the problem now, thank you.  And we can't/don't
> use -fvisibility=hidden because then we would have to annotate all of
> the_public_  symbols with visibility default, yes?

That, and on some architectures, it is impossible to call IFUNCs with 
hidden visibility declared to the compiler.

Thanks,
Florian

Patch

diff --git a/include/sys/syslog.h b/include/sys/syslog.h
index 3be3189ed1..459ca70746 100644
--- a/include/sys/syslog.h
+++ b/include/sys/syslog.h
@@ -1,11 +1,20 @@ 
+#ifndef _LIBC_SYS_SYSLOG_H
+#define _LIBC_SYS_SYSLOG_H 1
 #include <misc/sys/syslog.h>
-
 #ifndef _ISOMAC
+
 libc_hidden_proto (syslog)
 libc_hidden_proto (vsyslog)
 
+/* __vsyslog_internal uses the same mode_flags bits as
+   __v*printf_internal; see libio/libioP.h.  */
+extern void __vsyslog_internal (int pri, const char *fmt, __gnuc_va_list ap,
+                                unsigned int mode_flags)
+     __attribute__ ((__format__ (__printf__, 2, 0)));
+
 extern void __vsyslog_chk (int __pri, int __flag, const char *__fmt,
 			   __gnuc_va_list __ap)
      __attribute__ ((__format__ (__printf__, 3, 0)));
-libc_hidden_proto (__vsyslog_chk)
-#endif
+
+#endif /* _ISOMAC */
+#endif /* syslog.h */
diff --git a/misc/syslog.c b/misc/syslog.c
index 644dbe80ec..eb1283a604 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -53,7 +53,7 @@  static char sccsid[] = "@(#)syslog.c	8.4 (Berkeley) 3/18/94";
 
 #include <stdarg.h>
 
-#include <libio/iolibio.h>
+#include <libio/libioP.h>
 #include <math_ldbl_opt.h>
 
 #include <kernel-features.h>
@@ -114,24 +114,39 @@  __syslog(int pri, const char *fmt, ...)
 	va_list ap;
 
 	va_start(ap, fmt);
-	__vsyslog_chk(pri, -1, fmt, ap);
+	__vsyslog_internal(pri, fmt, ap, 0);
 	va_end(ap);
 }
 ldbl_hidden_def (__syslog, syslog)
 ldbl_strong_alias (__syslog, syslog)
 
+void
+__vsyslog(int pri, const char *fmt, va_list ap)
+{
+	__vsyslog_internal(pri, fmt, ap, 0);
+}
+ldbl_hidden_def (__vsyslog, vsyslog)
+ldbl_weak_alias (__vsyslog, vsyslog)
+
 void
 __syslog_chk(int pri, int flag, const char *fmt, ...)
 {
 	va_list ap;
 
 	va_start(ap, fmt);
-	__vsyslog_chk(pri, flag, fmt, ap);
+	__vsyslog_internal(pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
 	va_end(ap);
 }
 
 void
 __vsyslog_chk(int pri, int flag, const char *fmt, va_list ap)
+{
+	__vsyslog_internal(pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
+}
+
+void
+__vsyslog_internal(int pri, const char *fmt, va_list ap,
+		   unsigned int mode_flags)
 {
 	struct tm now_tm;
 	time_t now;
@@ -216,10 +231,7 @@  __vsyslog_chk(int pri, int flag, const char *fmt, va_list ap)
 
 	    /* We have the header.  Print the user's format into the
                buffer.  */
-	    if (flag == -1)
-	      vfprintf (f, fmt, ap);
-	    else
-	      __vfprintf_chk (f, flag, fmt, ap);
+            __vfprintf_internal (f, fmt, ap, mode_flags);
 
 	    /* Close the memory stream; this will finalize the data
 	       into a malloc'd buffer in BUF.  */
@@ -316,15 +328,6 @@  __vsyslog_chk(int pri, int flag, const char *fmt, va_list ap)
 	if (buf != failbuf)
 		free (buf);
 }
-libc_hidden_def (__vsyslog_chk)
-
-void
-__vsyslog(int pri, const char *fmt, va_list ap)
-{
-  __vsyslog_chk (pri, -1, fmt, ap);
-}
-ldbl_hidden_def (__vsyslog, vsyslog)
-ldbl_weak_alias (__vsyslog, vsyslog)
 
 static struct sockaddr_un SyslogAddr;	/* AF_UNIX address of local logger */
 
diff --git a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c
index 7e8af80d63..f00eb55eb5 100644
--- a/sysdeps/ieee754/ldbl-opt/nldbl-compat.c
+++ b/sysdeps/ieee754/ldbl-opt/nldbl-compat.c
@@ -842,7 +842,7 @@  attribute_compat_text_section
 __nldbl___vsyslog_chk (int pri, int flag, const char *fmt, va_list ap)
 {
   set_no_long_double ();
-  __vsyslog_chk (pri, flag, fmt, ap);
+  __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
   clear_no_long_double ();
 }
 libc_hidden_def (__nldbl___vsyslog_chk)