lib/bitmap.c: fix some parsing issues and code style

PATCH Superseded Pan Xinhui 2 participants 8 messages Jul 1, 2015

Subsystems (2)

Library code Bitmap API

lore

Share & tools

lore

Pan Xinhui

author

2015-07-01 03:17 UTC #0 lore raw

PATCH 1/1 lib/bitmap.c: fix some parsing issues and code style

In __bitmap_parselist we can accept whitespaces on head or tail during every parsing procedure.
If input has valid ranges, there is no reason to reject the user.

fixes:
1) if input ends with ',', bit 0 might be set unexpectedly. now we check if any digit is available after every loop.
2) if input has '0-', bit 0 might be set unexpectedly, now we return -EINVAL as this kind of input is definitely wrong.
3) minor code style fix in __bitmap_parse.
and avoid in-loop incrementation of ndigits.

Signed-off-by Pan Xinhui <[email protected]>

---
 lib/bitmap.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

lib/bitmap.c +22 ~~-12~~

index 64c0926..3ae3ef1 100644
@@ -367,7 +367,8 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
 
 	nchunks = nbits = totaldigits = c = 0;
 	do {
-		chunk = ndigits = 0;
+		chunk = 0;
+		ndigits = totaldigits;
 
 		/* Get the next chunk of the bitmap */
 		while (buflen) {

@@ -406,9 +407,9 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
 				return -EOVERFLOW;
 
 			chunk = (chunk << 4) | hex_to_bin(c);
-			ndigits++; totaldigits++;
+			totaldigits++;
 		}
-		if (ndigits == 0)
+		if (unlikely(ndigits == totaldigits))
 			return -EINVAL;
 		if (nchunks == 0 && chunk == 0)
 			continue;

@@ -504,7 +505,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		int nmaskbits)
 {
 	unsigned a, b;
-	int c, old_c, totaldigits;
+	int c, old_c, totaldigits, ndigits;
 	const char __user __force *ubuf = (const char __user __force *)buf;
 	int exp_digit, in_range;

@@ -514,6 +515,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		exp_digit = 1;
 		in_range = 0;
 		a = b = 0;
+		ndigits = totaldigits;
 
 		/* Get the next cpu# or a range of cpu#'s */
 		while (buflen) {

@@ -527,17 +529,20 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			if (isspace(c))
 				continue;
 
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
 			/* A '\0' or a ',' signal the end of a cpu# or range */
 			if (c == '\0' || c == ',')
 				break;
+			/*
+			* whitespaces between digits are not allowed,
+			* but it's ok if whitespaces are on head or tail.
+			* when old_c is whilespace,
+			* if totaldigits == ndigits, whitespace is on head.
+			* if whitespace is on tail, it should not run here.
+			* as c was ',' or '\0',
+			* ans the last code line has broken the current loop.
+			*/
+			if ((totaldigits != ndigits) && isspace(old_c))
+				return -EINVAL;
 
 			if (c == '-') {
 				if (exp_digit || in_range)

@@ -557,6 +562,11 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			exp_digit = 0;
 			totaldigits++;
 		}
+		if (unlikely(ndigits == totaldigits))
+			continue;
+		/* if no digti is after '-', it's wrong*/
+		if (unlikely(exp_digit && in_range))
+			return -EINVAL;
 		if (!(a <= b))
 			return -EINVAL;
 		if (b >= nmaskbits)

Signature · 1 line

1.9.1

Pan Xinhui

author

2015-07-01 04:18 UTC #1 lore raw

PATCH v2 1/1 lib/bitmap.c: fix some parsing issues and code style

In __bitmap_parselist we can accept whitespaces on head or tail during every parsing procedure.
If input has valid ranges, there is no reason to reject the user.

fixes are:
1) if input ends with ',', bit 0 might be set unexpectedly. now we check if any digit is available after every loop.
2) if input has '0-', bit 0 might be set unexpectedly, now we return -EINVAL as this kind of input is definitely wrong.
3) minor code style fix in __bitmap_parse.
and avoid in-loop incrementation of ndigits.

commit 2528a8b also add some check, but it's still not enough. it only correct the result in fix 1 above.

Signed-off-by Pan Xinhui <[email protected]>

---
 lib/bitmap.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

---
change log
V2 from v1: solve the codes conflicts
---

lib/bitmap.c +26 ~~-17~~

index a578a01..b0e3afc 100644
@@ -367,7 +367,8 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
 
 	nchunks = nbits = totaldigits = c = 0;
 	do {
-		chunk = ndigits = 0;
+		chunk = 0;
+		ndigits = totaldigits;
 
 		/* Get the next chunk of the bitmap */
 		while (buflen) {

@@ -406,9 +407,9 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
 				return -EOVERFLOW;
 
 			chunk = (chunk << 4) | hex_to_bin(c);
-			ndigits++; totaldigits++;
+			totaldigits++;
 		}
-		if (ndigits == 0)
+		if (unlikely(ndigits == totaldigits))
 			return -EINVAL;
 		if (nchunks == 0 && chunk == 0)
 			continue;

@@ -505,7 +506,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		int nmaskbits)
 {
 	unsigned a, b;
-	int c, old_c, totaldigits;
+	int c, old_c, totaldigits, ndigits;
 	const char __user __force *ubuf = (const char __user __force *)buf;
 	int at_start, in_range;

@@ -515,6 +516,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		at_start = 1;
 		in_range = 0;
 		a = b = 0;
+		ndigits = totaldigits;
 
 		/* Get the next cpu# or a range of cpu#'s */
 		while (buflen) {

@@ -528,23 +530,27 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			if (isspace(c))
 				continue;
 
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
 			/* A '\0' or a ',' signal the end of a cpu# or range */
 			if (c == '\0' || c == ',')
 				break;
+			/*
+			* whitespaces between digits are not allowed,
+			* but it's ok if whitespaces are on head or tail.
+			* when old_c is whilespace,
+			* if totaldigits == ndigits, whitespace is on head.
+			* if whitespace is on tail, it should not run here.
+			* as c was ',' or '\0',
+			* the last code line has broken the current loop.
+			*/
+			if ((totaldigits != ndigits) && isspace(old_c))
+				return -EINVAL;
 
 			if (c == '-') {
 				if (at_start || in_range)
 					return -EINVAL;
 				b = 0;
 				in_range = 1;
+				at_start = 1;
 				continue;
 			}

@@ -557,15 +563,18 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			at_start = 0;
 			totaldigits++;
 		}
+		if (unlikely(ndigits == totaldigits))
+			continue;
+		/* if no digit is after '-', it's wrong*/
+		if (unlikely(at_start && in_range))
+			return -EINVAL;
 		if (!(a <= b))
 			return -EINVAL;
 		if (b >= nmaskbits)
 			return -ERANGE;
-		if (!at_start) {
-			while (a <= b) {
-				set_bit(a, maskp);
-				a++;
-			}
+		while (a <= b) {
+			set_bit(a, maskp);
+			a++;
 		}
 	} while (buflen && c == ',');
 	return 0;

Signature · 1 line

1.9.1

Frans Klaver

2015-07-01 06:17 UTC #2 lore raw

Quoted 5 lines · Pan Xinhui

In __bitmap_parselist we can accept whitespaces on head or tail during every parsing procedure.
If input has valid ranges, there is no reason to reject the user.

Quoted 7 lines

fixes are:
1) if input ends with ',', bit 0 might be set unexpectedly. now we check if any digit is available after every loop.
2) if input has '0-', bit 0 might be set unexpectedly, now we return -EINVAL as this kind of input is definitely wrong.
3) minor code style fix in __bitmap_parse.
and avoid in-loop incrementation of ndigits.

Why not three patches, so it becomes easier to see which is which?

Quoted 2 lines

commit 2528a8b also add some check, but it's still not enough. it only correct the result in fix 1 above.

I believe the convention is to have at least 12 characters of the sha1, with the title behind it: 2528a8b8f457 (__bitmap_parselist: fix bug in empty string handling). Using only seven characters still risks collisions.

Thanks,
Frans

Pan Xinhui

author

2015-07-01 06:28 UTC #3 lore raw

hello, Frans
thanks for your reply :)

Quoted 14 lines · Frans Klaver

Quoted 5 lines · Pan Xinhui
In __bitmap_parselist we can accept whitespaces on head or tail during every parsing procedure.
If input has valid ranges, there is no reason to reject the user.
Quoted 7 lines
fixes are:
1) if input ends with ',', bit 0 might be set unexpectedly. now we check if any digit is available after every loop.
2) if input has '0-', bit 0 might be set unexpectedly, now we return -EINVAL as this kind of input is definitely wrong.
3) minor code style fix in __bitmap_parse.
and avoid in-loop incrementation of ndigits.

Quoted 2 lines

Why not three patches, so it becomes easier to see which is which?

your advice sounds good, I will have a try. and welcome for review. :) thanks.

Quoted 9 lines

Quoted 3 lines
commit 2528a8b also add some check, but it's still not enough. it only correct the result in fix 1 above.
I believe the convention is to have at least 12 characters of the sha1, with the title behind it: 2528a8b8f457 (__bitmap_parselist: fix bug in empty string handling). Using only seven characters still risks collisions.

sorry for my lack knowledge of comment rules. thanks for pointing out my mistakes.

thanks
xinhui

Quoted 3 lines

Thanks,
Frans

Frans Klaver

2015-07-01 06:36 UTC #4 lore raw

Quoted 31 lines · Pan Xinhui

hello, Frans
thanks for your reply :)
Quoted 17 lines · Frans Klaver
Quoted 6 lines · Pan Xinhui
In __bitmap_parselist we can accept whitespaces on head or tail during every parsing procedure.
If input has valid ranges, there is no reason to reject the user.
Quoted 7 lines
fixes are:
1) if input ends with ',', bit 0 might be set unexpectedly. now we check if any digit is available after every loop.
2) if input has '0-', bit 0 might be set unexpectedly, now we return -EINVAL as this kind of input is definitely wrong.
3) minor code style fix in __bitmap_parse.
and avoid in-loop incrementation of ndigits.
Quoted 2 lines
Why not three patches, so it becomes easier to see which is which?
your advice sounds good, I will have a try. and welcome for review. :) thanks.
Quoted 5 lines
Quoted 3 lines
commit 2528a8b also add some check, but it's still not enough. it only correct the result in fix 1 above.

Quoted 7 lines

Quoted 5 lines
I believe the convention is to have at least 12 characters of the sha1, with the title behind it: 2528a8b8f457 (__bitmap_parselist: fix bug in empty string handling). Using only seven characters still risks collisions.
sorry for my lack knowledge of comment rules. thanks for pointing out my mistakes.

No problem. These kinds of things can be caught if you use scripts/checkpatch.pl on your patches before sending, by the way.

You should probably read Documentation/SubmittingPatches if you haven't already done so.

Frans

Pan Xinhui

author

2015-07-01 08:01 UTC #5 lore raw

PATCH 1/3 lib/bitmap.c: correct a code style and do some optimization in __bitmap_parse

In reply to Pan Xinhui

We can avoid in-loop incrementation of ndigits. Save current totaldigits to ndigits before loop, and check ndigits against totaldigits after the loop.

Signed-off-by Pan Xinhui <[email protected]>

Cc Yury Norov <[email protected]>

---
 lib/bitmap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

lib/bitmap.c +4 -3

index a578a01..eb21456 100644
@@ -367,7 +367,8 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
 
 	nchunks = nbits = totaldigits = c = 0;
 	do {
-		chunk = ndigits = 0;
+		chunk = 0;
+		ndigits = totaldigits;
 
 		/* Get the next chunk of the bitmap */
 		while (buflen) {

@@ -406,9 +407,9 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
 				return -EOVERFLOW;
 
 			chunk = (chunk << 4) | hex_to_bin(c);
-			ndigits++; totaldigits++;
+			totaldigits++;
 		}
-		if (ndigits == 0)
+		if (ndigits == totaldigits)
 			return -EINVAL;
 		if (nchunks == 0 && chunk == 0)
 			continue;

Signature · 1 line

1.9.1

Pan Xinhui

author

2015-07-01 08:02 UTC #7 lore raw

PATCH 2/3 lib/bitmap.c: fix a special string handling bug in __bitmap_parselist

In reply to Pan Xinhui

If string end with '-', for exapmle, bitmap_parselist("1,0-",&mask, nmaskbits), It is not in a valid pattern, so add a check after loop.
Return -EINVAL on such condition.

Signed-off-by Pan Xinhui <[email protected]>

---
 lib/bitmap.c | 4 ++++
 1 file changed, 4 insertions(+)

lib/bitmap.c +4 -0

index eb21456..f549176 100644
@@ -546,6 +546,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 					return -EINVAL;
 				b = 0;
 				in_range = 1;
+				at_start = 1;
 				continue;
 			}

@@ -558,6 +559,9 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			at_start = 0;
 			totaldigits++;
 		}
+		/* if no digit is after '-', it's wrong*/
+		if (at_start && in_range)
+			return -EINVAL;
 		if (!(a <= b))
 			return -EINVAL;
 		if (b >= nmaskbits)

Signature · 1 line

1.9.1

Pan Xinhui

author

2015-07-01 08:04 UTC #6 lore raw

PATCH 3/3 lib/bitmap.c: bitmap_parselist can accept string with whitespaces on head or tail

In reply to Pan Xinhui

In __bitmap_parselist we can accept whitespaces on head or tail during every parsing procedure.
If input has valid ranges, there is no reason to reject the user.

For example, bitmap_parselist(" 1-3, 5, ", &mask, nmaskbits). After separating the string, we get " 1-3", " 5", and " ". It's possible and reasonable to accept such string as long as the parsing result is correct.

Signed-off-by Pan Xinhui <[email protected]>

Cc Yury Norov <[email protected]>

---
 lib/bitmap.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

lib/bitmap.c +18 ~~-14~~

index f549176..8148143 100644
@@ -506,7 +506,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		int nmaskbits)
 {
 	unsigned a, b;
-	int c, old_c, totaldigits;
+	int c, old_c, totaldigits, ndigits;
 	const char __user __force *ubuf = (const char __user __force *)buf;
 	int at_start, in_range;

@@ -516,6 +516,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		at_start = 1;
 		in_range = 0;
 		a = b = 0;
+		ndigits = totaldigits;
 
 		/* Get the next cpu# or a range of cpu#'s */
 		while (buflen) {

@@ -529,17 +530,20 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			if (isspace(c))
 				continue;
 
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
 			/* A '\0' or a ',' signal the end of a cpu# or range */
 			if (c == '\0' || c == ',')
 				break;
+			/*
+			* whitespaces between digits are not allowed,
+			* but it's ok if whitespaces are on head or tail.
+			* when old_c is whilespace,
+			* if totaldigits == ndigits, whitespace is on head.
+			* if whitespace is on tail, it should not run here.
+			* as c was ',' or '\0',
+			* the last code line has broken the current loop.
+			*/
+			if ((totaldigits != ndigits) && isspace(old_c))
+				return -EINVAL;
 
 			if (c == '-') {
 				if (at_start || in_range)

@@ -559,6 +563,8 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			at_start = 0;
 			totaldigits++;
 		}
+		if (ndigits == totaldigits)
+			continue;
 		/* if no digit is after '-', it's wrong*/
 		if (at_start && in_range)
 			return -EINVAL;

@@ -566,11 +572,9 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 			return -EINVAL;
 		if (b >= nmaskbits)
 			return -ERANGE;
-		if (!at_start) {
-			while (a <= b) {
-				set_bit(a, maskp);
-				a++;
-			}
+		while (a <= b) {
+			set_bit(a, maskp);
+			a++;
 		}
 	} while (buflen && c == ',');
 	return 0;

Signature · 1 line

1.9.1