5:37 p.m.
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.
---
src/phyp/phyp_driver.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index ee1e21b..f8fd29b 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)
@@ -410,6 +410,11 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
ret = phypExec(session, cmd, &exit_status, conn);
+ if (strlen(ret) > 4) {
+ VIR_ERROR0(ret);
+ goto err;
+ }
+
/* I need to parse the textual return in order to get the ret */
if (exit_status < 0 || ret == NULL)
goto err;
--
1.7.0.4
9:01 p.m.
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.
Also, your patch doesn't account for the possibility of nids > 1. I
don't know anything about the output of lssyscfg other than what I
learned from google just now, but it looks like you could end up with
multiple lines of output (and this function was certainly written to
at least attempt to parse multiple lines, ie multiple ids). Since you
error out if the entire string returned from the exec (ret) is > 4
bytes long (again, 3 was what you should have gone for, since you have
a 4 byte array and need to reserve one for the NULL), you would fail
if there was more than one value returned. You should instead be
making sure that *each individual line* is no longer than the limit.
Finally, you didn't change the length arg for the 2nd memset that's
inside the while loop, thus guaranteeing that you would overwrite 6
bytes of stack every time this function is called.
Of course, since virStrToLong_i will stop at the \n ending each line,
and not return an error since you're passing an endptr to it, why not
just do the conversion directly from the original string instead? That
way you wouldn't need to worry about all the memsets, checking for
overflows, etc, and you can use the pointer returned from the strtol
as the start of the next line, thus eliminating the overly complicated
character copy loop.
Here's a stab at doing it this way. I haven't even compiled it, but
you can give it a try and see if it solves your problem.
---
src/phyp/phyp_driver.c | 45 ++++++++++++++++-----------------------------
1 files changed, 16 insertions(+), 29 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index ee1e21b..b6e4c5a 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -380,12 +380,10 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
int system_type = phyp_driver->system_type;
char *managed_system = phyp_driver->managed_system;
int exit_status = 0;
- int got = 0;
- char *char_ptr;
- unsigned int i = 0, j = 0;
- char id_c[10];
+ int got = -1;
char *cmd = NULL;
char *ret = NULL;
+ char *line, *next_line;
const char *state;
virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -394,8 +392,6 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
else
state = " ";
- memset(id_c, 0, 10);
-
virBufferAddLit(&buf, "lssyscfg -r lpar");
if (system_type == HMC)
virBufferVSprintf(&buf, " -m %s", managed_system);
@@ -410,37 +406,28 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
ret = phypExec(session, cmd, &exit_status, conn);
- /* I need to parse the textual return in order to get the ret */
if (exit_status < 0 || ret == NULL)
goto err;
- else {
- while (got < nids) {
- if (ret[i] == '\0')
- break;
- else if (ret[i] == '\n') {
- if (virStrToLong_i(id_c, &char_ptr, 10, &ids[got]) == -1) {
- VIR_ERROR(_("Cannot parse number from '%s'"),
id_c);
- goto err;
- }
- memset(id_c, 0, 10);
- j = 0;
- got++;
- } else {
- id_c[j] = ret[i];
- j++;
- }
- i++;
+
+ /* I need to parse the textual return in order to get the ids */
+ line = ret;
+ got = 0;
+ while (*line && got < nids) {
+ if (virStrToLong_i(line, &next_line, 10, &ids[got]) == -1) {
+ VIR_ERROR(_("Cannot parse number from '%s'"), line);
+ got = -1;
+ goto err;
}
+ got++;
+ line = next_line;
+ while (*line == '\n')
+ line++; /* skip \n */
}
- VIR_FREE(cmd);
- VIR_FREE(ret);
- return got;
-
err:
VIR_FREE(cmd);
VIR_FREE(ret);
- return -1;
+ return got;
}
static int
--
1.7.1.1
2:04 a.m.
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
2:44 p.m.
On 07/16/2010 03:04 AM, Eduardo Otubo wrote:
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.
(this new patch is
corrupt and doesn't apply. Not sure how you sent it,
but I'm guessing your mailer mangled it. It's usually better to only
send patches with git send-email, or to attach the output of git
format-patch to the mail, rather than pasting it inline)
This version is closer *in location* to avoiding the overflow, but it's
still comparing the wrong index - it's looking at the index into the
full array (ret, ie "i") rather than into the small copy that is used to
convert (id_c, ie "j").
Also, it still allows writing past the end of the array (i == j == 4, so
it can write to id_c[4]). Even allowing a write to id_c[3] would be a
problem, since that would overwrite the terminating NULL, thus creating
the possibility that virStrToLong_i could overrun the end of the array
(or, if virStrToLong_i fails, the VIR_ERROR following it would try to
print a string that is not NULL terminated - a *much* worse prospect).
Additionally the 2nd occurence of "memset(id_c, 0, 10)" inside the loop
that I noted in my last mail has been left unchanged (and this memset
will be done every time the function is called, even if there is no
overflow of string length, so it is *guaranteed* the buffer will be
overflowed).
Finally, in the case that the output of the exec is too long, this new
patch will simply exit from the loop early and return success, rather
than logging an error and returning failure.
These problems *could* be fixed by 1) changing "i <= 4" to "j <
3", 2)
fixing the 2nd memset to clear 4 bytes instead of 10, and 3) turning the
case of "j >= 3" into an error instead of just exiting the loop.
However... did you look at the counter-proposed patch in my previous
email? I'm assuming that either you didn't notice it, or that you're
trying to come up with a patch that makes the fewest changes possible
while fixing the perceived bug. I think that patch would be a better
idea for several reasons, and as long as we're fixing this function, we
may as well fix it in the best way possible.
It takes advantage of the fact that virStrToLong_i will terminate on any
non-numeric (not just NULL) to avoid the copy completely thus
eliminating any built-in limits, and uses virStrToLong_i's returned
"endptr" in place of counting characters itself. The resulting
advantages are:
1) It is more "future proof". Maybe for now lssyscfg will not have any
id's > 256, that may change in the future, and we have no way of knowing
how long this code will last. For example, even if there are never more
than 999 of them, maybe someone down the road will decide that these
id's should be randomly generated for security reasons or something.
2) Because there is no fixed limit, there is no possibility of any "off
by one" bugs.
3) The complicated character copying loop has been eliminated.
4) Also, error and success exit paths have been merged, meaning that
resources are cleared in a single place, making it more difficult to
forget to free resources on one exit path but not the other.
All of these make the function easier to verify as bug-free now, and
will make future maintenance easier, since it will be both easier to
understand, and more difficult to break (this current iteration of
patches is ample proof that the current design makes it more difficult
to catch bugs ;-)
2 p.m.
Hello Laine,
It's been quite a while since we had this discussion about my patch.
Sorry about the delay on replaying, I had a congress and I've been sick
in the last couple of days. Now going back to work. :-)
(this new patch is corrupt and doesn't apply. Not sure how you
sent it,
but I'm guessing your mailer mangled it. It's usually better to only
send patches with git send-email, or to attach the output of git
format-patch to the mail, rather than pasting it inline)
I don't know what happened, I always send patches using git send-email.
I'll recheck next times. Thanks.
This version is closer *in location* to avoiding the overflow, but it's
still comparing the wrong index - it's looking at the index into the
full array (ret, ie "i") rather than into the small copy that is used to
convert (id_c, ie "j").
Also, it still allows writing past the end of the array (i == j == 4, so
it can write to id_c[4]). Even allowing a write to id_c[3] would be a
problem, since that would overwrite the terminating NULL, thus creating
the possibility that virStrToLong_i could overrun the end of the array
(or, if virStrToLong_i fails, the VIR_ERROR following it would try to
print a string that is not NULL terminated - a *much* worse prospect).
Additionally the 2nd occurence of "memset(id_c, 0, 10)" inside the loop
that I noted in my last mail has been left unchanged (and this memset
will be done every time the function is called, even if there is no
overflow of string length, so it is *guaranteed* the buffer will be
overflowed).
Finally, in the case that the output of the exec is too long, this new
patch will simply exit from the loop early and return success, rather
than logging an error and returning failure.
These problems *could* be fixed by 1) changing "i <= 4" to "j <
3", 2)
fixing the 2nd memset to clear 4 bytes instead of 10, and 3) turning the
case of "j >= 3" into an error instead of just exiting the loop.
However... did you look at the counter-proposed patch in my previous
email? I'm assuming that either you didn't notice it, or that you're
trying to come up with a patch that makes the fewest changes possible
while fixing the perceived bug. I think that patch would be a better
idea for several reasons, and as long as we're fixing this function, we
may as well fix it in the best way possible.
I just saw your patch now, seem reasonable. I understand your points and
I believe your patch fixes the bug pretty well. Applied here, compiled
and run with some tests, seems pretty ack for me.
Thanks for the help :-)
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo(a)linux.vnet.ibm.com
2:04 p.m.
On 08/04/2010 03:00 PM, Eduardo Otubo wrote:
Hello Laine,
It's been quite a while since we had this discussion about my patch.
Sorry about the delay on replaying, I had a congress and I've been
sick in the last couple of days. Now going back to work. :-)
> (this new patch is corrupt and doesn't apply. Not sure how you sent it,
> but I'm guessing your mailer mangled it. It's usually better to only
> send patches with git send-email, or to attach the output of git
> format-patch to the mail, rather than pasting it inline)
I don't know what happened, I always send patches using git
send-email. I'll recheck next times. Thanks.
>
> This version is closer *in location* to avoiding the overflow, but it's
> still comparing the wrong index - it's looking at the index into the
> full array (ret, ie "i") rather than into the small copy that is used to
> convert (id_c, ie "j").
>
> Also, it still allows writing past the end of the array (i == j == 4, so
> it can write to id_c[4]). Even allowing a write to id_c[3] would be a
> problem, since that would overwrite the terminating NULL, thus creating
> the possibility that virStrToLong_i could overrun the end of the array
> (or, if virStrToLong_i fails, the VIR_ERROR following it would try to
> print a string that is not NULL terminated - a *much* worse prospect).
>
> Additionally the 2nd occurence of "memset(id_c, 0, 10)" inside the loop
> that I noted in my last mail has been left unchanged (and this memset
> will be done every time the function is called, even if there is no
> overflow of string length, so it is *guaranteed* the buffer will be
> overflowed).
>
> Finally, in the case that the output of the exec is too long, this new
> patch will simply exit from the loop early and return success, rather
> than logging an error and returning failure.
>
> These problems *could* be fixed by 1) changing "i <= 4" to "j <
3", 2)
> fixing the 2nd memset to clear 4 bytes instead of 10, and 3) turning the
> case of "j >= 3" into an error instead of just exiting the loop.
>
> However... did you look at the counter-proposed patch in my previous
> email? I'm assuming that either you didn't notice it, or that you're
> trying to come up with a patch that makes the fewest changes possible
> while fixing the perceived bug. I think that patch would be a better
> idea for several reasons, and as long as we're fixing this function, we
> may as well fix it in the best way possible.
I just saw your patch now, seem reasonable. I understand your points
and I believe your patch fixes the bug pretty well. Applied here,
compiled and run with some tests, seems pretty ack for me.
I just want to verify: you applied the patch from my mail with 0
changes, and built/tested with that, correct?
8:55 a.m.
On 08/05/2010 04:04 PM, Laine Stump wrote:
On 08/04/2010 03:00 PM, Eduardo Otubo wrote:
> Hello Laine,
>
> It's been quite a while since we had this discussion about my patch.
> Sorry about the delay on replaying, I had a congress and I've been
> sick in the last couple of days. Now going back to work. :-)
>
>> (this new patch is corrupt and doesn't apply. Not sure how you sent it,
>> but I'm guessing your mailer mangled it. It's usually better to only
>> send patches with git send-email, or to attach the output of git
>> format-patch to the mail, rather than pasting it inline)
>
> I don't know what happened, I always send patches using git
> send-email. I'll recheck next times. Thanks.
>
>>
>> This version is closer *in location* to avoiding the overflow, but it's
>> still comparing the wrong index - it's looking at the index into the
>> full array (ret, ie "i") rather than into the small copy that is used
to
>> convert (id_c, ie "j").
>>
>> Also, it still allows writing past the end of the array (i == j == 4, so
>> it can write to id_c[4]). Even allowing a write to id_c[3] would be a
>> problem, since that would overwrite the terminating NULL, thus creating
>> the possibility that virStrToLong_i could overrun the end of the array
>> (or, if virStrToLong_i fails, the VIR_ERROR following it would try to
>> print a string that is not NULL terminated - a *much* worse prospect).
>>
>> Additionally the 2nd occurence of "memset(id_c, 0, 10)" inside the
loop
>> that I noted in my last mail has been left unchanged (and this memset
>> will be done every time the function is called, even if there is no
>> overflow of string length, so it is *guaranteed* the buffer will be
>> overflowed).
>>
>> Finally, in the case that the output of the exec is too long, this new
>> patch will simply exit from the loop early and return success, rather
>> than logging an error and returning failure.
>>
>> These problems *could* be fixed by 1) changing "i <= 4" to "j
< 3", 2)
>> fixing the 2nd memset to clear 4 bytes instead of 10, and 3) turning the
>> case of "j >= 3" into an error instead of just exiting the loop.
>>
>> However... did you look at the counter-proposed patch in my previous
>> email? I'm assuming that either you didn't notice it, or that you're
>> trying to come up with a patch that makes the fewest changes possible
>> while fixing the perceived bug. I think that patch would be a better
>> idea for several reasons, and as long as we're fixing this function, we
>> may as well fix it in the best way possible.
>
> I just saw your patch now, seem reasonable. I understand your points
> and I believe your patch fixes the bug pretty well. Applied here,
> compiled and run with some tests, seems pretty ack for me.
I just want to verify: you applied the patch from my mail with 0
changes, and built/tested with that, correct?
Yes, applied your patch with no changes to a clean branch.
--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo(a)linux.vnet.ibm.com
3:34 p.m.
On 07/15/2010 08:01 PM, Laine Stump wrote:
Here's a stab at doing it this way. I haven't even compiled
it, but
you can give it a try and see if it solves your problem.
I _have_ compiled it, and double-checked it for any obvious logic flaws.
There's a subtle change in semantics:
+ /* I need to parse the textual return in order to get the ids
*/
+ line = ret;
+ got = 0;
+ while (*line && got < nids) {
+ if (virStrToLong_i(line, &next_line, 10, &ids[got]) == -1) {
+ VIR_ERROR(_("Cannot parse number from '%s'"), line);
+ got = -1;
+ goto err;
}
+ got++;
+ line = next_line;
+ while (*line == '\n')
+ line++; /* skip \n */
}
- VIR_FREE(cmd);
- VIR_FREE(ret);
- return got;
-
err:
VIR_FREE(cmd);
VIR_FREE(ret);
- return -1;
+ return got;
}
Before, this always returned -1 on failure. But now, if you parse one
line before failing to parse the second, it returns 1. I think the err:
label should continue to return -1 on failure.
ACK with that change.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:49 p.m.
On 08/05/2010 04:34 PM, Eric Blake wrote:
On 07/15/2010 08:01 PM, Laine Stump wrote:
> Here's a stab at doing it this way. I haven't even compiled it, but
> you can give it a try and see if it solves your problem.
I _have_ compiled it, and double-checked it for any obvious logic flaws.
There's a subtle change in semantics:
> + /* I need to parse the textual return in order to get the ids */
> + line = ret;
> + got = 0;
> + while (*line&& got< nids) {
> + if (virStrToLong_i(line,&next_line, 10,&ids[got]) == -1) {
> + VIR_ERROR(_("Cannot parse number from '%s'"), line);
> + got = -1;
> + goto err;
> }
> + got++;
> + line = next_line;
> + while (*line == '\n')
> + line++; /* skip \n */
> }
>
> - VIR_FREE(cmd);
> - VIR_FREE(ret);
> - return got;
> -
> err:
> VIR_FREE(cmd);
> VIR_FREE(ret);
> - return -1;
> + return got;
> }
Before, this always returned -1 on failure. But now, if you parse one
line before failing to parse the second, it returns 1. I think the err:
label should continue to return -1 on failure.
But right before the goto err; there is a "got = -1;" Am I missing
something?
ACK with that change.
9:09 p.m.
On 08/05/2010 07:49 PM, Laine Stump wrote:
>> + if (virStrToLong_i(line,&next_line,
10,&ids[got]) == -1) {
>> + VIR_ERROR(_("Cannot parse number from '%s'"),
line);
>> + got = -1;
>> + goto err;
>> err:
>> VIR_FREE(cmd);
>> VIR_FREE(ret);
>> - return -1;
>> + return got;
>> }
> Before, this always returned -1 on failure. But now, if you parse one
> line before failing to parse the second, it returns 1. I think the err:
> label should continue to return -1 on failure.
But right before the goto err; there is a "got = -1;" Am I missing
something?
Nope - I was missing the got = -1. Thanks; no change needed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:23 a.m.
On 08/05/2010 10:09 PM, Eric Blake wrote:
On 08/05/2010 07:49 PM, Laine Stump wrote:
>>> + if (virStrToLong_i(line,&next_line, 10,&ids[got]) == -1) {
>>> + VIR_ERROR(_("Cannot parse number from '%s'"),
line);
>>> + got = -1;
>>> + goto err;
>>> err:
>>> VIR_FREE(cmd);
>>> VIR_FREE(ret);
>>> - return -1;
>>> + return got;
>>> }
>> Before, this always returned -1 on failure. But now, if you parse one
>> line before failing to parse the second, it returns 1. I think the err:
>> label should continue to return -1 on failure.
>
> But right before the goto err; there is a "got = -1;" Am I missing
> something?
Nope - I was missing the got = -1. Thanks; no change needed.
Okay, based on Eduardo's report of success, and your approval, I pushed
the patch. Thanks!
5222
days inactive
5244
days old
10 comments
3 participants
participants (3)
-
Eduardo Otubo -
Eric Blake -
Laine Stump