Commit: Fix potential null pointer dereference in linker

Message ID 87tv6xi2nh.fsf@redhat.com
State New
Headers show
Series
  • Commit: Fix potential null pointer dereference in linker
Related show

Commit Message

Nick Clifton Nov. 21, 2019, 5:02 p.m.
Hi Guys,

  I am applying the patch below to fix a potential null pointer
  dereference in the linker.  This also fixes an annoying message from
  the undefined behaviour sanitizer...

Cheers
  Nick

ld/ChangeLog
2019-11-21  Nick Clifton  <nickc@redhat.com>

	* ldlang.h (LANG_FOR_EACH_INPUT_STATEMENT): Check for an empty
	file chain before examining the first input statement.

Comments

Alan Modra Nov. 21, 2019, 11:02 p.m. | #1
On Thu, Nov 21, 2019 at 05:02:10PM +0000, Nick Clifton wrote:
> Hi Guys,

> 

>   I am applying the patch below to fix a potential null pointer

>   dereference in the linker.


Actually, &file_chain.head->input_statement is exactly the same as
(lang_input_statement_type *) file_chain.head

Neither expression dereferences a pointer.  It's true in the general
case that the first expression might offset a pointer, and that
could require an extra check for NULL, but here the field offset is
zero.

>  This also fixes an annoying message from

>   the undefined behaviour sanitizer...


Yeah, well, nothing's perfect I guess.  I know in the past we've caved
in to every annoying false positive warning message from gcc, but if
we hide this sort of problem *in ubsan* will ubsan ever get fixed?

(The C standard does explicity say that in "&*p" the "*" is not
evaluated but unfortunately doesn't say the same for "->" in
"&p->field".  Which means ubsan authors need to use some common
sense.)

Is the ubsan complaint on every use of LANG_FOR_EACH_INPUT_STATEMENT,
and do you also get a complaint in ldlang.c:lang_for_each_input_file?
If not, then maybe something else is going on here.

See also PR23772, a related asan complaint that I'm about to close as
commit 36983a93bb avoided the construct.

> +  for (statement = file_chain.head == NULL ? NULL : &file_chain.head->input_statement; \


Please, can you just go back to using the cast here instead?  A while
ago I spent quite a bit of effort removing casts in ldlang.* but I
reckon a cast looks better than this overlong line.

-- 
Alan Modra
Australia Development Lab, IBM
Nick Clifton Nov. 22, 2019, 1:14 p.m. | #2
Hi Alan,

> Actually, &file_chain.head->input_statement is exactly the same as

> (lang_input_statement_type *) file_chain.head


> Please, can you just go back to using the cast here instead?  A while

> ago I spent quite a bit of effort removing casts in ldlang.* but I

> reckon a cast looks better than this overlong line.


Fair enough.  Done.

Cheers
   Nick

binutils/ChangeLog
2019-11-22  Nick Clifton  <nickc@redhat.com>

	* ldlang.h (LANG_FOR_EACH_INPUT_STATEMENT): Use cast instead of
	extra check.

Patch

diff --git a/ld/ldlang.h b/ld/ldlang.h
index 8cc5cf7f90..3e3e6a0289 100644
--- a/ld/ldlang.h
+++ b/ld/ldlang.h
@@ -574,7 +574,7 @@  extern asection *section_for_dot
 
 #define LANG_FOR_EACH_INPUT_STATEMENT(statement)			\
   lang_input_statement_type *statement;					\
-  for (statement = &file_chain.head->input_statement;			\
+  for (statement = file_chain.head == NULL ? NULL : &file_chain.head->input_statement; \
        statement != NULL;						\
        statement = statement->next)