[Ipmitool-devel] [patch] Avoid comparing unsigned long with unsigned long (CID 1149026)

Discussion:

Petter Reinholdtsen

2014-01-31 13:29:32 UTC

While we wait to conclude if we should move to git or not, making me
unsure if I should commit to CVS or git, here is a small patch to fix
a bug discovered by Coverity.

The problem is that the value of rv, extracted from
serial_read_line(), is checked if it is negative. The unsigned long
value can never be negative, causing errors reported from
serial_read_line() to be ignored.

I propose to fix it by making the temp result variable local to the
scope where it is used, and having the same type as is returned from
the function used to give it a value. This patch should fix CID
1149026.

Index: src/plugins/serial/serial_terminal.c
===================================================================
RCS file: /cvsroot/ipmitool/ipmitool/src/plugins/serial/serial_terminal.c,v
retrieving revision 1.4
diff -u -3 -p -u -r1.4 serial_terminal.c
--- src/plugins/serial/serial_terminal.c 31 Jan 2014 06:13:58 -0000 1.4
+++ src/plugins/serial/serial_terminal.c 31 Jan 2014 13:25:09 -0000
@@ -362,12 +362,12 @@ recv_response(struct ipmi_intf * intf, u
{
char hex_rs[IPMI_SERIAL_MAX_RESPONSE * 3];
int i, j, resp_len = 0;
- unsigned long rv;
char *p, *pp;
char ch, str_hex[3];

p = hex_rs;
while (1) {
+ int rv;
if ((rv = serial_read_line(intf, p, sizeof(hex_rs) - resp_len)) < 0) {
/* error */
return -1;
@@ -400,7 +400,7 @@ recv_response(struct ipmi_intf * intf, u
sleep(1);
serial_flush(intf);
errno = 0;
- rv = strtoul(p + 4, &p, 16);
+ unsigned long rv = strtoul(p + 4, &p, 16);
if ((rv && rv < 0x100 && *p == '\0')
|| (rv == 0 && !errno)) {
/* The message didn't get it through. The upper

As soon as we conclude about cvs vs. git, I'll commit into the winning
repository. :)

--
Happy hacking
Petter Reinholdtsen

Zdenek Styblik

2014-01-31 14:08:16 UTC

Permalink

Post by Petter Reinholdtsen
While we wait to conclude if we should move to git or not, making me
unsure if I should commit to CVS or git, here is a small patch to fix
a bug discovered by Coverity.
The problem is that the value of rv, extracted from
serial_read_line(), is checked if it is negative. The unsigned long
value can never be negative, causing errors reported from
serial_read_line() to be ignored.
I propose to fix it by making the temp result variable local to the
scope where it is used, and having the same type as is returned from
the function used to give it a value. This patch should fix CID
1149026.
Index: src/plugins/serial/serial_terminal.c
===================================================================
RCS file: /cvsroot/ipmitool/ipmitool/src/plugins/serial/serial_terminal.c,v
retrieving revision 1.4
diff -u -3 -p -u -r1.4 serial_terminal.c
--- src/plugins/serial/serial_terminal.c 31 Jan 2014 06:13:58 -0000 1.4
+++ src/plugins/serial/serial_terminal.c 31 Jan 2014 13:25:09 -0000
@@ -362,12 +362,12 @@ recv_response(struct ipmi_intf * intf, u
{
char hex_rs[IPMI_SERIAL_MAX_RESPONSE * 3];
int i, j, resp_len = 0;
- unsigned long rv;
char *p, *pp;
char ch, str_hex[3];
p = hex_rs;
while (1) {
+ int rv;
if ((rv = serial_read_line(intf, p, sizeof(hex_rs) - resp_len)) < 0) {
/* error */
return -1;
@@ -400,7 +400,7 @@ recv_response(struct ipmi_intf * intf, u
sleep(1);
serial_flush(intf);
errno = 0;
- rv = strtoul(p + 4, &p, 16);
+ unsigned long rv = strtoul(p + 4, &p, 16);
if ((rv && rv < 0x100 && *p == '\0')
|| (rv == 0 && !errno)) {
/* The message didn't get it through. The upper

Either I'm missing something, or your patch is wrong. Either way, I
really don't like your approach to this problem. I'm sorry, but this
is fugly.

Z.

Post by Petter Reinholdtsen
As soon as we conclude about cvs vs. git, I'll commit into the winning
repository. :)
--
Happy hacking
Petter Reinholdtsen
------------------------------------------------------------------------------
WatchGuard Dimension instantly turns raw network data into actionable
security intelligence. It gives you real-time visual feedback on key
security issues and trends. Skip the complicated setup - simply import
a virtual appliance and go from zero to informed in seconds.
http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

Zdenek Styblik

2014-01-31 14:10:48 UTC

Permalink

On Fri, Jan 31, 2014 at 3:08 PM, Zdenek Styblik

Post by Zdenek Styblik

Either I'm missing something, or your patch is wrong. Either way, I
really don't like your approach to this problem. I'm sorry, but this
is fugly.
Z.

Please, just declare new variable, be it rv_readline, with type of int
and don't do things which cause a sudden need to suffocate myself with
a big black whooper ;)

Z.

Post by Zdenek Styblik

Petter Reinholdtsen

2014-01-31 14:17:08 UTC

Permalink

[Zdenek Styblik]

Post by Zdenek Styblik

Post by Zdenek Styblik
Either I'm missing something, or your patch is wrong. Either way, I
really don't like your approach to this problem. I'm sorry, but this
is fugly.

Please, just declare new variable, be it rv_readline, with type of int
and don't do things which cause a sudden need to suffocate myself with
a big black whooper ;)

Thank you for the feedback. Now we know you do not like the approach.
But based on what you wrote, it is not possible to understand what it
is with the approach you do not like. Can you tell us a bit more?

--
Happy hacking
Petter Reinholdtsen

Zdenek Styblik

2014-01-31 15:21:20 UTC

Permalink

Post by Petter Reinholdtsen
[Zdenek Styblik]

Post by Zdenek Styblik

Post by Zdenek Styblik
Either I'm missing something, or your patch is wrong. Either way, I
really don't like your approach to this problem. I'm sorry, but this
is fugly.

Please, just declare new variable, be it rv_readline, with type of int
and don't do things which cause a sudden need to suffocate myself with
a big black whooper ;)

Sure. Not only you're reusing variable name, you're declaring it deep
inside the function/code. So no.

Z.

Post by Petter Reinholdtsen
--
Happy hacking
Petter Reinholdtsen

------------------------------------------------------------------------------

Post by Petter Reinholdtsen
WatchGuard Dimension instantly turns raw network data into actionable
security intelligence. It gives you real-time visual feedback on key
security issues and trends. Skip the complicated setup - simply import
a virtual appliance and go from zero to informed in seconds.

http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk

Post by Petter Reinholdtsen
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

Petter Reinholdtsen

2014-01-31 15:55:11 UTC

Permalink

[Zdenek Styblik]

Post by Zdenek Styblik
Sure. Not only you're reusing variable name, you're declaring it
deep inside the function/code. So no.

Right. Then I understand what you mean.

Declaring variables close to where they are used is normally
considered good coding practice, not a problem. Or as
<URL:http://www.lrdev.com/lr/c/ccgl.html#scopevariable> put it: "The
variable scope should be as small as possible". There is no overhead
in using variables in code blocks, so there is really do downside to
keeping the scope of variables small.

Why do you believe variables should be given function scope instead of
block scope, when the variable is only used in a small block in a
function?

The variable in question is the return value, rv for short, and given
that the scope is limited to the few lines, it seemed to me that it
gave more consistent naming of variables by using 'rv' for return
value both places, but I realise that is a matter of opinion . I am
happy to give return values different names. :)

--
Happy hacking
Petter Reinholdtsen

Zdenek Styblik

2014-02-01 06:14:21 UTC

Permalink

Post by Petter Reinholdtsen
[Zdenek Styblik]

Post by Zdenek Styblik
Sure. Not only you're reusing variable name, you're declaring it
deep inside the function/code. So no.

Right. Then I understand what you mean.
Declaring variables close to where they are used is normally
considered good coding practice, not a problem. Or as
<URL:http://www.lrdev.com/lr/c/ccgl.html#scopevariable> put it: "The
variable scope should be as small as possible". There is no overhead
in using variables in code blocks, so there is really do downside to
keeping the scope of variables small.
Why do you believe variables should be given function scope instead of
block scope, when the variable is only used in a small block in a
function?
The variable in question is the return value, rv for short, and given
that the scope is limited to the few lines, it seemed to me that it
gave more consistent naming of variables by using 'rv' for return
value both places, but I realise that is a matter of opinion . I am
happy to give return values different names. :)

Scope, sure. But I'll be damned if what you're trying to sell, that is
to re-use variable name with different data types, is good practice.
But hey, it's your commit. I'm sure you know better.

Z.

Post by Petter Reinholdtsen
--
Happy hacking
Petter Reinholdtsen
------------------------------------------------------------------------------
WatchGuard Dimension instantly turns raw network data into actionable
security intelligence. It gives you real-time visual feedback on key
security issues and trends. Skip the complicated setup - simply import
a virtual appliance and go from zero to informed in seconds.
http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel

Dmitry Bazhenov

2014-01-31 14:51:14 UTC

Permalink

Hello, Zdenek, Petter,

The problem is resolved simply by changing rv to signed value.

Index: serial_terminal.c
===================================================================
--- serial_terminal.c (revision 170)
+++ serial_terminal.c (working copy)
@@ -345,7 +345,7 @@
{
char hex_rs[IPMI_SERIAL_MAX_RESPONSE * 3];
int i, j, resp_len = 0;
- unsigned long rv;
+ long rv;
char *p, *pp;
char ch, str_hex[3];

Regards,
Dmitry

Post by Zdenek Styblik
On Fri, Jan 31, 2014 at 3:08 PM, Zdenek Styblik

Post by Zdenek Styblik

Either I'm missing something, or your patch is wrong. Either way, I
really don't like your approach to this problem. I'm sorry, but this
is fugly.
Z.

Please, just declare new variable, be it rv_readline, with type of int
and don't do things which cause a sudden need to suffocate myself with
a big black whooper ;)
Z.

Post by Zdenek Styblik

------------------------------------------------------------------------------
WatchGuard Dimension instantly turns raw network data into actionable
security intelligence. It gives you real-time visual feedback on key
security issues and trends. Skip the complicated setup - simply import
a virtual appliance and go from zero to informed in seconds.
http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel