Re: [PATCH] bpf: btf: Fix vsnprintf return value check

From: Fedor Tokarev <ftokarev@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Jiri Olsa <olsajiri@gmail.com>,
	Fedor Tokarev <ftokarev@gmail.com>,
	Martin KaFai Lau <martin.lau@linux.dev>,
	bpf <bpf@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] bpf: btf: Fix vsnprintf return value check
Date: Fri, 15 Jul 2022 09:07:42 +0200	[thread overview]
Message-ID: <20220715070742.GA165641@laptop> (raw)
In-Reply-To: <ef44abe0-81fc-9c97-a4e4-2b3ba19cd84f@oracle.com>

On Thu, Jul 14, 2022 at 11:06:22AM +0100, Alan Maguire wrote:
> On 13/07/2022 19:40, Andrii Nakryiko wrote:
> > On Mon, Jul 11, 2022 at 2:45 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >>
> >> On Mon, Jul 11, 2022 at 11:13:17PM +0200, Fedor Tokarev wrote:
> >>> vsnprintf returns the number of characters which would have been written if
> >>> enough space had been available, excluding the terminating null byte. Thus,
> >>> the return value of 'len_left' means that the last character has been
> >>> dropped.
> >>
> >> should we have test for this in progs/test_snprintf.c ?
> > 
> > It might be too annoying to set up such test, and given the fix is
> > pretty trivial IMO it's ok without extra test. But cc Alan for ack.
> > Alan, please take a look as well.
> > 
> 
> I can follow up with a test, it should be okay I think (we can use
> the "don't show types" flag and tryp to print "10" with a 2-byte len or
> similar).

I'll gladly give it a try.

> In terms of the fix, it looks good, but given that the code is tricky, 
> it might be good to expand a bit on the explanation. Something like the below?
> 
Agreed.

> "When using btf_type_snprintf_show(), the user passes in a "len" value, and
> we use it to initialize ssnprintf.len_left, indicating how much space
> remains for the string representation, including the null byte, so "len - 1" 
> bytes are actually available for the actual string data, leaving one for 
> the terminating null byte.
> 
> In btf_snprintf_show() - which is passed the ssnprintf data as an argument -
> vsnprintf() returns the len that would have been written, and this _excludes_ 
> the null terminator. But we want to handle cases where the length of the string
> to be written (excluding the null terminator) exactly matches the original len 
> value we passed in (len == len_left) in the same way was we do other
> overflow cases (len > len_left)."
> 
> Acked-by: Alan Maguire <alan.maguire@oracle.com>
> 
> >>
> >> jirka
> >>
> >>>
> >>> Signed-off-by: Fedor Tokarev <ftokarev@gmail.com>
> >>> ---
> >>>  kernel/bpf/btf.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> >>> index eb12d4f705cc..a9c1c98017d4 100644
> >>> --- a/kernel/bpf/btf.c
> >>> +++ b/kernel/bpf/btf.c
> >>> @@ -6519,7 +6519,7 @@ static void btf_snprintf_show(struct btf_show *show, const char *fmt,
> >>>       if (len < 0) {
> >>>               ssnprintf->len_left = 0;
> >>>               ssnprintf->len = len;
> >>> -     } else if (len > ssnprintf->len_left) {
> >>> +     } else if (len >= ssnprintf->len_left) {
> >>>               /* no space, drive on to get length we would have written */
> >>>               ssnprintf->len_left = 0;
> >>>               ssnprintf->len += len;
> >>> --
> >>> 2.25.1
> >>>