14722 ld should keep group members in separate output sections

Reviewed by: Gordon Ross <Gordon.W.Ross@gmail.com>
Reviewed by: Toomas Soome <tsoome@me.com>
Reviewed by: Igor Kozhukhov <igor@dilos.org>
Approved by: Dan McDonald <danmcd@mnx.io>

14 files changed, 297 insertions, 9 deletions

diff --git a/usr/src/cmd/sgs/elfdump/common/elfdump.c b/usr/src/cmd/sgs/elfdump/common/elfdump.c
index a5c8aa2200..b706afd12f 100644
--- a/usr/src/cmd/sgs/elfdump/common/elfdump.c
+++ b/usr/src/cmd/sgs/elfdump/common/elfdump.c
@@ -4258,7 +4258,7 @@ group(Cache *cache, Word shnum, const char *file, uint_t flags)
 			(void) snprintf(index, MAXNDXSIZE,
 			    MSG_ORIG(MSG_FMT_INDEX), EC_XWORD(gcnt));
 
-			if (grpdata[gcnt] >= shnum)
+			if ((grpdata[gcnt] == 0) || (grpdata[gcnt] >= shnum))
 				name = MSG_INTL(MSG_GRP_INVALSCN);
 			else
 				name = cache[grpdata[gcnt]].c_name;
diff --git a/usr/src/cmd/sgs/libld/common/groups.c b/usr/src/cmd/sgs/libld/common/groups.c
index 75b5a6fb88..9b3b9b184e 100644
--- a/usr/src/cmd/sgs/libld/common/groups.c
+++ b/usr/src/cmd/sgs/libld/common/groups.c
@@ -256,9 +256,9 @@ ld_group_process(Is_desc *gisc, Ofl_desc *ofl)
 	 * group, mark each section as COMDAT.
 	 */
 	for (ndx = 1; ndx < gd.gd_cnt; ndx++) {
-		Word	gndx;
+		Word	gndx = gd.gd_data[ndx];
 
-		if ((gndx = gd.gd_data[ndx]) >= gifl->ifl_shnum) {
+		if ((gndx == 0) || (gndx >= gifl->ifl_shnum)) {
 			ld_eprintf(ofl, ERR_FATAL,
 			    MSG_INTL(MSG_GRP_INVALNDX), gifl->ifl_name,
 			    EC_WORD(gisc->is_scnndx), gisc->is_name, ndx, gndx);
diff --git a/usr/src/cmd/sgs/libld/common/libld.msg b/usr/src/cmd/sgs/libld/common/libld.msg
index 8f61288f16..93e9889c0f 100644
--- a/usr/src/cmd/sgs/libld/common/libld.msg
+++ b/usr/src/cmd/sgs/libld/common/libld.msg
@@ -260,6 +260,7 @@
 #
 @ MSG_GRP_INVALNDX	"file %s: group section [%u]%s: entry %d: \
 			 invalid section index: %d"
+@ MSG_GRP_MISSINGOUT	"file %s: discarded section: [%u]%s: part of output group [%u]%s"
 
 # Relocation processing messages (some of these are required to satisfy
 # do_reloc(), which is common code used by cmd/sgs/rtld - make sure both
diff --git a/usr/src/cmd/sgs/libld/common/place.c b/usr/src/cmd/sgs/libld/common/place.c
index 2724ab20a4..53d9e856d5 100644
--- a/usr/src/cmd/sgs/libld/common/place.c
+++ b/usr/src/cmd/sgs/libld/common/place.c
@@ -1080,6 +1080,7 @@ ld_place_section(Ofl_desc *ofl, Is_desc *isp, Place_path_info *path_info,
 		 * -	The ident values match
 		 * -	The names match
 		 * -	Not a GROUP section
+		 * -	Not a GROUP member, if producing a relocatable object
 		 * -	Not a DTrace dof section
 		 * -	Section types match
 		 * -	Matching section flags, after screening out the
@@ -1096,6 +1097,8 @@ ld_place_section(Ofl_desc *ofl, Is_desc *isp, Place_path_info *path_info,
 		    (ident != ld_targ.t_id.id_rel) &&
 		    (onamehash == osp->os_namehash) &&
 		    (shdr->sh_type != SHT_GROUP) &&
+		    (((shdr->sh_flags & SHF_GROUP) == 0) ||
+		    ((ofl->ofl_flags & FLG_OF_RELOBJ) == 0)) &&
 		    (shdr->sh_type != SHT_SUNW_dof) &&
 		    ((shdr->sh_type == os_shdr->sh_type) ||
 		    (is_ehframe && (osp->os_flags & FLG_OS_EHFRAME)) ||
diff --git a/usr/src/cmd/sgs/libld/common/update.c b/usr/src/cmd/sgs/libld/common/update.c
index 0865ae64fa..87de49817d 100644
--- a/usr/src/cmd/sgs/libld/common/update.c
+++ b/usr/src/cmd/sgs/libld/common/update.c
@@ -3304,17 +3304,23 @@ update_ogroup(Ofl_desc *ofl)
 		gdata = (Word *)osp->os_outdata->d_buf;
 
 		for (i = 1; i < grpcnt; i++) {
-			Os_desc	*_osp;
 			Is_desc	*_isp = ifl->ifl_isdesc[gdata[i]];
+			Os_desc	*_osp = _isp->is_osdesc;
 
 			/*
-			 * If the referenced section didn't make it to the
-			 * output file - just zero out the entry.
+			 * If a section in the group is not part of the output
+			 * image, something has gone wrong, and corrupted this
+			 * group.
 			 */
-			if ((_osp = _isp->is_osdesc) == NULL)
-				gdata[i] = 0;
-			else
+			if (_osp == NULL) {
+				ld_eprintf(ofl, ERR_FATAL,
+				    MSG_INTL(MSG_GRP_MISSINGOUT),
+				    ifl->ifl_name, gdata[i], _isp->is_name,
+				    elf_ndxscn(osp->os_scn), osp->os_name);
+				error = S_ERROR;
+			} else {
 				gdata[i] = (Word)elf_ndxscn(_osp->os_scn);
+			}
 		}
 	}
 	return (error);
diff --git a/usr/src/cmd/sgs/tools/SUNWonld-README b/usr/src/cmd/sgs/tools/SUNWonld-README
index adcfecb0a9..83906f59c3 100644
--- a/usr/src/cmd/sgs/tools/SUNWonld-README
+++ b/usr/src/cmd/sgs/tools/SUNWonld-README
@@ -1688,3 +1688,4 @@ Bugid   Risk Synopsis
 14401	elfdump should understand LLVM section types
 4795	/usr/bin/ld manpage and help should indicate '-soname' not '--soname'
 14090	ld(1) could use a normal allocator
+14722	ld should keep group members in separate output sections
diff --git a/usr/src/pkg/manifests/system-test-elftest.p5m b/usr/src/pkg/manifests/system-test-elftest.p5m
index 490458e6d9..108876ce67 100644
--- a/usr/src/pkg/manifests/system-test-elftest.p5m
+++ b/usr/src/pkg/manifests/system-test-elftest.p5m
@@ -27,6 +27,13 @@ dir  path=opt/elf-tests/tests
 dir  path=opt/elf-tests/tests/assert-deflib
 file path=opt/elf-tests/tests/assert-deflib/link.c mode=0444
 file path=opt/elf-tests/tests/assert-deflib/test-deflib mode=0555
+dir  path=opt/elf-tests/tests/groups
+dir  path=opt/elf-tests/tests/groups/no-relobj-group-merge
+file path=opt/elf-tests/tests/groups/no-relobj-group-merge/README.md mode=0444
+file \
+    path=opt/elf-tests/tests/groups/no-relobj-group-merge/no-relobj-group-merge \
+    mode=0555
+file path=opt/elf-tests/tests/groups/no-relobj-group-merge/sections.s mode=0444
 dir  path=opt/elf-tests/tests/linker-sets
 file path=opt/elf-tests/tests/linker-sets/in-use-check mode=0555
 file path=opt/elf-tests/tests/linker-sets/simple mode=0555
diff --git a/usr/src/test/elf-tests/runfiles/default.run b/usr/src/test/elf-tests/runfiles/default.run
index 6d833f9b53..0005b8976b 100644
--- a/usr/src/test/elf-tests/runfiles/default.run
+++ b/usr/src/test/elf-tests/runfiles/default.run
@@ -22,6 +22,9 @@ outputdir = /var/tmp/test_results
 [/opt/elf-tests/tests/assert-deflib]
 tests = ['test-deflib']
 
+[/opt/elf-tests/tests/groups/no-relobj-group-merge]
+tests = ['no-relobj-group-merge']
+
 [/opt/elf-tests/tests/linker-sets]
 tests = ['simple', 'in-use-check']
 
diff --git a/usr/src/test/elf-tests/tests/Makefile b/usr/src/test/elf-tests/tests/Makefile
index bc060eb653..74f358d77a 100644
--- a/usr/src/test/elf-tests/tests/Makefile
+++ b/usr/src/test/elf-tests/tests/Makefile
@@ -16,6 +16,7 @@
 
 SUBDIRS =		\
 	assert-deflib	\
+	groups		\
 	linker-sets	\
 	mapfiles	\
 	relocs		\
diff --git a/usr/src/test/elf-tests/tests/groups/Makefile b/usr/src/test/elf-tests/tests/groups/Makefile
new file mode 100644
index 0000000000..5e109be063
--- /dev/null
+++ b/usr/src/test/elf-tests/tests/groups/Makefile
@@ -0,0 +1,19 @@
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+#
+# Copyright (c) 2012, 2016 by Delphix. All rights reserved.
+# Copyright 2018 Joyent, Inc.
+#
+
+SUBDIRS = no-relobj-group-merge
+
+include $(SRC)/test/Makefile.com
diff --git a/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/Makefile b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/Makefile
new file mode 100644
index 0000000000..26e2a4fe9c
--- /dev/null
+++ b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/Makefile
@@ -0,0 +1,49 @@
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+# Copyright 2022, Richard Lowe.
+
+include $(SRC)/cmd/Makefile.cmd
+include $(SRC)/test/Makefile.com
+
+PROG =	no-relobj-group-merge
+
+DATAFILES =	sections.s README.md
+
+ROOTOPTPKG = $(ROOT)/opt/elf-tests
+TESTDIR = $(ROOTOPTPKG)/tests/groups/no-relobj-group-merge
+
+CMDS = $(PROG:%=$(TESTDIR)/%)
+$(CMDS) := FILEMODE = 0555
+
+DATA = $(DATAFILES:%=$(TESTDIR)/%)
+$(DATA) := FILEMODE = 0444
+
+all:
+
+install: all $(CMDS) $(DATA)
+
+clobber: clean
+	-$(RM) $(PROG)
+
+clean:
+	-$(RM) $(CLEANFILES)
+
+$(CMDS): $(TESTDIR)
+
+$(TESTDIR):
+	$(INS.dir)
+
+$(TESTDIR)/%: %.sh
+	$(INS.rename)
+
+$(TESTDIR)/%: %
+	$(INS.file)
diff --git a/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/README.md b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/README.md
new file mode 100644
index 0000000000..40c4832986
--- /dev/null
+++ b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/README.md
@@ -0,0 +1,29 @@
+This is a test that sections which are members of groups are not merged when
+creating relocatable objects.
+
+If we place an input section which is in a group in the same output section as
+another input section this leaves us with problems:
+
+1. If the other input section was not previously member of a group, its data
+   becomes part of a group and we may now discard it along with that group.
+2. If the other input section _was_ a member of a group we now have two groups
+   containing the same section, where discarding one will corrupt the other.
+3. ... and if that section had associated relocations, which must have been
+   part of the group, we will now associate those relocations with the merged
+   output section further corrupting the group, as there is now no mapping
+   between input and output relocation sections.
+
+We test this by defining 3 sections in two groups in two input objects:
+- `.test_data_conflict` in `group1`
+- `.test_data_conflict` in `group2`
+- `.test_data_conflict` in no group at all
+
+We then link these objects together using `ld -r` and expect:
+- `.test_data_conflict` from `group1` remains in `group1`, is merged with no
+  other input section, and has the duplicate section discarded by the COMDAT
+  group logic.
+- `.test_data_conflict` from `group2` remains in `group2`, is merged with no
+  other input section, and has the duplicate section discarded by the COMDAT
+  group logic.
+- the ungrouped `.test_data_conflict` remains in no groups, and the two input
+  sections are merged into a single output section.
diff --git a/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/no-relobj-group-merge.sh b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/no-relobj-group-merge.sh
new file mode 100644
index 0000000000..6cc8c16bf3
--- /dev/null
+++ b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/no-relobj-group-merge.sh
@@ -0,0 +1,127 @@
+#!/bin/ksh
+#
+# This file and its contents are supplied under the terms of the
+# Common Development and Distribution License ("CDDL"), version 1.0.
+# You may only use this file in accordance with the terms of version
+# 1.0 of the CDDL.
+#
+# A full copy of the text of the CDDL should have accompanied this
+# source.  A copy of the CDDL is also available via the Internet at
+# http://www.illumos.org/license/CDDL.
+#
+
+# Copyright 2022, Richard Lowe.
+
+TESTDIR=$(dirname $0)
+
+tmpdir=/tmp/test.$$
+mkdir $tmpdir
+cd $tmpdir
+
+cleanup() {
+	cd /
+	rm -fr $tmpdir
+}
+
+trap 'cleanup' EXIT
+
+if [[ $PWD != $tmpdir ]]; then
+	print -u2 "Failed to create temporary directory: $tmpdir"
+	exit 1;
+fi
+
+if [[ -n $PROTO ]]; then
+	export LD_ALTEXEC=$PROTO/bin/ld
+fi
+
+gas -c ${TESTDIR}/sections.s -o obj1.o
+if (( $? != 0 )); then
+	print -u2 "Couldn't assemble ${TESTDIR}/sections.s (obj1)"
+	exit 1;
+fi
+
+gas -c ${TESTDIR}/sections.s -o obj2.o
+if (( $? != 0 )); then
+	print -u2 "Couldn't assemble ${TESTDIR}/sections.s (obj2)"
+	exit 1;
+fi
+
+/bin/ld -r obj1.o obj2.o -o test-obj.o
+if (( $? != 0 )); then
+	print -u2 "Couldn't link ${TESTDIR}/test-obj.o"
+	exit 1;
+fi
+
+# section_content <index> <file>
+section_content() {
+	elfdump -I$1 -w /dev/stdout $2 | tr '\0' '\n'
+}
+
+# find_in_group <group> <section> <file>
+find_in_group() {
+	elfdump -g $3 | awk -v group="${1}\$" -v section=$2 '
+		BEGIN { slurp = 0 };
+		$0 ~ group { slurp = 1 };
+		slurp && $0 ~ section {
+			gsub(/[\[\]]/, "", $3);
+			print $3;
+			exit;
+		}' | read index
+	if [[ -z $index ]] || (( index <= 0 )); then
+		print -u2 "Couldn't find $2 in $1"
+		exit 1
+	fi
+	print $index;
+}
+
+# The first test_data_conflict, a member of group1 unmerged with only one
+# copy kept.
+GROUP1_INDEX=$(find_in_group group1 test_data_conflict test-obj.o)
+
+# The first test_data_conflict, a member of group2 unmerged with only one
+# copy kept.
+GROUP2_INDEX=$(find_in_group group2 test_data_conflict test-obj.o)
+
+# The un-grouped test_data_conflict, with both copies kept
+elfdump -cN.test_data_conflict test-obj.o | \
+    awk	 -v group1=$GROUP1_INDEX -v group2=$GROUP2_INDEX '
+	/^Section Header/ {
+		gsub(/[^0-9]/, "", $2);
+		if (($2 != group1) && ($2 != group2)) {
+			print $2
+		}
+	}' | read UNGROUP_INDEX
+if [[ -z $UNGROUP_INDEX ]] || (( UNGROUP_INDEX <= 0 )); then
+	print -u2 "Couldn't find ungrouped .test_data_conflict"
+	exit 1
+fi
+
+if (( GROUP1_INDEX == GROUP2_INDEX )); then
+	print -u2 "FAIL: group1 and group2 contain the same section";
+	exit 1
+fi
+
+cmp -s <(section_content $GROUP1_INDEX test-obj.o) /dev/stdin <<EOF
+2: test_data_conflict (group 1)
+EOF
+if (( $? != 0 )); then
+	print -u2 "FAIL: the .test_data_conflict section in group1 has the wrong content"
+	exit 1;
+fi
+
+cmp -s <(section_content $GROUP2_INDEX test-obj.o) /dev/stdin <<EOF
+3: test_data_conflict (group 2)
+EOF
+if (( $? != 0 )); then
+	print -u2 "FAIL: the .test_data_conflict section in group2 has the wrong content"
+	exit 1;
+fi
+
+cmp -s <(section_content $UNGROUP_INDEX test-obj.o) /dev/stdin <<EOF
+4: test_data_conflict (two copies not in group)
+4: test_data_conflict (two copies not in group)
+EOF
+if (( $? != 0 )); then
+	print -u2 "FAIL: the ungrouped .test_data_conflict has the wrong content"
+	exit 1;
+fi
diff --git a/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/sections.s b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/sections.s
new file mode 100644
index 0000000000..3fbb67bcea
--- /dev/null
+++ b/usr/src/test/elf-tests/tests/groups/no-relobj-group-merge/sections.s
@@ -0,0 +1,42 @@
+/*
+ * This file and its contents are supplied under the terms of the
+ * Common Development and Distribution License ("CDDL"), version 1.0.
+ * You may only use this file in accordance with the terms of version
+ * 1.0 of the CDDL.
+ *
+ * A full copy of the text of the CDDL should have accompanied this
+ * source.  A copy of the CDDL is also available via the Internet at
+ * http://www.illumos.org/license/CDDL.
+ */
+
+/* Copyright 2022, Richard Lowe. */
+
+/*
+ * We want to verify that if two sections which otherwise will be merged during
+ * the link-edit are members of groups that they are not merged
+ * but that if they are of the same group they will be discarded.
+ */
+
+	/*
+	 * .test_data_conflict
+	 * A member of group1, one copy will be kept
+	 */
+	.section	.test_data_conflict,"aG",@progbits,group1,comdat
+	.string "2: test_data_conflict (group 1)"
+
+	/*
+	 * .test_data_conflict
+	 * A member of group2.  One copy will be kept, and that copy will _not_
+	 * be merged into a single .test_data_conflict section
+	 */
+	.section	.test_data_conflict,"aG",@progbits,group2,comdat
+	.string "3: test_data_conflict (group 2)"
+
+	/*
+	 * .test_data_conflict
+	 * Not a member of any group.  Both copies will be kept and will be
+	 * merged, but will _not_ be merged into a section that is part of
+	 * a group.
+	 */
+	.section	.test_data_conflict,"a",@progbits
+	.string "4: test_data_conflict (two copies not in group)"