On 15/07/2022 08:07, Fedor Tokarev wrote: > 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. Thanks! I've sent https://lore.kernel.org/bpf/1658734261-4951-1-git-send-email-alan.maguire@xxxxxxxxxx/ If you could give it a try that would be great; tested at my end with your fix and all works well. I'd suggest pulling it into a 2-patch series comprised of your fix + the selftest, but since the fix targets bpf and the tests are new (so would be more like a bpf-next candidate), not sure if that's the right way to handle this.. If not I can follow up with the test once the fix lands. Anyway thanks again for finding and fixing this! Alan > >> 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 >>>>>
