Bug 22245 - Potential UB in bfd_set_error
Summary: Potential UB in bfd_set_error
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.29
: P2 normal
Target Milestone: ---
Assignee: Alan Modra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-03 20:04 UTC by Pavel I. Kryukov
Modified: 2017-10-05 01:34 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Last reconfirmed: 2017-10-04 00:00:00


Attachments
Fix potential UB in bfd_set_error (715 bytes, patch)
2017-10-03 20:07 UTC, Pavel I. Kryukov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel I. Kryukov 2017-10-03 20:04:22 UTC
Hello

My student Kirill Nedostoev tried to build binutils 2.29.1 with Clang 7 and he found a following message:

```
bfd.c:519:21: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Werror,-Wvarargs]`
      va_start (ap, error_tag);
```

The description of the actual problem is available on CERT site https://www.securecoding.cert.org/confluence/display/cplusplus/EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start

As a solution, we suggest to change signature of `bfd_set_error` in a following way:

-void bfd_set_error (bfd_error_type error_tag, ...);
+void bfd_set_error (int error_tag, ...);

To get a patch, you may pull from https://github.com/pavelkryukov/binutils.git

Thanks,
Pavel Kryukov
Comment 1 Pavel I. Kryukov 2017-10-03 20:07:49 UTC
Created attachment 10503 [details]
Fix potential UB in bfd_set_error
Comment 2 Alan Modra 2017-10-04 01:30:14 UTC
Thanks for the patch.  Will apply.
Comment 3 Sourceware Commits 2017-10-04 03:59:56 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9ba5f27cdd15d22d6c5739ff5d2b1c81d796e114

commit 9ba5f27cdd15d22d6c5739ff5d2b1c81d796e114
Author: Pavel I. Kryukov <kryukov@frtk.ru>
Date:   Tue Oct 3 22:42:07 2017 +0300

    PR22245, Fix potential UB in bfd_set_error
    
    Passing enum as a first argument to variadic argument function
    may lead to undefined behavior. The explanation on CERT site:
    https://www.securecoding.cert.org/confluence/display/cplusplus/
    EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start
    
    The bug was found by Kirill Nedostoev (nedostoev.ka@phystech.edu)
    when he tried to build GNU binutils with Clang 7.
    
    	PR 22245
    	* bfd.c (bfd_set_error): Avoid UB on passing arg to va_start that
    	undergoes default promotion.
    	* bfd-in2.h: Regenerate.
Comment 4 Sourceware Commits 2017-10-04 04:47:04 UTC
The binutils-2_29-branch branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=99ca76d3db25af8e017d7d54df677db0561907f5

commit 99ca76d3db25af8e017d7d54df677db0561907f5
Author: Pavel I. Kryukov <kryukov@frtk.ru>
Date:   Tue Oct 3 22:42:07 2017 +0300

    PR22245, Fix potential UB in bfd_set_error
    
    Passing enum as a first argument to variadic argument function
    may lead to undefined behavior. The explanation on CERT site:
    https://www.securecoding.cert.org/confluence/display/cplusplus/
    EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start
    
    The bug was found by Kirill Nedostoev (nedostoev.ka@phystech.edu)
    when he tried to build GNU binutils with Clang 7.
    
    	PR 22245
    	* bfd.c (bfd_set_error): Avoid UB on passing arg to va_start that
    	undergoes default promotion.
    	* bfd-in2.h: Regenerate.
    
    (cherry picked from commit 9ba5f27cdd15d22d6c5739ff5d2b1c81d796e114)
Comment 5 Pavel I. Kryukov 2017-10-04 06:34:41 UTC
Thank you!
Comment 6 Sourceware Commits 2017-10-05 01:34:51 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2ca7de3746be7484aa5affceafa1ad2e1d789381

commit 2ca7de3746be7484aa5affceafa1ad2e1d789381
Author: Pedro Alves <palves@redhat.com>
Date:   Wed Oct 4 14:20:51 2017 +0100

    bfd_set_input_error
    
    A downside to the 2017-10-04 PR22245 fix is that bfd_set_error can now
    silently accept invalid errors if/when someone passes the a value of
    the wrong enumeration type, which previously would be caught by the
    -Wenum-conversion warning.
    
    	PR 22245
    	* bfd.c (bfd_set_error): Revert 2017-10-04 change.  Remove
    	ellipsis parameter.  Split out bfd_error_on_input code to..
    	(bfd_set_input_error): .. New function.
    	* archive.c (_bfd_write_archive_contents): Use bfd_set_input_error.
    	* vms-lib.c (_bfd_vms_lib_write_archive_contents): Likewise.
    	* bfd-in2.h: Regenerate.