On Mon, May 04, 2026 at 04:06:54PM +0200, Claudio Jeker wrote:
> On Mon, May 04, 2026 at 02:03:52PM +0200, Theo Buehler wrote:
> > On Thu, Apr 30, 2026 at 09:22:56PM +0200, Claudio Jeker wrote:
> > > This is a fun one. The new bucket allocator used for attrs jumps from
> > > 240 to 256 entries. The problem is that l is a uint8_t and so 256
> > > becomes 0. The reallocarray shrinks the array and you can imagine the
> > > rest.
> > > 
> > > In attr_optadd() check the value returned by bin_of_attrs() and limit it
> > > to UCHAR_MAX to prevent the overflow. Also switch the iterator variable
> > > from uint8_t to int (I did this everywhere because it is more natural).
> > > 
> > > Does this make sense?
> > 
> > I think it does but I find it very difficult to see through all the magic.
> > 
> > Just to make sure: do I understand correctly that the
> > 
> > 	fatalx("attr_optadd: attribute overflow");
> > 
> > wasn't reachable before and that the fact that it is reachable now is
> > making things work as intended?
> 
> Yes. The check
> 	if (asp->others_len == UCHAR_MAX)
> could no longer trigger since bin_of_attrs() returns a sequence that
> goes from 240 to 256.
> 
> In other words when adding the 241 attribute l turned 0 and as a
> consequence of that a reallocarray of 0 elements and a memset with a
> negative size would happen. 
> 
> We can't really get to 255 attributes since a fair bunch are handled
> differently (like ASPATH, COMMUNITY or MED) but it is possible to exceed
> 240.

Thanks, that helped. I'm not sure I would have changed all the other l
and oalen, but if you prefer it this way I won't object.

ok tb