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@xxxxxxxxx> 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@xxxxxxxxxx>
>
> >>
> >> jirka
> >>
> >>>
> >>> Signed-off-by: Fedor Tokarev <ftokarev@xxxxxxxxx>
> >>> ---
> >>> 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
> >>>
