Skip to contentSkip to conversation

xfsprogs: properly terminate string in quota's restore_file()

PATCH In review Eric Sandeen 3 participants 3 messages Aug 25, 2015 – Aug 26, 2015

lore
Share & tools
lore

Eric Sandeen

author
#0 lore raw

PATCH 1/1 xfsprogs: properly terminate string in quota's restore_file()

This code copies up to the entire size of devbuffer, and then tries to use "strlen" to null terminate it.

But strlen works by *finding* the null, so it's at best a no-op, and at worst not properly terminating the string.

Fix this by placing the null at the last byte of the buffer.

Addresses-Coverity-Id: 1297519

Signed-off-by Eric Sandeen <[email protected]>

---

quota/edit.c +1 -1
index d226e89..a53a7e6 100644
@@ -385,7 +385,7 @@ restore_file(
 	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
 		if (strncmp("fs = ", buffer, 5) == 0) {
 			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
-			dev[strlen(dev) - 1] = '\0';
+			dev[sizeof(devbuffer) - 1] = '\0';
 			continue;
 		}
 		rtbsoft = rtbhard = 0;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#1 lore raw
Quoted 17 lines · Eric Sandeen

This code copies up to the entire size of devbuffer, and then tries to use "strlen" to null terminate it.

But strlen works by *finding* the null, so it's at best a no-op, and at worst not properly terminating the string.

Fix this by placing the null at the last byte of the buffer.

Addresses-Coverity-Id: 1297519

Signed-off-by Eric Sandeen <[email protected]>

---

quota/edit.c +0 -0
index d226e89..a53a7e6 100644

quota/edit.c restore_file( · line 388

Quoted 5 lines
quota/edit.c +1 -1
@@ -385,7 +385,7 @@ restore_file(
 	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
 		if (strncmp("fs = ", buffer, 5) == 0) {
 			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
-			dev[strlen(dev) - 1] = '\0';
+			dev[sizeof(devbuffer) - 1] = '\0';

According to the man page, fgets() NULL terminates the provided buffer. Next, we attempt to strncpy() just the device name part of the string (copying up to 512 bytes from a 512-5 byte buffer). I'm not quite sure, but it looks like the above line could be trying to replace a newline with a NULL terminator..? E.g., it expects the last character in an already NULL terminated line to be a newline.

Brian

Quoted 8 lines
quota/edit.c +0 -0
 			continue;
 		}
 		rtbsoft = rtbhard = 0;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#2 lore raw
Quoted 13 lines · Brian Foster
Quoted 12 lines · Eric Sandeen

This code copies up to the entire size of devbuffer, and then tries to use "strlen" to null terminate it.

But strlen works by *finding* the null, so it's at best a no-op, and at worst not properly terminating the string.

Fix this by placing the null at the last byte of the buffer.

Addresses-Coverity-Id: 1297519

Signed-off-by Eric Sandeen <[email protected]>

---

Quoted 12 lines

quota/edit.c restore_file( · line 388

Quoted 10 lines
quota/edit.c +1 -1
index d226e89..a53a7e6 100644
@@ -385,7 +385,7 @@ restore_file(
 	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
 		if (strncmp("fs = ", buffer, 5) == 0) {
 			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
-			dev[strlen(dev) - 1] = '\0';
+			dev[sizeof(devbuffer) - 1] = '\0';

According to the man page, fgets() NULL terminates the provided buffer.

Yep, I should have looked more closely.

It copies at most size-1 (511 bytes), including the newline if it fits, and terminates with \0.

Quoted 2 lines

Next, we attempt to strncpy() just the device name part of the string (copying up to 512 bytes from a 512-5 byte buffer).

In fact since the original buffer (via fgets, 512) will be <= 512 long, copying again from 5 bytes in will always result in less than 512 bytes copied.

Quoted 4 lines

I'm not quite sure,
but it looks like the above line could be trying to replace a newline with a NULL terminator..? E.g., it expects the last character in an already NULL terminated line to be a newline.

Yes, that's exactly it, thanks.

There is one very corner case where if the last char in the buffer is part of the device name, the buffer won't contain the newline, and the '\0' will actually take out one char, rather than the newline. But that's one heck of a corner case, the exactly-sized-device-name case ... probably not worth worrying about, though I might send a patch just to satisfy my OCD.

Thanks for keeping me straight on this one, sorry for the noise.

-Eric

Quoted 11 lines

Brian

Quoted 8 lines
quota/edit.c +0 -0
 			continue;
 		}
 		rtbsoft = rtbhard = 0;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 5 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs