From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: follow up on 'stacktrace_save_utrace() should be more robust'
To: Alexandr Nedvedicky <sashan@fastmail.net>
Cc: tech@openbsd.org, tedu@tedunangst.com
Date: Sun, 08 Jun 2025 15:25:02 +0200

> Date: Sun, 8 Jun 2025 15:10:52 +0200
> From: Alexandr Nedvedicky <sashan@fastmail.net>
> 
> Hello,
> 
> this is a finishing touch to change which got committed two weeks ago [1].
> The current code copies invalid stack frame. This tweak just discards it.
> 
> OK to commit?

I don't think this is right.  It is perfectly possible to have a stack
frame with a valid return address but a saved %rbp or %ebp that isn't
a valid frame pointer because the compiler decided it didn't need one.

I think your userland tooling needs to be adjusted to handle this.

> --------8<---------------8<---------------8<------------------8<--------
> 
> diff --git a/sys/arch/amd64/amd64/db_trace.c b/sys/arch/amd64/amd64/db_trace.c
> index 624d7ed1a73..79dcf745a21 100644
> --- a/sys/arch/amd64/amd64/db_trace.c
> +++ b/sys/arch/amd64/amd64/db_trace.c
> @@ -318,8 +318,14 @@ 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) {
> +			/*
> +			 * the last return address (if saved) is invalid,
> +			 * strip it
> +			 */
> +			st->st_count -= (st->st_count == 0) ? 0 : 1;
>  			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 b7c8b68711a..6d988dc4d57 100644
> --- a/sys/arch/i386/i386/db_trace.c
> +++ b/sys/arch/i386/i386/db_trace.c
> @@ -317,8 +317,14 @@ 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) {
> +			/*
> +			 * the last return address (if saved) is invalid,
> +			 * strip it
> +			 */
> +			st->st_count -= (st->st_count == 0) ? 0 : 1;
>  			break;
> +		}
>  		st->st_pc[st->st_count++] = f.f_retaddr;
>  		frame = f.f_frame;
>  	}
> [1] https://marc.info/?l=openbsd-cvs&m=174796965116589&w=2
> 
>