From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: make stacktrace_save_utrace() nicer to btrace(8)
To: tech@openbsd.org
Date: Sat, 2 Aug 2025 23:59:51 +0200

Hello,

this diff resurrects idea from diff I've sent few weeks ago [1].
kettenis@ was not excited about that change back then.

I've sent another version earlier [2] today which was still kind
of disappointing to kettenis@. This time we had a chance to
discuss those changes at the table.

The outcome was that the change [3] which avoids the crash 
went in [4]. The remaining part which I'd like to get it in
is presented here.

the change builds an assumption into stacktrace_save_utrace()
function that if particular stack frame contains an invalid
address to next frame (,f_frame is bogus address), then
the return address kept in frame is also invalid.

stop reading here, if your are not interested in details.

the reason why I don't want to pass potentially invalid
instruction address to btrace(8) is the cost the btrace
process needs to pay to figure out the address does not
point to instruction in traced process.

btrace process tries to resolve address to symbol. if
it finds out there is no matching symbol for presented address,
it uses ioctl(2) on dt(4) to query kernel which .so object
the instruction belongs to. If instruction belongs to so .so
in traced process, the kernel opens a file descriptor in
btrace file descriptor table and associates a vnode which
refers to .so with it. It then passes the file descriptor
number back to btrace process, so btrace can use mmap(2)
and libelf to load symbol table.

Works nicely as long as btrace process is presented with
valid addresses. However if traced process happens not
to use frame pointers correctly then stacktrace_save_utrace()
function may misinterpret frame and return invalid address
to btrace. btrace then does ioctl(2) just to find out
the address is invalid.

after a brief discussion kettenis@ agrees current simple
approach proposed here seems to be good enough for now.
the alternative option would be to implement a negative
caching in btrace. the btrace could remember addresses
(page ranges) the dt(5) failed to resolve to symbol tables.
This is something that can be done later.

diff follows.

thanks and
regards
sashan

[1] https://marc.info/?l=openbsd-tech&m=174938917209440&w=2

[2] https://marc.info/?l=openbsd-tech&m=175414134428800&w=2

[3] https://marc.info/?l=openbsd-tech&m=175415293201212&w=2

[4] https://marc.info/?l=openbsd-cvs&m=175416247805522&w=2

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
index 5279024e04d..c4499042036 100644
--- a/sys/arch/amd64/amd64/db_trace.c
+++ b/sys/arch/amd64/amd64/db_trace.c
@@ -319,8 +319,19 @@ stacktrace_save_utrace(struct stacktrace *st)
 		st->st_pc[st->st_count++] = lastframe->f_retaddr;
 
 	while (frame != NULL && st->st_count < STACKTRACE_MAX) {
-		if (copyin(frame, &f, sizeof(f)) != 0)
+		if (copyin(frame, &f, sizeof(f)) != 0) {
+			/*
+			 * We assume the return address from previous frame is
+			 * invalid, because the frame refers to invalid
+			 * address. We don't want to provide invalid address
+			 * to btrace(8) in userland.
+			 */
+			if (st->st_count == 0)
+				st->st_pc[0] = 0;
+			else
+				st->st_count--;
 			break;
+		}
 		st->st_pc[st->st_count++] = f.f_retaddr;
 		frame = f.f_frame;
 	}
diff --git a/sys/arch/i386/i386/db_trace.c b/sys/arch/i386/i386/db_trace.c
index 140ba9eca33..086afb11a5a 100644
--- a/sys/arch/i386/i386/db_trace.c
+++ b/sys/arch/i386/i386/db_trace.c
@@ -318,8 +318,19 @@ stacktrace_save_utrace(struct stacktrace *st)
 		st->st_pc[st->st_count++] = lastframe->f_retaddr;
 
 	while (frame != NULL && st->st_count < STACKTRACE_MAX) {
-		if (copyin(frame, &f, sizeof(f)) != 0)
+		if (copyin(frame, &f, sizeof(f)) != 0) {
+			/*
+			 * We assume the return address from previous frame is
+			 * invalid, because the frame refers to invalid
+			 * address. We don't want to provide invalid address
+			 * to btrace(8) in userland.
+			 */
+			if (st->st_count == 0)
+				st->st_pc[0] = 0;
+			else
+				st->st_count--;
 			break;
+		}
 		st->st_pc[st->st_count++] = f.f_retaddr;
 		frame = f.f_frame;
 	}