From: Tobias Heider <tobias.heider@stusta.de>
Subject: Re: move got_pkt_readlen() to separate function
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Fri, 19 Apr 2024 00:11:45 +0200

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
> 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.

diff /home/user/got/co/got
commit - 02dab75a4ff7b71643c9c150a7254721785d8b0e
path + /home/user/got/co/got
blob - 009561888bba0c0f18ea02211c6639bf87cfb368
file + lib/got_lib_pkt.h
--- lib/got_lib_pkt.h
+++ lib/got_lib_pkt.h
@@ -20,6 +20,7 @@
 const struct got_error *got_pkt_readn(ssize_t *off, int fd, void *buf,
     size_t n);
 const struct got_error *got_pkt_flushpkt(int fd, int chattygot);
+const struct got_error *got_pkt_readlen(int *len, const char *str, int chattygot);
 const struct got_error *got_pkt_readhdr(int *datalen, int fd, int chattygot);
 const struct got_error *got_pkt_readpkt(int *outlen, int fd, char *buf,
     int buflen, int chattygot);
blob - fad7f03e4f70f1513ceb7af03bb71e1e7907a950
file + lib/pkt.c
--- lib/pkt.c
+++ lib/pkt.c
@@ -58,6 +58,30 @@ got_pkt_flushpkt(int fd, int chattygot)
 	return NULL;
 }
 
+const struct got_error *
+got_pkt_readlen(int *len, const char *str, int chattygot)
+{
+	int	 i;
+
+	*len = 0;
+	for (i = 0; i < 4; i++) {
+		if ('0' <= str[i] && str[i] <= '9') {
+			*len *= 16;
+			*len += str[i] - '0';
+		} else if ('a' <= str[i] && str[i] <= 'f') {
+			*len *= 16;
+			*len += str[i] - 'a' + 10;
+		} else {
+			if (chattygot)
+				fprintf(stderr, "%s: bad length: '%s'\n",
+				    getprogname(), str);
+			return got_error_msg(GOT_ERR_BAD_PACKET,
+			    "packet length has invalid format");
+		}
+	}
+	return NULL;
+}
+
 /*
  * Packet header contains a 4-byte hexstring which specifies the length
  * of data which follows.
@@ -65,12 +89,10 @@ got_pkt_flushpkt(int fd, int chattygot)
 const struct got_error *
 got_pkt_readhdr(int *datalen, int fd, int chattygot)
 {
-	static const struct got_error *err = NULL;
-	char lenstr[5];
-	long len;
-	char *e;
-	int n, i;
+	static const struct got_error *err;
+	char lenstr[4];
 	ssize_t r;
+	int n;
 
 	*datalen = 0;
 
@@ -87,35 +109,12 @@ got_pkt_readhdr(int *datalen, int fd, int chattygot)
 		return got_error_msg(GOT_ERR_BAD_PACKET,
 		    "wrong packet header length");
 
-	lenstr[4] = '\0';
-	for (i = 0; i < 4; i++) {
-		if (!isprint((unsigned char)lenstr[i]))
-			return got_error_msg(GOT_ERR_BAD_PACKET,
-			    "unprintable character in packet length field");
-	}
-	for (i = 0; i < 4; i++) {
-		if (!isxdigit((unsigned char)lenstr[i])) {
-			if (chattygot)
-				fprintf(stderr, "%s: bad length: '%s'\n",
-				    getprogname(), lenstr);
-			return got_error_msg(GOT_ERR_BAD_PACKET,
-			    "packet length not specified in hex");
-		}
-	}
-	errno = 0;
-	len = strtol(lenstr, &e, 16);
-	if (lenstr[0] == '\0' || *e != '\0')
-		return got_error(GOT_ERR_BAD_PACKET);
-	if (errno == ERANGE && (len == LONG_MAX || len == LONG_MIN))
-		return got_error_msg(GOT_ERR_BAD_PACKET, "bad packet length");
-	if (len > INT_MAX || len < INT_MIN)
-		return got_error_msg(GOT_ERR_BAD_PACKET, "bad packet length");
-	n = len;
+	err = got_pkt_readlen(&n, lenstr, chattygot);
 	if (n == 0)
-		return NULL;
+		return err;
 	if (n <= 4)
 		return got_error_msg(GOT_ERR_BAD_PACKET, "packet too short");
-	n  -= 4;
+	n -= 4;
 
 	*datalen = n;
 	return NULL;