[Ipmitool-devel] [PATCH 1/4] ipmi_intf_socket_connect fails with IPv4 hosts

[Anton Blanchard]

A recent IPv6 patch broke IPv4 connections. Fix the incorrect
conditional to get it going again.
---
src/plugins/ipmi_intf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/plugins/ipmi_intf.c b/src/plugins/ipmi_intf.c
index 1ea6fbe..579cf02 100644
--- a/src/plugins/ipmi_intf.c
+++ b/src/plugins/ipmi_intf.c
@@ -382,7 +382,7 @@ ipmi_intf_socket_connect(struct ipmi_intf * intf)
session->ai_family = AF_UNSPEC;
for (rp = rp0; rp != NULL; rp = rp->ai_next) {
/* We are only interested in IPv4 and IPv6 */
- if ((rp->ai_family != AF_INET6) && (rp->ai_family == AF_INET))
+ if ((rp->ai_family != AF_INET6) && (rp->ai_family != AF_INET))
continue;

intf->fd = socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol);

[Anton Blanchard]

Check the return code of ipmi_main_intf->open(), and take the
error path if it fails. Right now we continue on blindly which
results in a SEGV.
---
lib/ipmi_main.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/ipmi_main.c b/lib/ipmi_main.c
index 54b80c0..81c64bf 100644
--- a/lib/ipmi_main.c
+++ b/lib/ipmi_main.c
@@ -894,8 +894,11 @@ ipmi_main(int argc, char ** argv,

/* Open the interface with the specified or default IPMB address */
ipmi_main_intf->my_addr = arg_addr ? arg_addr : IPMI_BMC_SLAVE_ADDR;
- if (ipmi_main_intf->open != NULL)
- ipmi_main_intf->open(ipmi_main_intf);
+
+ if (ipmi_main_intf->open != NULL) {
+ if (ipmi_main_intf->open(ipmi_main_intf) < 0)
+ goto out_free;
+ }

/*
* Attempt picmg discovery of the actual interface address unless

[Anton Blanchard]

If ipmi_lanplus_send_payload fails we get a NULL pointer
returned. Error out straight away instead of continuing on
and getting a SEGV when we dereference rsp.
---
src/plugins/lanplus/lanplus.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/src/plugins/lanplus/lanplus.c b/src/plugins/lanplus/lanplus.c
index 1f8169e..9ce9945 100644
--- a/src/plugins/lanplus/lanplus.c
+++ b/src/plugins/lanplus/lanplus.c
@@ -2859,6 +2859,12 @@ ipmi_lanplus_open_session(struct ipmi_intf * intf)
free(msg);
msg = NULL;

+ if (!rsp)
+ {
+ lprintf(LOG_WARNING, "Error sending open session message\n");
+ return -1;
+ }
+
if (verbose)
lanplus_dump_open_session_response(rsp);

[Zdenek Styblik]

Post by Anton Blanchard
If ipmi_lanplus_send_payload fails we get a NULL pointer
returned. Error out straight away instead of continuing on
and getting a SEGV when we dereference rsp.
---
src/plugins/lanplus/lanplus.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/plugins/lanplus/lanplus.c b/src/plugins/lanplus/lanplus.c
index 1f8169e..9ce9945 100644
--- a/src/plugins/lanplus/lanplus.c
+++ b/src/plugins/lanplus/lanplus.c
@@ -2859,6 +2859,12 @@ ipmi_lanplus_open_session(struct ipmi_intf * intf)
free(msg);
msg = NULL;
+ if (!rsp)
+ {
+ lprintf(LOG_WARNING, "Error sending open session message\n");
+ return -1;
+ }
+
if (verbose)
lanplus_dump_open_session_response(rsp);
--
1.8.3.2

Hello Anton,

I wonder, is there really a LOG_WARNING? And either way, it should be
LOG_ERR and no '\n' at the end. It makes me wonder whether you've
tested these changes.

Thanks,
Z.

[Anton Blanchard]

Hi Zdenek,

Post by Zdenek Styblik
I wonder, is there really a LOG_WARNING? And either way, it should be
LOG_ERR and no '\n' at the end. It makes me wonder whether you've
tested these changes.

The code was tested. There are 0 occurences of LOG_WARN and 7 of
LOG_WARNING in src/plugins/lanplus/lanplus.c.

Also, 3 lines down from my patch we see:

lprintf(LOG_WARNING, "Error in open session response
message : %s\n",

I used this as the basis of my patch. All of your concerns are here
to be seen. It sounds like there is a lot of cleanup to do in this
plugin if indeed all of this is incorrect.

Anton
--

Fix SEGV in ipmi_lanplus_open_session

If ipmi_lanplus_send_payload fails we get a NULL pointer
returned. Error out straight away instead of continuing on
and getting a SEGV when we dereference rsp.

diff --git a/src/plugins/lanplus/lanplus.c b/src/plugins/lanplus/lanplus.c
index 1f8169e..0955b11 100644
--- a/src/plugins/lanplus/lanplus.c
+++ b/src/plugins/lanplus/lanplus.c
@@ -2859,6 +2859,12 @@ ipmi_lanplus_open_session(struct ipmi_intf * intf)
free(msg);
msg = NULL;

+ if (!rsp)
+ {
+ lprintf(LOG_ERR, "Error sending open session message");
+ return -1;
+ }
+
if (verbose)
lanplus_dump_open_session_response(rsp);

[Zdenek Styblik]

On Fri, Nov 22, 2013 at 6:34 AM, Anton Blanchard <***@samba.org> wrote:
[...]

Post by Anton Blanchard
The code was tested. There are 0 occurences of LOG_WARN and 7 of
LOG_WARNING in src/plugins/lanplus/lanplus.c.

I see. My bad then(it still should have been LOG_ERR, though).

Post by Anton Blanchard
lprintf(LOG_WARNING, "Error in open session response
message : %s\n",

Yup, this is wrong. lprintf() doesn't need '\n' unless you really want
double '\n' as a result.

Post by Anton Blanchard
I used this as the basis of my patch. All of your concerns are here
to be seen. It sounds like there is a lot of cleanup to do in this
plugin if indeed all of this is incorrect.

There is a lots of cleaning up to do around ipmitool.

One more thing. Send patches as attachments. Since these are
"one-lines" they're easy to pick up. But I don't want to imagine doing
that with larger and more complex patch.

Z.

Post by Anton Blanchard
Anton
--
Fix SEGV in ipmi_lanplus_open_session
If ipmi_lanplus_send_payload fails we get a NULL pointer
returned. Error out straight away instead of continuing on
and getting a SEGV when we dereference rsp.
diff --git a/src/plugins/lanplus/lanplus.c b/src/plugins/lanplus/lanplus.c
index 1f8169e..0955b11 100644
--- a/src/plugins/lanplus/lanplus.c
+++ b/src/plugins/lanplus/lanplus.c
@@ -2859,6 +2859,12 @@ ipmi_lanplus_open_session(struct ipmi_intf * intf)
free(msg);
msg = NULL;
+ if (!rsp)
+ {
+ lprintf(LOG_ERR, "Error sending open session message");
+ return -1;
+ }
+
if (verbose)
lanplus_dump_open_session_response(rsp);

[Anton Blanchard]

If we have to retry an open session request, we hit an assert
that assumes we can only be in LANPLUS_STATE_PRESESSION state.

Add LANPLUS_STATE_OPEN_SESSION_SENT so we don't abort if
we retry.
---
src/plugins/lanplus/lanplus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/plugins/lanplus/lanplus.c b/src/plugins/lanplus/lanplus.c
index 9ce9945..23ded05 100644
--- a/src/plugins/lanplus/lanplus.c
+++ b/src/plugins/lanplus/lanplus.c
@@ -2174,7 +2174,7 @@ ipmi_lanplus_send_payload(
else if (payload->payload_type == IPMI_PAYLOAD_TYPE_RMCP_OPEN_REQUEST)
{
lprintf(LOG_DEBUG, ">> SENDING AN OPEN SESSION REQUEST\n");
- assert(session->v2_data.session_state == LANPLUS_STATE_PRESESSION);
+ assert(session->v2_data.session_state == LANPLUS_STATE_PRESESSION || session->v2_data.session_state == LANPLUS_STATE_OPEN_SESSION_SENT);

ipmi_lanplus_build_v2x_msg(intf, /* in */
payload, /* in */

[Liebig, Holger]

Hi Anton,
thanks for catching this. I

Holger

-----Original Message-----
Sent: Friday, November 22, 2013 1:35 AM
Subject: [Ipmitool-devel] [PATCH 1/4] ipmi_intf_socket_connect fails with
IPv4 hosts
A recent IPv6 patch broke IPv4 connections. Fix the incorrect conditional to
get it going again.
---
src/plugins/ipmi_intf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/plugins/ipmi_intf.c b/src/plugins/ipmi_intf.c index
1ea6fbe..579cf02 100644
--- a/src/plugins/ipmi_intf.c
+++ b/src/plugins/ipmi_intf.c
@@ -382,7 +382,7 @@ ipmi_intf_socket_connect(struct ipmi_intf * intf)
session->ai_family = AF_UNSPEC;
for (rp = rp0; rp != NULL; rp = rp->ai_next) {
/* We are only interested in IPv4 and IPv6 */
- if ((rp->ai_family != AF_INET6) && (rp->ai_family ==
AF_INET))
+ if ((rp->ai_family != AF_INET6) && (rp->ai_family !=
AF_INET))
continue;
intf->fd = socket(rp->ai_family, rp->ai_socktype, rp-

Post by Anton Blanchard
ai_protocol);

--
1.8.3.2
------------------------------------------------------------------------------
Shape the Mobile Experience: Free Subscription
Software experts and developers: Be at the forefront of tech innovation.
Intel(R) Software Adrenaline delivers strategic insight and game-changing
conversations that shape the rapidly evolving mobile landscape. Sign up
now.
http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.cl
ktrk
_______________________________________________
Ipmitool-devel mailing list
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel