[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] phyp: Fixing possible buffer overflow



On 07/15/2010 11:01 PM, Laine Stump wrote:
On 07/15/2010 06:37 PM, Eduardo Otubo wrote:
The line src/phyp/phyp_driver.c:427 was crashing by buffer overflow
if the return of the command wasn't<=10. The highest number for a
LPAR ID is 256 per machine, no need to allocate 10 bytes for it. So,
adjusting the correct size (+1 byte for the '\n') and checking for
errors.

This analysis doesn't make sense to me. On the one hand, you say that
the function was crashing if the return from the command wasn't<= 10
bytes long (implying that it sometimes was longer), on the other hand,
you say that part of the solution is to change the code to only handle
4 bytes (you forgot about the trailing NULL there, BTW - should have
been 3), because you'll never get more than that.

I'm sorry. My bad here. The lssyscfg (with the correct parameters and greps) will return something like this:

5
4
6
7
10
1
143

These are the LPAR IDs. Each line cannot be bigger than 3 characters (+1 for '\n'). So, I just allocate 4 bytes to hold these values, one at a time and memset it with zeros. If there's an error, there will be a single line with the error message, like this one:

"HSCL8018 The managed system rico was not found."

IMO the best choice is to loop in the result and if I can't find a '\n' after 3 character, I return an error. My mistake was to check the entire string returned from exec. My bad, sorry.

I think this patch is much clearer on what the problem is.

Thanks for the comments!

---
 src/phyp/phyp_driver.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index ee1e21b..0c50c9a 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -383,7 +383,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
     int got = 0;
     char *char_ptr;
     unsigned int i = 0, j = 0;
-    char id_c[10];
+    char id_c[4];
     char *cmd = NULL;
     char *ret = NULL;
     const char *state;
@@ -394,7 +394,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
     else
         state = " ";

-    memset(id_c, 0, 10);
+    memset(id_c, 0, 4);

     virBufferAddLit(&buf, "lssyscfg -r lpar");
     if (system_type == HMC)
@@ -414,7 +414,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
     if (exit_status < 0 || ret == NULL)
         goto err;
     else {
-        while (got < nids) {
+        while (got < nids && i <= 4) {
             if (ret[i] == '\0')
                 break;
             else if (ret[i] == '\n') {
--
1.7.0.4


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]