Skip to content

fix(photon-targeting): correct off-by-one bounds checks in Packet decode methods #2528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

fix(photon-targeting): correct off-by-one bounds checks in Packet decode methods #2528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions photon-targeting/src/main/java/org/photonvision/common/dataflow/structures/Packet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ public <T extends PhotonStructSerializable<T>> void encodeOptional(Optional<T> d
* @return A decoded byte from the packet.
*/
public byte decodeByte() {
if (packetData.length < readPos) {
if (packetData.length < readPos + 1) {
return '\0';
}
return packetData[readPos++];
Expand All @@ -303,7 +303,7 @@ public byte decodeByte() {
* @return A decoded int from the packet.
*/
public int decodeInt() {
if (packetData.length < readPos + 3) {
if (packetData.length < readPos + 4) {
return 0;
}
return (0xff & packetData[readPos++])
Expand All @@ -313,7 +313,7 @@ public int decodeInt() {
}

public long decodeLong() {
if (packetData.length < (readPos + 7)) {
if (packetData.length < (readPos + 8)) {
return 0;
}
long data =
Expand All @@ -335,7 +335,7 @@ public long decodeLong() {
* @return A decoded double from the packet.
*/
public double decodeDouble() {
if (packetData.length < (readPos + 7)) {
if (packetData.length < (readPos + 8)) {
return 0;
}
long data =
Expand All @@ -357,7 +357,7 @@ public double decodeDouble() {
* @return A decoded float from the packet.
*/
public float decodeFloat() {
if (packetData.length < (readPos + 3)) {
if (packetData.length < (readPos + 4)) {
return 0;
}

Expand All @@ -375,7 +375,7 @@ public float decodeFloat() {
* @return A decoded boolean from the packet.
*/
public boolean decodeBoolean() {
if (packetData.length < readPos) {
if (packetData.length < readPos + 1) {
return false;
}
return packetData[readPos++] == 1;
Expand All @@ -396,7 +396,7 @@ public double[] decodeDoubleArray(int len) {
}

public short decodeShort() {
if (packetData.length < readPos + 1) {
if (packetData.length < readPos + 2) {
return 0;
}
return (short) ((0xff & packetData[readPos++]) | (0xff & packetData[readPos++]) << 8);
Expand Down
88 changes: 88 additions & 0 deletions photon-targeting/src/test/java/org/photonvision/PacketTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,94 @@
import org.wpilib.math.geometry.*;

class PacketTest {
@Test
public void testDecodeByteAtBoundary() {
// Exactly enough data must decode
var packet = new Packet(new byte[] {0x12});
assertEquals((byte) 0x12, packet.decodeByte());

// One byte short must return the default instead of reading out of bounds
var shortPacket = new Packet(new byte[] {});
assertEquals((byte) 0, shortPacket.decodeByte());
}

@Test
public void testDecodeBooleanAtBoundary() {
var packet = new Packet(new byte[] {1});
assertEquals(true, packet.decodeBoolean());

var shortPacket = new Packet(new byte[] {});
assertEquals(false, shortPacket.decodeBoolean());
}

@Test
public void testDecodeShortAtBoundary() {
var packet = new Packet(new byte[] {0x34, 0x12});
assertEquals((short) 0x1234, packet.decodeShort());

var shortPacket = new Packet(new byte[] {0x34});
assertEquals((short) 0, shortPacket.decodeShort());
}

@Test
public void testDecodeIntAtBoundary() {
var packet = new Packet(new byte[] {0x78, 0x56, 0x34, 0x12});
assertEquals(0x12345678, packet.decodeInt());

var shortPacket = new Packet(new byte[] {0x78, 0x56, 0x34});
assertEquals(0, shortPacket.decodeInt());
}

@Test
public void testDecodeLongAtBoundary() {
var packet = new Packet(new byte[] {0x21, 0, 0, 0, 0, 0, 0, 0});
assertEquals(0x21L, packet.decodeLong());

var shortPacket = new Packet(new byte[] {0x21, 0, 0, 0, 0, 0, 0});
assertEquals(0L, shortPacket.decodeLong());
}

@Test
public void testDecodeDoubleAtBoundary() {
long bits = Double.doubleToLongBits(42.5);
var data = new byte[8];
for (int i = 0; i < 8; i++) {
data[i] = (byte) (bits >> (8 * i));
}
var packet = new Packet(data);
assertEquals(42.5, packet.decodeDouble());

var shortPacket = new Packet(java.util.Arrays.copyOfRange(data, 0, 7));
assertEquals(0.0, shortPacket.decodeDouble());
}

@Test
public void testDecodeFloatAtBoundary() {
int bits = Float.floatToIntBits(3.25f);
var data = new byte[4];
for (int i = 0; i < 4; i++) {
data[i] = (byte) (bits >> (8 * i));
}
var packet = new Packet(data);
assertEquals(3.25f, packet.decodeFloat());

var shortPacket = new Packet(java.util.Arrays.copyOfRange(data, 0, 3));
assertEquals(0.0f, shortPacket.decodeFloat());
}

@Test
public void testDecodeAtBoundaryWithNonzeroReadPos() {
// 8 bytes: two ints back to back; the second decode starts at readPos 4
var packet = new Packet(new byte[] {1, 0, 0, 0, 2, 0, 0, 0});
assertEquals(1, packet.decodeInt());
assertEquals(2, packet.decodeInt());

// 7 bytes: the second int decode must underflow and return the default
var shortPacket = new Packet(new byte[] {1, 0, 0, 0, 2, 0, 0});
assertEquals(1, shortPacket.decodeInt());
assertEquals(0, shortPacket.decodeInt());
}

@Test
public void testTargetCorner() {
TargetCorner corner = new TargetCorner(1, 2);
Expand Down
Loading