Bug 20558 - POSIX bcopy/bzero decls do not implement Object Size Checking
Summary: POSIX bcopy/bzero decls do not implement Object Size Checking
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: string (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: 2.25
Assignee: Adhemerval Zanella
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-06 08:47 UTC by Yaakov Selkowitz
Modified: 2017-01-05 18:42 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yaakov Selkowitz 2016-09-06 08:47:21 UTC
Code using the POSIX.1 declarations of bcopy or bzero in <strings.h> do not benefit from their available Object Size Checking functionality even when compiled with -D_FORTIFY_SOURCE=*.

Test case:

$ cat bzero-test.c
#include <stdio.h>
#include <string.h>
#include <strings.h>

int main (void) {
  char buf1[9], buf2[2];
  bzero (buf1, 10);
  printf("%s %s\n", buf1, buf2);
  return 0;
}

In many cases, the BSD-compat declarations in <string.h> are used, which are properly handled:

$ gcc -O2 -D_FORTIFY_SOURCE=2 bzero-test.c
In file included from /usr/include/string.h:635:0,
                 from bzero-test.c:2:
In function ‘bzero’,
    inlined from ‘main’ at bzero-test.c:7:3:
/usr/include/bits/string3.h:103:3: warning: call to __builtin___memset_chk will always overflow destination buffer
   (void) __builtin___memset_chk (__dest, '\0', __len, __bos0 (__dest));
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
$ nm -C a.out | grep -E '(bzero|memset)'
                 U __memset_chk@@GLIBC_2.3.4


However, using the POSIX.1 declarations in <strings.h> are not:

$ gcc -O2 -D_FORTIFY_SOURCE=2 -D_POSIX_SOURCE bzero-test.c
$ nm -C a.out | grep -E '(bzero|memset)'
$ gcc -O2 -D_FORTIFY_SOURCE=2 -D_POSIX_C_SOURCE=200112L bzero-test.c
$ nm -C a.out | grep -E '(bzero|memset)'

Note that these functions were removed in POSIX.1-2008, so there is a fairly narrow window of compile options that would lead to this scenario.
Comment 1 Adhemerval Zanella 2017-01-04 14:20:07 UTC
From features.h, default if other than _GNU_SOURCE or _DEFAULT_SOURCE is defined _DEFAULT_SOURCE is used.  This is the case for the compiler usage on first case (gcc -O2 -D_FORTIFY_SOURCE=2).

Now, if one explicit specific _POSIX_SOURCE (second case), _DEFAULT_SOURCE is then not defined and then __USE_MISC is also not defined. __USE_MISC sets if the fortified macros for bzero/bcopy will be used.

This is the expected behavior imho opinion and the way to use the fortified wrappers for bzero/bcopy is to explicit defines _DEFAULT_SOURCE or __USE_MISC when _POSIX_SOURCE is used.

This is not a bug IMHO.
Comment 2 joseph@codesourcery.com 2017-01-04 16:48:17 UTC
Fortification should be orthogonal to other feature test macros.  That is, 
any function should be fortified if that function is declared, 
fortification for that function is supported and fortification is enabled.  
This should be independent of which header the declaration is obtained 
from.
Comment 3 Adhemerval Zanella 2017-01-04 18:10:58 UTC
In this case I think the definitions to declare bzero/bcopy on string/string.h is out the sync of the ones on strings.h.  The former just use __USE_MISC to declare the bzero/etc. where the latter also adds !defined __USE_XOPEN2K8.  My understanding of the conform tests is the latter should be the correct one.
Comment 4 joseph@codesourcery.com 2017-01-04 18:17:25 UTC
No standard includes those functions in string.h, only in strings.h.  
Thus __USE_MISC is correct in string.h; they can't be declared in string.h 
in strict conformance modes for any standard, whereas XSI POSIX before 
2008 includes these functions in strings.h (and non-XSI doesn't include 
strings.h at all before 2008) so !defined __USE_XOPEN2K8 is appropriate 
there.
Comment 5 Adhemerval Zanella 2017-01-04 18:25:37 UTC
So, at least then for string.h, the fortification omission of bzero/bcopy seems correct: string3.h is included in string.h and then the fortified version is just guarded by __USE_MISC.

I think correct solution would be to add a new fortified header meant to be included only by strings.h with the fortified macros for bzero/bcopy.
Comment 6 cvs-commit@gcc.gnu.org 2017-01-05 18:41:46 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  38765ab68f329fd84a5984decc8953f8c89d6e5c (commit)
      from  983a9637f730fa265e228509d090c4c5f031d713 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=38765ab68f329fd84a5984decc8953f8c89d6e5c

commit 38765ab68f329fd84a5984decc8953f8c89d6e5c
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Thu Jan 5 10:07:24 2017 -0200

    Use fortify macros for b{zero,copy} along decl from strings.h
    
    As described in BZ#20558, bzero and bcopy declaration can only benefit
    from fortified macros when decl came from string.h and when __USE_MISC
    is defined (default behaviour).
    
    This is due no standard includes those functions in string.h, so they
    are only declared if __USE_MISC is defined (as pointed out in comment 4).
    However fortification should be orthogona to other features test macros,
    i.e, any function should be fortified if that function is declared.
    
    To fix this behavior, the patch moved the bzero, bcopy, and
    __explicit_bzero_chk to a common header (string/bits/strings_fortified.h)
    and explicit fortified inclusion macros similar to string.h is added
    on strings.h.  This allows to get fortified declarions by only including
    strings.h.
    
    Checked on x86_64-linux-gnu and along on a bootstrap installation to check
    if the fortified are correctly triggered with example from bug report.
    
    	[BZ #20558]
    	* string/bits/string3.h [__USE_MISC] (bcopy): Move to
    	strings_fortified.h.
    	[__USE_MISC] (bzero): Likewise.
    	[__USE_MISC] (explicit_bzero): Likewise.
    	* string/strings.h: Include strings_fortified.h.
    	* string/Makefile (headers): Add strings_fortified.h.
    	* string/bits/strings_fortified.h: New file.
    	* include/bits/strings_fortified.h: Likewise.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                                          |   12 +++++++
 include/bits/strings_fortified.h                   |    1 +
 string/Makefile                                    |    3 +-
 string/bits/string3.h                              |   21 +-----------
 .../bits/strings_fortified.h                       |   35 +++++++++----------
 string/strings.h                                   |    8 ++++
 6 files changed, 41 insertions(+), 39 deletions(-)
 create mode 100644 include/bits/strings_fortified.h
 copy sysdeps/arm/tst-armtlsdesclocmod.c => string/bits/strings_fortified.h (55%)
Comment 7 Adhemerval Zanella 2017-01-05 18:42:13 UTC
Fixed by 38765ab68f329fd84a5.