On 2024/04/18 16:51:37 +0200, Tobias Heider <tobias.heider@stusta.de> wrote:
> On Wed, Apr 17, 2024 at 10:41:01AM +0200, Omar Polo wrote:
> > > +const struct got_error *
> > > +got_pkt_readlen(int *len, const char *str, int chattygot)
> > > +{
> > > [...]
> > > +	errno = 0;
> > > +	*len = strtol(str, &e, 16);
> > > +	if (str[0] == '\0' || *e != '\0')
> > > +		return got_error(GOT_ERR_BAD_PACKET);
> > > +	if (errno == ERANGE || *len > INT_MAX || *len < INT_MIN)
> > > +		return got_error_msg(GOT_ERR_BAD_PACKET, "bad pkt-line length");
> > 
> > I'm not sure this bit is correct.  You're storing a long inside a int,
> > then check if it's bigger than INT_MAX or lesser than INT_MIN, which is
> > impossible.
> 
> Right, I guess this broke during refactoring when I changed the type to int.
> I wonder if we need to check anything at all since we know we parse at most
> 4 digits.

4 digits will fit a uint8_t, so we could just inline something instead
of bringing out strtol(3).  see for e.g. bgpd' parse.y rev 1.458 where
claudio ditched strtoul() and inlined something to convert two hex to
int.

sorry for nitpicking but found weird the check, and often code like this
gets copied around.