[Ltrace-devel] ./ltrace: proc.c: 755: breakpoint_for_symbol: Assertion `bp->libsym == ((void *)0)' failed.

[Edgar E. Iglesias]

On Thu, Nov 08, 2012 at 03:53:26PM +0100, Sedat Dilek wrote:
> On Fri, Nov 2, 2012 at 11:58 PM, Petr Machata <pmachata at redhat.com> wrote:
> > "Edgar E. Iglesias" <edgar.iglesias at gmail.com> writes:
> >
> >> On Fri, Nov 02, 2012 at 06:07:50PM +0100, Sedat Dilek wrote:
> >>> This happens with latest ltrace-git...
> >>>
> >>> ./ltrace: proc.c: 755: breakpoint_for_symbol: Assertion `bp->libsym ==
> >>> ((void *)0)' failed.
> >>>
> >> I've seen this bug, and I'm actually inclined to think that the assert
> >> should go away and that we instead should somehow just not use that
> >> break point. But not sure, maybe Petr has more input on this.
> >
> > If you assign more than one symbol to a breakpoint, and that breakpoint
> > hits, there's no way to tell which of the symbols actually hit.  So
> > there's no point storing more than one symbol to a breakpoint.  We could
> > still drop the assert and just ignore the symbol, as you write.
> >
> > What might make sense is having several breakpoint handlers per
> > breakpoint, so that ltrace allows several service breakpoints per
> > address.  Software singlestepping in particular should work well even if
> > it steps over another breakpoint, service or not.  (As should normal
> > singlestepping, and I'm not positive that it does, but that's a
> > different can of worms.)
> >
> >> The issue I've seen is with weak symbols that are made to point to
> >> other symbols. I think, I saw it with strlen IIRC. In our glibc,
> >> strlen points resolves to the same addr as __strlen but it's got
> >> it's own PLT entry and symbol.
> >
> > Hmm, how about having two PLT slots, each for a different symbol, but
> > those symbols are aliases in the target library.  Ltrace filters
> > aliases, but only for purposes of -x.  For -e there's no way to tell
> > that they end up being aliases.  And normally you don't care anyway, but
> > on MIPS breakpoints are put not to PLT slots, but to destination
> > addresses.  That's how you end up needing two breakpoints on the same
> > address.  Could it be the cause?
> >
> > If yes, then the reproducer is as simple as -e func -x func.
> >
> >> I've got a local patch for it, but I unfortunately I haven't had much
> >> time lately to look at it further. My plan was to create a test case
> >> that hopefully can trig the issue on x86 aswell. Then we can work out
> >> a solution from there.
> >
> > I guess on x86, one could trigger the same by IFUNC symbols, but those
> > will need to be supported at all in the first place.
> >
> 
> Hi Edgar,
> 
> Any news or even patch(es) around for this issue?
> 
> I re-tested with ltrace-0.7.0-git1584cfc again to see if any new
> changes did or did not break build on MIPSEL (own toolchain built with
> Freetz build-system).
> Building is fine, but the reported issue still remains.
> 
> I can only help with more testing these days...


Hi Sedak,

This was the latest patch in my tree when I had to stop. I need to go
through this before something can be submitted. But if you want to try it
out, that would help.

Thanks,
Edgar

diff --git a/libltrace.c b/libltrace.c
index f4bb2c8..9a6056d 100644
--- a/libltrace.c
+++ b/libltrace.c
@@ -103,6 +103,7 @@ ltrace_init(int argc, char **argv) {
 	struct opt_p_t *opt_p_tmp;
 
 	atexit(normal_exit);
+	signal(SIGPIPE, signal_exit);
 	signal(SIGINT, signal_exit);	/* Detach processes when interrupted */
 	signal(SIGTERM, signal_exit);	/*  ... or killed */
 
diff --git a/proc.c b/proc.c
index 3dab1e2..08cd3c2 100644
--- a/proc.c
+++ b/proc.c
@@ -656,8 +659,13 @@ breakpoint_for_symbol(struct library_symbol *libsym, struct Process *proc)
 	struct breakpoint *bp = dict_find_entry(proc->breakpoints,
 						bp_addr);
 	if (bp != NULL) {
-		assert(bp->libsym == NULL);
-		bp->libsym = libsym;
+		fprintf(stderr, "%s: bp_addr=%p bp->libsym=%p libsym=%p %s %s\n",
+			__func__, bp_addr, bp->libsym, libsym,
+			bp->libsym->name, libsym->name);
+		fflush(NULL);
+//		assert(bp->libsym == NULL || bp->libsym == libsym);
+//		bp->libsym = libsym;
+		libsym->delayed = 1;
 		return 0;
 	}
 
diff --git a/sysdeps/linux-gnu/mipsel/plt.c b/sysdeps/linux-gnu/mipsel/plt.c
index 7799dfa..7bb00e0 100644
--- a/sysdeps/linux-gnu/mipsel/plt.c
+++ b/sysdeps/linux-gnu/mipsel/plt.c
@@ -251,6 +251,13 @@ void arch_symbol_ret(struct Process *proc, struct library_symbol *libsym)
 		return;
 	}
 
+	/* For multiple symbols that resolve to the same address, we need
+	 * to avoid adding multiple breakpoints.  */
+	if (dict_find_entry(proc->leader->breakpoints, resolved_addr) != NULL) {
+		fprintf(stderr, "%s: skip %p %s\n", __func__, resolved_addr, libsym->name);
+		return;
+	}
+
 	bp = malloc(sizeof (*bp));
 	if (bp == NULL) {
 		fprintf(stderr, "Failed to allocate bp for %s\n",
@@ -361,8 +368,23 @@ arch_elf_add_plt_entry(struct Process *proc, struct ltelf *lte,
 		/* Delay breakpoint activation until the symbol gets
 		 * resolved.  */
 		libsym->delayed = 1;
-	} else if (mips_elf_is_cpic(lte->ehdr.e_flags)) {
-		libsym->arch.pltalways = 1;
+	} else {
+		if (mips_elf_is_cpic(lte->ehdr.e_flags)) {
+			libsym->arch.pltalways = 1;
+		}
+
+		/* For multiple symbols that resolve to the same address,
+		 * we need to avoid adding multiple breakpoints.  */
+		if (dict_find_entry(proc->leader->breakpoints,
+				    bp_addr) != NULL) {
+			fprintf(stderr, "%s: dup %p %s\n", __func__, bp_addr, libsym->name);
+			libsym->delayed = 1;
+		}
+	}
+
+	if (strstr(name, "strdup") != NULL) {
+		fprintf(stderr, "%s: %s %p idx=%d\n",
+			__func__, name, bp_addr, ndx);
 	}
 
 	*ret = libsym;