On 2024/04/19 00:11:45 +0200, Tobias Heider <tobias.heider@stusta.de> wrote:
> On Thu, Apr 18, 2024 at 05:01:20PM +0200, Omar Polo wrote:
> > 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

(for posteriority: I was confused since the change in bgpd was about two
hex; for 4 we need at least a uint16_t :)

> > 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.
> > 
> 
> Sounds good.  how do you like this instead?
> I also removed the explicit null termination in lenstr because that isn't
> needed anymore.

yeah; ok op@ with a nit below

> +const struct got_error *
> +got_pkt_readlen(int *len, const char *str, int chattygot)
> +{
> [...]
> +			if (chattygot)
> +				fprintf(stderr, "%s: bad length: '%s'\n",

here it should be %.4s since str is not NUL-terminated anymore.

> +				    getprogname(), str);
> +			return got_error_msg(GOT_ERR_BAD_PACKET,
> +			    "packet length has invalid format");
> +		}
> +	}
> +	return NULL;
> +}
> +


Thanks!

Omar Polo