Skip to content
This repository was archived by the owner on May 10, 2024. It is now read-only.

fix bug where multiline ipmitool response causes panic #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edaniszewski
Copy link

Using ipmitool (version 1.8.16) to control chassis identify will result in multiple lines of raw response to be returned

$ ipmitool -H 127.0.0.1 -U ADMIN -P ADMIN -I lanplus raw 0x00 0x04 0x01 0x00
 7f 00 00 90 4a 57 f8 fd 7f 00 00 b0 49 57 f8 fd
 7f 00 00 90 4a 57 f8 fd 7f 00 00 20 f1 b9 8a 8c
 55 00 00

The newline in this response is not handled, so when parsed/decoded, it panics - as shown by the regression tests.

--- FAIL: TestResponseFromString (0.00s)
panic: encoding/hex: odd length hex string [recovered]
	panic: encoding/hex: odd length hex string

goroutine 70 [running]:
testing.tRunner.func1(0xc4201f6000)
	/usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:711 +0x2d2
panic(0x12a4340, 0xc420076560)
	/usr/local/Cellar/go/1.9.1/libexec/src/runtime/panic.go:491 +0x283
github.com/vapor-ware/goipmi.rawDecode(0x130fa6d, 0x68, 0x130fa6d, 0x68, 0x12c3800)
	/Users/edaniszewski/go/src/github.com/vapor-ware/goipmi/tool.go:148 +0x18e
github.com/vapor-ware/goipmi.responseFromString(0x130fa6c, 0x69, 0x1456d80, 0xc4201b1461, 0xc42003af08, 0x1)
	/Users/edaniszewski/go/src/github.com/vapor-ware/goipmi/tool.go:138 +0x51
github.com/vapor-ware/goipmi.TestResponseFromString(0xc4201f6000)
	/Users/edaniszewski/go/src/github.com/vapor-ware/goipmi/tool_test.go:186 +0xd7
testing.tRunner(0xc4201f6000, 0x13135d0)
	/usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	/usr/local/Cellar/go/1.9.1/libexec/src/testing/testing.go:789 +0x2de
exit status 2

With these changes, newlines are stripped out of the response prior to further parsing/decoding, so newline-separated byte list gets handles as a continuous list of bytes. Tests should pass:

$ go test
PASS
ok  	github.com/vapor-ware/goipmi	0.075s

@vmwclabot
Copy link

@edaniszewski, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

Copy link
Contributor

@dougm dougm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @edaniszewski

We can merge once @vmwclabot approves.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants