Re: [PATCH] printk: Do not lose last line in kmsg dump

[Steven Rostedt]

On Wed, 10 Jul 2019 14:10:49 +0200
Petr Mladek <pmladek@xxxxxxxx> wrote:

> On Wed 2019-07-10 17:19:22, Sergey Senozhatsky wrote:
 
> > > arch/powerpc/xmon/xmon.c
> > > 2836:	while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf), &len)) {
> > > 2837-		buf[len] = '\0';
> > > 
> > > arch/um/kernel/kmsg_dump.c
> > > 29:	while (kmsg_dump_get_line(dumper, true, line, sizeof(line), &len)) {
> > > 30-		line[len] = '\0';
> > > 
> > > I guess we should fix these first and leave this patch as is?  
> > 
> > We certainly need to fix something here, and I'd say that we
> > better handle it on the msg_print_text() side. There might be
> > more kmsg_dump_get_line() users doing `buf[len] = '\0'' in the
> > future.  

So basically the issues is that callers may expect that the return size
is still in the buffer and they append a '\0' to it. This is the same
issue with strncpy() and will cause the same kinds of bugs.

> 
> I though more about it and I agree with Sergey. One unused byte does
> not look worth the risk. Especially when we are talking about strings
> where many people expect the trailing '\0'.
> 
> I would even modify msg_print_text() to always add the trailing '\0'.
> All bytes will be used and it will be more error-proof API. I guess
> that this is what Sergey meant by better handling it on the
> msg_print_text() side.

I agree too. We either keep it as is and let the callers be able to add
the '\0', or we preferably add the '\0' ourselves and return the length
written (not counting the terminating '\0'), and we can then remove the
callers adding the '\0' later.

That's the safest approach.

-- Steve