From: Nikita Popov <nikita.ppv@gmail.com>
Date: Tue, 14 Nov 2017 20:18:40 +0000 (+0100)
Subject: Clarify bin_to_readable code
X-Git-Tag: php-7.3.0alpha1~1028^2~4
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a57f370e590367a149b39e5908a2c84228547b00;p=php

Clarify bin_to_readable code

I got a bit of a scare when I first saw this code. Turns out that
the way it's used inlen==outlen and that's why it works.
---

diff --git a/ext/session/session.c b/ext/session/session.c
index a2642807ad..bfd1c9b566 100644
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -268,12 +268,10 @@ static int php_session_decode(zend_string *data) /* {{{ */
 
 static char hexconvtab[] = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-";
 
-/* returns a pointer to the byte after the last valid character in out */
-static size_t bin_to_readable(unsigned char *in, size_t inlen, char *out, char nbits) /* {{{ */
+static void bin_to_readable(unsigned char *in, size_t inlen, char *out, size_t outlen, char nbits) /* {{{ */
 {
 	unsigned char *p, *q;
 	unsigned short w;
-	size_t len = inlen;
 	int mask;
 	int have;
 
@@ -284,16 +282,15 @@ static size_t bin_to_readable(unsigned char *in, size_t inlen, char *out, char n
 	have = 0;
 	mask = (1 << nbits) - 1;
 
-	while (inlen--) {
+	while (outlen--) {
 		if (have < nbits) {
 			if (p < q) {
 				w |= *p++ << have;
 				have += 8;
 			} else {
-				/* consumed everything? */
-				if (have == 0) break;
-				/* No? We need a final round */
-				have = nbits;
+				/* Should never happen. Input must be large enough. */
+				ZEND_ASSERT(0);
+				break;
 			}
 		}
 
@@ -304,7 +301,6 @@ static size_t bin_to_readable(unsigned char *in, size_t inlen, char *out, char n
 	}
 
 	*out = '\0';
-	return len;
 }
 /* }}} */
 
@@ -315,13 +311,18 @@ PHPAPI zend_string *php_session_create_id(PS_CREATE_SID_ARGS) /* {{{ */
 	unsigned char rbuf[PS_MAX_SID_LENGTH + PS_EXTRA_RAND_BYTES];
 	zend_string *outid;
 
+	/* It would be enough to read ceil(sid_length * sid_bits_per_character / 8) bytes here.
+	 * We read sid_length bytes instead for simplicity. */
 	/* Read additional PS_EXTRA_RAND_BYTES just in case CSPRNG is not safe enough */
 	if (php_random_bytes_throw(rbuf, PS(sid_length) + PS_EXTRA_RAND_BYTES) == FAILURE) {
 		return NULL;
 	}
 
 	outid = zend_string_alloc(PS(sid_length), 0);
-	ZSTR_LEN(outid) = bin_to_readable(rbuf, PS(sid_length), ZSTR_VAL(outid), (char)PS(sid_bits_per_character));
+	bin_to_readable(
+		rbuf, PS(sid_length),
+		ZSTR_VAL(outid), ZSTR_LEN(outid),
+		(char)PS(sid_bits_per_character));
 
 	return outid;
 }