This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: PR22245, Fix potential UB in bfd_set_error
- From: Pedro Alves <palves at redhat dot com>
- To: Alan Modra <amodra at gmail dot com>, binutils at sourceware dot org
- Cc: "Pavel I. Kryukov" <kryukov at frtk dot ru>
- Date: Wed, 4 Oct 2017 15:00:47 +0100
- Subject: Re: PR22245, Fix potential UB in bfd_set_error
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 76EEC63E21
- References: <20171004035949.GX25070@bubble.grove.modra.org>
Hi Alan,
On 10/04/2017 04:59 AM, Alan Modra wrote:
>
> -void bfd_set_error (bfd_error_type error_tag, ...);
> +void bfd_set_error (int error_tag, ...);
A downside here is that this 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
new -Wenum-conversion warning.
AFAICS, the only reason bfd_set_error is a variadic function is
to handle bfd_error_on_input.
I'd suggest instead to move the bfd_error_on_input functionality
to separate function, and make bfd_set_error non-variadic.
I gave it a quick try (only tested ld on x86_64 GNU/Linux), and
that exercise even caught an invalid use
of "bfd_set_error (bfd_error_on_input)" in elflink.c:
--- c/bfd/elflink.c
+++ w/bfd/elflink.c
@@ -10444,7 +10444,7 @@ elf_link_input_bfd (struct elf_final_link_info *flinfo, bfd *input_bfd)
(_("error: %B: size of section %A is not "
"multiple of address size"),
input_bfd, o);
- bfd_set_error (bfd_error_on_input);
+ bfd_set_input_error (input_bfd, bfd_get_error ());
return FALSE;
That's broken currently because bfd_set_error is going to
access random stack/registers as second and third arguments
and setting input_bfd and input_error from that.
I pushed this to the users/palves/bfd_set_input_error
branch in case you'd like to run with it.
>From 607d5e91a594de9da4454b950f34cda930aa9159 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 4 Oct 2017 14:20:51 +0100
Subject: [PATCH] bfd_set_input_error
---
bfd/archive.c | 2 +-
bfd/bfd-in2.h | 4 +++-
bfd/bfd.c | 51 +++++++++++++++++++++++++++++++++------------------
bfd/elflink.c | 2 +-
bfd/vms-lib.c | 2 +-
5 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/bfd/archive.c b/bfd/archive.c
index 3ce3f9e..1e87685 100644
--- a/bfd/archive.c
+++ b/bfd/archive.c
@@ -2309,7 +2309,7 @@ _bfd_write_archive_contents (bfd *arch)
return TRUE;
input_err:
- bfd_set_error (bfd_error_on_input, current, bfd_get_error ());
+ bfd_set_input_error (current, bfd_get_error ());
return FALSE;
}
diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index d126aed..4ba05b1 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -7054,7 +7054,9 @@ bfd_error_type;
bfd_error_type bfd_get_error (void);
-void bfd_set_error (bfd_error_type error_tag, ...);
+void bfd_set_error (bfd_error_type error_tag);
+
+void bfd_set_input_error (bfd *input, bfd_error_type error_tag);
const char *bfd_errmsg (bfd_error_type error_tag);
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 665f182..8a16594 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -497,32 +497,47 @@ FUNCTION
bfd_set_error
SYNOPSIS
- void bfd_set_error (bfd_error_type error_tag, ...);
+ void bfd_set_error (bfd_error_type error_tag);
DESCRIPTION
Set the BFD error condition to be @var{error_tag}.
- If @var{error_tag} is bfd_error_on_input, then this function
- takes two more parameters, the input bfd where the error
- occurred, and the bfd_error_type error.
+
+ @var{error_tag} must not be bfd_error_on_input. Use
+ bfd_set_input_error for input errors instead.
*/
void
-bfd_set_error (bfd_error_type error_tag, ...)
+bfd_set_error (bfd_error_type error_tag)
{
bfd_error = error_tag;
- if (error_tag == bfd_error_on_input)
- {
- /* This is an error that occurred during bfd_close when
- writing an archive, but on one of the input files. */
- va_list ap;
-
- va_start (ap, error_tag);
- input_bfd = va_arg (ap, bfd *);
- input_error = (bfd_error_type) va_arg (ap, int);
- if (input_error >= bfd_error_on_input)
- abort ();
- va_end (ap);
- }
+ if (bfd_error == bfd_error_on_input)
+ abort ();
+}
+
+/*
+FUNCTION
+ bfd_set_input_error
+
+SYNOPSIS
+ void bfd_set_input_error (bfd *input, bfd_error_type error_tag);
+
+DESCRIPTION
+
+ Set the BFD error condition to be bfd_error_on_input.
+ @var{input} is the input bfd where the error occurred, and
+ @var{error_tag} the bfd_error_type error.
+*/
+
+void
+bfd_set_input_error (bfd *input, bfd_error_type error_tag)
+{
+ /* This is an error that occurred during bfd_close when writing an
+ archive, but on one of the input files. */
+ bfd_error = bfd_error_on_input;
+ input_bfd = input;
+ input_error = error_tag;
+ if (input_error >= bfd_error_on_input)
+ abort ();
}
/*
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 982bf4f..ec959f3 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -10444,7 +10444,7 @@ elf_link_input_bfd (struct elf_final_link_info *flinfo, bfd *input_bfd)
(_("error: %B: size of section %A is not "
"multiple of address size"),
input_bfd, o);
- bfd_set_error (bfd_error_on_input);
+ bfd_set_input_error (input_bfd, bfd_get_error ());
return FALSE;
}
o->flags |= SEC_ELF_REVERSE_COPY;
diff --git a/bfd/vms-lib.c b/bfd/vms-lib.c
index 7b1320d..27f07e1 100644
--- a/bfd/vms-lib.c
+++ b/bfd/vms-lib.c
@@ -2305,7 +2305,7 @@ _bfd_vms_lib_write_archive_contents (bfd *arch)
return TRUE;
input_err:
- bfd_set_error (bfd_error_on_input, current, bfd_get_error ());
+ bfd_set_input_error (current, bfd_get_error ());
return FALSE;
}
--
2.5.5