The assertion failure is the result of a double free in
dt_link.c:process_obj, and it's basically happening because libdtrace is
dependant on some internal behaviour of Solaris' libelf.

In the block between lines 1358 and 1415, some buffers are injected into
a couple of libelf structures (data_str and data_sym). On Solaris, it
looks like the overwritten pointers are just pointers into a larger
buffer; on FreeBSD, they're allocated on their own and then freed in
elf_close(3). Thus the injected buffers get freed by both libdtrace and
libelf.

I think the right fix is to just #ifdef the free()s out. One could
implement a portable fix by saving pointers to the original buffers
somewhere (probably in the struct dt_link_pair) and restore them before
calling elf_close(). But I don't think fixes to FreeBSD's dtrace port
are going to go back upstream anyway, so I've attached a patch with the
simple fix.

To reproduce this on a recent CURRENT, it'll be necessary to follow one
of the workarounds described in bin/171678 first.

Now, in the case of perl, we immediately run into another problem after
my patch. The build then fails with:

	dtrace: failed to link script perldtrace.d: an error was
	encountered while processing pp_ctl.o

It looks like this is caused by an unrelated dependance on Solaris
libelf's behaviour, but I haven't pinned it down yet.

Thanks,
-Mark

I've spent some more time digging into this and found a simple way to
reproduce it. The problem only seems to occur when a probe is located in
a static function. Some comments in dt_link.c indicate that there's some
special handling that's needed in this case, but I still don't quite
understand what's causing the problem.

I've placed a simple provider definition and sample program that
reproduce the issue here:
http://people.freebsd.org/~markj/dtrace/bin167103/

When the repro program is built, dtrace(1) will segfault when processing
the object file. When the patch attached to the PR is applied, dtrace
will just exit with a generic error:

dtrace -G -s provider.d repro.o
provider: failed to link script provider: an error was encountered while processing repro.o
*** [beforelinking] Error code 1

I'm still working on figuring this out.

Thanks,
-Mark

Comment 3 Mark Johnston 2013-01-10 22:45:48 UTC

State Changed
From-To: open->analyzed

I understand the problems now and am working on some patches. The second 
problem described in my replies is due to a bug in libelf which has been 
fixed upstream (in elftoolchain). 

Comment 4 Mark Johnston 2013-01-10 22:45:48 UTC

Responsible Changed
From-To: freebsd-bugs->markj

I understand the problems now and am working on some patches. The second 
problem described in my replies is due to a bug in libelf which has been 
fixed upstream (in elftoolchain).

I've managed to come up with proper fixes for the problems here. With
them, the dtrace step completes properly, but the perl build fails later
on for an unrelated reason which I haven't looked into. However, my
little test program (which just has a probe in a static function) works
properly now.

I've attached two patches. The first one changes libdtrace to do the
work in process_obj() using offical libelf APIs; it supersedes the
earlier patch attached to this PR. Currently, dtrace adds new symbol
table entries by copying the old symbol table into a larger buffer,
adding symbols at the end, and reinserting it into a libelf data
structure. This approach works, but it's not clear who's responsible for
freeing the new buffer. This patch changes things so that dtrace
allocates a new data descriptor for the symbol table and adds new
symbols there. It's a bit more code than the previous patch, but it has
the benefit of not depending on libelf internals.

The second patch is a partial merge of r1712 from elftoolchain. Without
this, libelf doesn't seem to handle section size changes when
recomputing the section extents and elf_update() will fail. It looks
like FreeBSD's libelf hasn't been kept in sync with upstream libelf; I'm
not sure why that is.

Thanks,
-Mark

Comment 6 dfilter service 2013-02-24 15:15:59 UTC

Author: markj
Date: Sun Feb 24 15:15:50 2013
New Revision: 247221
URL: http://svnweb.freebsd.org/changeset/base/247221

Log:
  Merge part of r1712 from elftoolchain, making it possible to resize ELF
  sections and indirectly change the layout of an ELF file when
  ELF_F_LAYOUT is not set.
  
  PR:		bin/167103
  Approved by:	rstone (co-mentor)
  Obtained from:	elftoolchain
  MFC after:	2 weeks

Modified:
  head/lib/libelf/elf_update.c

Modified: head/lib/libelf/elf_update.c
==============================================================================
--- head/lib/libelf/elf_update.c	Sun Feb 24 11:32:45 2013	(r247220)
+++ head/lib/libelf/elf_update.c	Sun Feb 24 15:15:50 2013	(r247221)
@@ -41,89 +41,79 @@ __FBSDID("$FreeBSD$");
 #include "_libelf.h"
 
 /*
- * Update the internal data structures associated with an ELF object.
- * Returns the size in bytes the ELF object would occupy in its file
- * representation.
+ * Layout strategy:
  *
- * After a successful call to this function, the following structures
- * are updated:
+ * - Case 1: ELF_F_LAYOUT is asserted
+ *     In this case the application has full control over where the
+ *     section header table, program header table, and section data
+ *     will reside.   The library only perform error checks.
  *
- * - The ELF header is updated.
- * - All sections are sorted in order of ascending addresses and their
- *   section header table entries updated.   An error is signalled
- *   if an overlap was detected among sections.
- * - All data descriptors associated with a section are sorted in order
- *   of ascending addresses.  Overlaps, if detected, are signalled as
- *   errors.  Other sanity checks for alignments, section types etc. are
- *   made.
+ * - Case 2: ELF_F_LAYOUT is not asserted
  *
- * After a resync_elf() successfully returns, the ELF descriptor is
- * ready for being handed over to _libelf_write_elf().
+ *     The library will do the object layout using the following
+ *     ordering:
+ *     - The executable header is placed first, are required by the
+ *     	 ELF specification.
+ *     - The program header table is placed immediately following the
+ *       executable header.
+ *     - Section data, if any, is placed after the program header
+ *       table, aligned appropriately.
+ *     - The section header table, if needed, is placed last.
  *
- * File alignments:
- * PHDR - Addr
- * SHDR - Addr
+ *     There are two sub-cases to be taken care of:
  *
- * XXX: how do we handle 'flags'.
+ *     - Case 2a: e->e_cmd == ELF_C_READ or ELF_C_RDWR
+ *
+ *       In this sub-case, the underlying ELF object may already have
+ *       content in it, which the application may have modified.  The
+ *       library will retrieve content from the existing object as
+ *       needed.
+ *
+ *     - Case 2b: e->e_cmd == ELF_C_WRITE
+ *
+ *       The ELF object is being created afresh in this sub-case;
+ *       there is no pre-existing content in the underlying ELF
+ *       object.
  */
 
 /*
  * Compute the extents of a section, by looking at the data
- * descriptors associated with it.  The function returns zero if an
- * error was detected.  `*rc' holds the maximum file extent seen so
- * far.
+ * descriptors associated with it.  The function returns 1 if
+ * successful, or zero if an error was detected.
  */
 static int
-_libelf_compute_section_extents(Elf *e, Elf_Scn *s, off_t *rc)
+_libelf_compute_section_extents(Elf *e, Elf_Scn *s, off_t rc)
 {
 	int ec;
-	Elf_Data *d, *td;
+	size_t fsz, msz;
+	Elf_Data *d;
+	Elf32_Shdr *shdr32;
+	Elf64_Shdr *shdr64;
 	unsigned int elftype;
 	uint32_t sh_type;
 	uint64_t d_align;
 	uint64_t sh_align, sh_entsize, sh_offset, sh_size;
 	uint64_t scn_size, scn_alignment;
 
-	/*
-	 * We need to recompute library private data structures if one
-	 * or more of the following is true:
-	 * - The underlying Shdr structure has been marked `dirty'.  Significant
-	 *   fields include: `sh_offset', `sh_type', `sh_size', `sh_addralign'.
-	 * - The Elf_Data structures part of this section have been marked
-	 *   `dirty'.  Affected members include `d_align', `d_offset', `d_type',
-	 *   and `d_size'.
-	 * - The section as a whole is `dirty', e.g., it has been allocated
-	 *   using elf_newscn(), or if a new Elf_Data structure was added using
-	 *   elf_newdata().
-	 *
-	 * Each of these conditions would result in the ELF_F_DIRTY bit being
-	 * set on the section descriptor's `s_flags' field.
-	 */
-
 	ec = e->e_class;
 
+	shdr32 = &s->s_shdr.s_shdr32;
+	shdr64 = &s->s_shdr.s_shdr64;
 	if (ec == ELFCLASS32) {
-		sh_type    = s->s_shdr.s_shdr32.sh_type;
-		sh_align   = (uint64_t) s->s_shdr.s_shdr32.sh_addralign;
-		sh_entsize = (uint64_t) s->s_shdr.s_shdr32.sh_entsize;
-		sh_offset  = (uint64_t) s->s_shdr.s_shdr32.sh_offset;
-		sh_size    = (uint64_t) s->s_shdr.s_shdr32.sh_size;
+		sh_type    = shdr32->sh_type;
+		sh_align   = (uint64_t) shdr32->sh_addralign;
+		sh_entsize = (uint64_t) shdr32->sh_entsize;
+		sh_offset  = (uint64_t) shdr32->sh_offset;
+		sh_size    = (uint64_t) shdr32->sh_size;
 	} else {
-		sh_type    = s->s_shdr.s_shdr64.sh_type;
-		sh_align   = s->s_shdr.s_shdr64.sh_addralign;
-		sh_entsize = s->s_shdr.s_shdr64.sh_entsize;
-		sh_offset  = s->s_shdr.s_shdr64.sh_offset;
-		sh_size    = s->s_shdr.s_shdr64.sh_size;
+		sh_type    = shdr64->sh_type;
+		sh_align   = shdr64->sh_addralign;
+		sh_entsize = shdr64->sh_entsize;
+		sh_offset  = shdr64->sh_offset;
+		sh_size    = shdr64->sh_size;
 	}
 
-	if (sh_type == SHT_NULL || sh_type == SHT_NOBITS)
-		return (1);
-
-	if ((s->s_flags & ELF_F_DIRTY) == 0) {
-		if ((size_t) *rc < sh_offset + sh_size)
-			*rc = sh_offset + sh_size;
-		return (1);
-	}
+	assert(sh_type != SHT_NULL && sh_type != SHT_NOBITS);
 
 	elftype = _libelf_xlate_shtype(sh_type);
 	if (elftype > ELF_T_LAST) {
@@ -131,15 +121,52 @@ _libelf_compute_section_extents(Elf *e, 
 		return (0);
 	}
 
-	/*
-	 * Compute the extent of the data descriptors associated with
-	 * this section.
-	 */
-	scn_alignment = 0;
 	if (sh_align == 0)
 		sh_align = _libelf_falign(elftype, ec);
 
-	/* Compute the section alignment. */
+	/*
+	 * Check the section's data buffers for sanity and compute the
+	 * section's alignment.
+	 * Compute the section's size and alignment using the data
+	 * descriptors associated with the section.
+	 */
+	if (STAILQ_EMPTY(&s->s_data)) {
+		/*
+		 * The section's content (if any) has not been read in
+		 * yet.  If section is not dirty marked dirty, we can
+		 * reuse the values in the 'sh_size' and 'sh_offset'
+		 * fields of the section header.
+		 */
+		if ((s->s_flags & ELF_F_DIRTY) == 0) {
+			/*
+			 * If the library is doing the layout, then we
+			 * compute the new start offset for the
+			 * section based on the current offset and the
+			 * section's alignment needs.
+			 *
+			 * If the application is doing the layout, we
+			 * can use the value in the 'sh_offset' field
+			 * in the section header directly.
+			 */
+			if (e->e_flags & ELF_F_LAYOUT)
+				goto updatedescriptor;
+			else
+				goto computeoffset;
+		}
+
+		/*
+		 * Otherwise, we need to bring in the section's data
+		 * from the underlying ELF object.
+		 */
+		if (e->e_cmd != ELF_C_WRITE && elf_getdata(s, NULL) == NULL)
+			return (0);
+	}
+
+	/*
+	 * Loop through the section's data descriptors.
+	 */
+	scn_size = 0L;
+	scn_alignment = 0L;
 	STAILQ_FOREACH(d, &s->s_data, d_next)  {
 		if (d->d_type > ELF_T_LAST) {
 			LIBELF_SET_ERROR(DATA, 0);
@@ -153,23 +180,40 @@ _libelf_compute_section_extents(Elf *e, 
 			LIBELF_SET_ERROR(DATA, 0);
 			return (0);
 		}
-		if (d_align > scn_alignment)
-			scn_alignment = d_align;
-	}
 
-	scn_size = 0L;
+		/*
+		 * The buffer's size should be a multiple of the
+		 * memory size of the underlying type.
+		 */
+		msz = _libelf_msize(d->d_type, ec, e->e_version);
+		if (d->d_size % msz) {
+			LIBELF_SET_ERROR(DATA, 0);
+			return (0);
+		}
 
-	STAILQ_FOREACH_SAFE(d, &s->s_data, d_next, td) {
+		/*
+		 * Compute the section's size.
+		 */
 		if (e->e_flags & ELF_F_LAYOUT) {
 			if ((uint64_t) d->d_off + d->d_size > scn_size)
 				scn_size = d->d_off + d->d_size;
 		} else {
 			scn_size = roundup2(scn_size, d->d_align);
 			d->d_off = scn_size;
-			scn_size += d->d_size;
+			fsz = _libelf_fsize(d->d_type, ec, d->d_version,
+			    d->d_size / msz);
+			scn_size += fsz;
 		}
+
+		/*
+		 * The section's alignment is the maximum alignment
+		 * needed for its data buffers.
+		 */
+		if (d_align > scn_alignment)
+			scn_alignment = d_align;
 	}
 
+
 	/*
 	 * If the application is requesting full control over the layout
 	 * of the section, check its values for sanity.
@@ -180,46 +224,60 @@ _libelf_compute_section_extents(Elf *e, 
 			LIBELF_SET_ERROR(LAYOUT, 0);
 			return (0);
 		}
-	} else {
-		/*
-		 * Otherwise compute the values in the section header.
-		 */
+		goto updatedescriptor;
+	}
 
-		if (scn_alignment > sh_align)
-			sh_align = scn_alignment;
+	/*
+	 * Otherwise compute the values in the section header.
+	 *
+	 * The section alignment is the maximum alignment for any of
+	 * its contained data descriptors.
+	 */
+	if (scn_alignment > sh_align)
+		sh_align = scn_alignment;
 
-		/*
-		 * If the section entry size is zero, try and fill in an
-		 * appropriate entry size.  Per the elf(5) manual page
-		 * sections without fixed-size entries should have their
-		 * 'sh_entsize' field set to zero.
-		 */
-		if (sh_entsize == 0 &&
-		    (sh_entsize = _libelf_fsize(elftype, ec, e->e_version,
-		    (size_t) 1)) == 1)
-			sh_entsize = 0;
+	/*
+	 * If the section entry size is zero, try and fill in an
+	 * appropriate entry size.  Per the elf(5) manual page
+	 * sections without fixed-size entries should have their
+	 * 'sh_entsize' field set to zero.
+	 */
+	if (sh_entsize == 0 &&
+	    (sh_entsize = _libelf_fsize(elftype, ec, e->e_version,
+	    (size_t) 1)) == 1)
+		sh_entsize = 0;
 
-		sh_size = scn_size;
-		sh_offset = roundup(*rc, sh_align);
+	sh_size = scn_size;
 
-		if (ec == ELFCLASS32) {
-			s->s_shdr.s_shdr32.sh_addralign = (uint32_t) sh_align;
-			s->s_shdr.s_shdr32.sh_entsize   = (uint32_t) sh_entsize;
-			s->s_shdr.s_shdr32.sh_offset    = (uint32_t) sh_offset;
-			s->s_shdr.s_shdr32.sh_size      = (uint32_t) sh_size;
-		} else {
-			s->s_shdr.s_shdr64.sh_addralign = sh_align;
-			s->s_shdr.s_shdr64.sh_entsize   = sh_entsize;
-			s->s_shdr.s_shdr64.sh_offset    = sh_offset;
-			s->s_shdr.s_shdr64.sh_size      = sh_size;
-		}
-	}
+computeoffset:
+	/*
+	 * Compute the new offset for the section based on
+	 * the section's alignment needs.
+	 */
+	sh_offset = roundup(rc, sh_align);
 
-	if ((size_t) *rc < sh_offset + sh_size)
-		*rc = sh_offset + sh_size;
+	/*
+	 * Update the section header.
+	 */
+	if (ec == ELFCLASS32) {
+		shdr32->sh_addralign = (uint32_t) sh_align;
+		shdr32->sh_entsize   = (uint32_t) sh_entsize;
+		shdr32->sh_offset    = (uint32_t) sh_offset;
+		shdr32->sh_size      = (uint32_t) sh_size;
+	} else {
+		shdr64->sh_addralign = sh_align;
+		shdr64->sh_entsize   = sh_entsize;
+		shdr64->sh_offset    = sh_offset;
+		shdr64->sh_size      = sh_size;
+	}
 
+updatedescriptor:
+	/*
+	 * Update the section descriptor.
+	 */
 	s->s_size = sh_size;
 	s->s_offset = sh_offset;
+
 	return (1);
 }
 
@@ -267,13 +325,16 @@ _libelf_insert_section(Elf *e, Elf_Scn *
 	return (1);
 }
 
+/*
+ * Recompute section layout.
+ */
+
 static off_t
 _libelf_resync_sections(Elf *e, off_t rc)
 {
 	int ec;
-	off_t nrc;
+	Elf_Scn *s;
 	size_t sh_type, shdr_start, shdr_end;
-	Elf_Scn *s, *ts;
 
 	ec = e->e_class;
 
@@ -281,13 +342,7 @@ _libelf_resync_sections(Elf *e, off_t rc
 	 * Make a pass through sections, computing the extent of each
 	 * section. Order in increasing order of addresses.
 	 */
-
-	nrc = rc;
-	STAILQ_FOREACH(s, &e->e_u.e_elf.e_scn, s_next)
-		if (_libelf_compute_section_extents(e, s, &nrc) == 0)
-			return ((off_t) -1);
-
-	STAILQ_FOREACH_SAFE(s, &e->e_u.e_elf.e_scn, s_next, ts) {
+	STAILQ_FOREACH(s, &e->e_u.e_elf.e_scn, s_next) {
 		if (ec == ELFCLASS32)
 			sh_type = s->s_shdr.s_shdr32.sh_type;
 		else
@@ -296,21 +351,22 @@ _libelf_resync_sections(Elf *e, off_t rc
 		if (sh_type == SHT_NOBITS || sh_type == SHT_NULL)
 			continue;
 
-		if (s->s_offset < (uint64_t) rc) {
-			if (s->s_offset + s->s_size < (uint64_t) rc) {
-				/*
-				 * Try insert this section in the
-				 * correct place in the list,
-				 * detecting overlaps if any.
-				 */
-				STAILQ_REMOVE(&e->e_u.e_elf.e_scn, s, _Elf_Scn,
-				    s_next);
-				if (_libelf_insert_section(e, s) == 0)
-					return ((off_t) -1);
-			} else {
-				LIBELF_SET_ERROR(LAYOUT, 0);
+		if (_libelf_compute_section_extents(e, s, rc) == 0)
+			return ((off_t) -1);
+
+		if (s->s_size == 0)
+			continue;
+
+		if (s->s_offset + s->s_size < (size_t) rc) {
+			/*
+			 * Try insert this section in the
+			 * correct place in the list,
+			 * detecting overlaps if any.
+			 */
+			STAILQ_REMOVE(&e->e_u.e_elf.e_scn, s, _Elf_Scn,
+			    s_next);
+			if (_libelf_insert_section(e, s) == 0)
 				return ((off_t) -1);
-			}
 		} else
 			rc = s->s_offset + s->s_size;
 	}
@@ -338,8 +394,6 @@ _libelf_resync_sections(Elf *e, off_t rc
 		}
 	}
 
-	assert(nrc == rc);
-
 	return (rc);
 }
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 7 Mark Linimon 2018-02-19 01:40:47 UTC

markj/jbeich: is there anything left that needs to be done here?

Comment 8 Mark Johnston 2018-03-18 17:10:55 UTC

(In reply to Mark Linimon from comment #7)
I don't think so. I've fixed the relevant issues in elftoolchain and libdtrace in r247221 and r313263.