There is a small bug in the AgentMessage. As exampled below, when checking bounds on integers and longs, the code checks the upper bound incorrectly. Instead of checking whether there is enough room in the byteArray to read an integer, it is one off, and will throw an error if there are any less than 5 bytes remaining (offset+4 > byteArrayLength-1). I believe that the intended functionality was to make sure that byteArray[offset:offset+4] can execute without throwing an error and not to ensure that there is always a single-byte buffer at the end of the array.
func getInteger(log logger.T, byteArray []byte, offset int) (result int32, err error) {
byteArrayLength := len(byteArray)
if offset > byteArrayLength-1 || offset+4 > byteArrayLength-1 || offset < 0 {
log.Error("getInteger failed: Offset is invalid.")
return 0, errors.New("Offset is bigger than the byte array.")
}
return bytesToInteger(log, byteArray[offset:offset+4])
}
Since the second argument in the slice is exclusive, byteArray[offset:offset+4] only reads the literal bytes from offset to offset+4-1. Therefore, offset+4-1 must be <= byteArrayLength-1 or, must not be offset+4-1 > byteArrayLength-1. This is logic is used in most other upper bound checks in the AgentMessage code, except for Integers and Longs.
Testing Fixes
I ended up only need to change a single test and added one to test the correct bounds:
The above test should not result in an error, if you took input[2:2+4] you would get a valid integer. Therefore, I changed this to assert.Nil(t, err). Secondly, I added a test to check the actual bounds:
This test does describe a situation where there are not enough bytes to read a full integer. input[3:3+4] would result in a panic.
Fixing Error Handling in Dezerialize
Perhaps this was a deliberate decision at the time, but the original code below shows a lack of error checking for the PayloadLength. I thought that there should be error reporting for PayloadLength, like there is for every other component. Then, the header error is conditioned on herr != nil yet prints and returns an err message instead. So, I changed the errs to herrs in that if statement.
agentMessage.PayloadLength, err = getUInteger(log, input, AgentMessage_PayloadLengthOffset)
headerLength, herr := getUInteger(log, input, AgentMessage_HLOffset)
if herr != nil {
log.Errorf("Could not deserialize field HeaderLength with error: %v", err)
return err
}
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Issue #, if available:
None
Description of changes:
Fixing Bounds Checking Logic
There is a small bug in the AgentMessage. As exampled below, when checking bounds on integers and longs, the code checks the upper bound incorrectly. Instead of checking whether there is enough room in the
byteArrayto read an integer, it is one off, and will throw an error if there are any less than 5 bytes remaining (offset+4 > byteArrayLength-1). I believe that the intended functionality was to make sure thatbyteArray[offset:offset+4]can execute without throwing an error and not to ensure that there is always a single-byte buffer at the end of the array.Since the second argument in the slice is exclusive,
byteArray[offset:offset+4]only reads the literal bytes fromoffsettooffset+4-1. Therefore,offset+4-1must be<= byteArrayLength-1or, must not beoffset+4-1 > byteArrayLength-1. This is logic is used in most other upper bound checks in the AgentMessage code, except for Integers and Longs.Testing Fixes
I ended up only need to change a single test and added one to test the correct bounds:
The above test should not result in an error, if you took
input[2:2+4]you would get a valid integer. Therefore, I changed this toassert.Nil(t, err). Secondly, I added a test to check the actual bounds:This test does describe a situation where there are not enough bytes to read a full integer.
input[3:3+4]would result in a panic.Fixing Error Handling in Dezerialize
Perhaps this was a deliberate decision at the time, but the original code below shows a lack of error checking for the PayloadLength. I thought that there should be error reporting for PayloadLength, like there is for every other component. Then, the header error is conditioned on
herr != nilyet prints and returns anerrmessage instead. So, I changed theerrs toherrs in that if statement.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.