Re: [PATCH v5 06/12] seq_buf: Add seq_buf_strlen()

[Alejandro Colomar]

Hi Steven,

> Date: 2026-10-22 15:57:51-0400
> From: Steven Rostedt <rostedt@xxxxxxxxxxx>
>
> On Thu, 22 Oct 2026 21:41:53 +0200
> Alejandro Colomar <alx+linux-hardening@xxxxxxxxxx> wrote:
> 
> > > > Hmm, I just realized that this could inadvertently overflow the buffer. If
> > > > len == size, it may not be overflowed. If using seq_buf_putmem() and
> > > > filling the entire buffer, calling this would force an overflow as we just
> > > > truncated the last byte.  
> > > 
> > > Hm, yes. So the "else" branch needs to include:
> > > 
> > > 	s->len = s->size + 1;  
> > 
> > Please think again about the possibility of using pointers instead of
> > lengths.  This is a source of off-by-one bugs that's only caused by
> > the choice of data, I believe.  If using pointers, truncation would be
> > stored as a NULL pointer, and off-by-ones would be less likely.
> > 
> > Consider this off-by-one to be just a sympthom of the underlying
> > problematic design.  Whenever a design forces you to write +1 or -1,
> > there are high chances that the design is sub-optimal regarding safety.
> 
> It's not an off-by-one error here. The s->len = size - 1 is correct, but it
> also has a side effect of removing a character without marking it
> overflowed.

It's correct, but at the same time an opportunity for off-by-one
mistakes.

> The seq_buf defines being "overflowed" as s->len == s->size + 1.
> What the else statement should have is:
> 
> 	else
> 		seq_buf_set_overflow(s);
> 
> Which would be less confusing to reviewers as that is defined as:
> 
> static inline void
> seq_buf_set_overflow(struct seq_buf *s)
> {
> 	s->len = s->size + 1;
> }

Agree.

But still, a better design would do:

	static inline void
	seq_buf_set_overflow(struct seq_buf *s)
	{
		s->p = NULL;
	}

This is way more robust, and also easier to understand.  My head is
boiling when trying to review this patch set for correctness.  That's
not a good sign.


Have a lovely night!
Alex

> 
> -- Steve

-- 
<https://www.alejandro-colomar.es>

Attachment: signature.asc
Description: PGP signature