Skip to content

Commit d8c09da

Browse files
joeygroverJulianKast
joeygrover
and
JulianKast
authored
Improve packet state machine (#1683)
* Make SdlPSM more robust Add more checks that will help the PSM churn through bad data to get back on track. Also add a try/catch block for each attempt to prevent any production crashes. * Overhaul SdlPsm unit tests and add more * Update base/src/main/java/com/smartdevicelink/transport/SdlPsm.java Co-authored-by: Julian Kast <Julian.kast@livio.io> Co-authored-by: Julian Kast <Julian.kast@livio.io>
1 parent 4aa6a62 commit d8c09da

2 files changed

Lines changed: 222 additions & 32 deletions

File tree

  • android/sdl_android/src/androidTest/java/com/smartdevicelink/test/transport
  • base/src/main/java/com/smartdevicelink/transport

‎android/sdl_android/src/androidTest/java/com/smartdevicelink/test/transport/SdlPsmTests.java‎

Lines changed: 170 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,17 @@
33
import android.util.Log;
44

55
import com.smartdevicelink.protocol.SdlPacket;
6+
import com.smartdevicelink.protocol.SdlPacketFactory;
67
import com.smartdevicelink.protocol.SdlProtocol;
8+
import com.smartdevicelink.protocol.enums.ControlFrameTags;
9+
import com.smartdevicelink.protocol.enums.SessionType;
710
import com.smartdevicelink.test.TestValues;
811
import com.smartdevicelink.transport.SdlPsm;
912

1013
import junit.framework.TestCase;
1114

15+
import org.junit.Assert;
16+
1217
import java.lang.reflect.Field;
1318
import java.lang.reflect.Method;
1419

@@ -18,12 +23,15 @@
1823
*/
1924
public class SdlPsmTests extends TestCase {
2025
private static final String TAG = "SdlPsmTests";
21-
private static final int MAX_DATA_LENGTH = SdlProtocol.V1_V2_MTU_SIZE - SdlProtocol.V1_HEADER_SIZE;
26+
private static final int MAX_DATA_LENGTH_V1 = SdlProtocol.V1_V2_MTU_SIZE - SdlProtocol.V1_HEADER_SIZE;
27+
private static final int MAX_DATA_LENGTH_V2 = SdlProtocol.V1_V2_MTU_SIZE - SdlProtocol.V2_HEADER_SIZE;
2228
SdlPsm sdlPsm;
2329
Field frameType, dataLength, version, controlFrameInfo;
2430
Method transitionOnInput;
2531
byte rawByte = (byte) 0x0;
2632

33+
SdlPacket startServiceACK;
34+
2735
protected void setUp() throws Exception {
2836
super.setUp();
2937
sdlPsm = new SdlPsm();
@@ -38,37 +46,91 @@ protected void setUp() throws Exception {
3846
dataLength.setAccessible(true);
3947
version.setAccessible(true);
4048
controlFrameInfo.setAccessible(true);
49+
50+
startServiceACK = SdlPacketFactory.createStartSessionACK(SessionType.RPC, (byte) 0x01, (byte) 0x05, (byte) 0x05);
51+
startServiceACK.putTag(ControlFrameTags.RPC.StartServiceACK.HASH_ID, "3bb34978fe3a");
52+
startServiceACK.putTag(ControlFrameTags.RPC.StartServiceACK.MTU, "150000");
53+
startServiceACK.putTag(ControlFrameTags.RPC.StartServiceACK.PROTOCOL_VERSION, "5.1.0");
4154
}
4255

4356

57+
public void testHappyPath() {
58+
59+
60+
byte[] packetBytes = startServiceACK.constructPacket();
61+
62+
SdlPsm sdlPsm = new SdlPsm();
63+
boolean didTransition = false;
64+
65+
for (byte packetByte : packetBytes) {
66+
didTransition = sdlPsm.handleByte(packetByte);
67+
assertTrue(didTransition);
68+
}
69+
70+
assertEquals(SdlPsm.FINISHED_STATE, sdlPsm.getState());
71+
SdlPacket parsedPacket = sdlPsm.getFormedPacket();
72+
assertNotNull(parsedPacket);
73+
74+
assertEquals(startServiceACK.getVersion(), parsedPacket.getVersion());
75+
assertEquals(startServiceACK.getServiceType(), parsedPacket.getServiceType());
76+
assertEquals(startServiceACK.getFrameInfo(), parsedPacket.getFrameInfo());
77+
assertEquals(startServiceACK.getFrameType(), parsedPacket.getFrameType());
78+
assertEquals(startServiceACK.getDataSize(), parsedPacket.getDataSize());
79+
assertEquals(startServiceACK.getMessageId(), parsedPacket.getMessageId());
80+
81+
assertTrue(startServiceACK.getTag(ControlFrameTags.RPC.StartServiceACK.HASH_ID).equals(parsedPacket.getTag(ControlFrameTags.RPC.StartServiceACK.HASH_ID)));
82+
assertTrue(startServiceACK.getTag(ControlFrameTags.RPC.StartServiceACK.MTU).equals(parsedPacket.getTag(ControlFrameTags.RPC.StartServiceACK.MTU)));
83+
assertTrue(startServiceACK.getTag(ControlFrameTags.RPC.StartServiceACK.PROTOCOL_VERSION).equals(parsedPacket.getTag(ControlFrameTags.RPC.StartServiceACK.PROTOCOL_VERSION)));
84+
85+
}
86+
4487
public void testGarbledControlFrame() {
4588
try {
4689
rawByte = 0x0;
4790
version.set(sdlPsm, 1);
4891
controlFrameInfo.set(sdlPsm, SdlPacket.FRAME_INFO_START_SERVICE);
4992
frameType.set(sdlPsm, SdlPacket.FRAME_TYPE_CONTROL);
5093

51-
dataLength.set(sdlPsm, MAX_DATA_LENGTH + 1);
94+
dataLength.set(sdlPsm, MAX_DATA_LENGTH_V1 + 1);
5295
int STATE = (Integer) transitionOnInput.invoke(sdlPsm, rawByte, SdlPsm.DATA_SIZE_4_STATE);
5396

5497
assertEquals(TestValues.MATCH, SdlPsm.ERROR_STATE, STATE);
5598
} catch (Exception e) {
99+
Assert.fail(e.toString());
56100
Log.e(TAG, e.toString());
57101
}
58102
}
59103

60-
public void testMaximumControlFrame() {
104+
public void testMaximumControlFrameForVersion1() {
61105
try {
62106
rawByte = 0x0;
63107
version.set(sdlPsm, 1);
64108
controlFrameInfo.set(sdlPsm, SdlPacket.FRAME_INFO_START_SERVICE);
65109
frameType.set(sdlPsm, SdlPacket.FRAME_TYPE_CONTROL);
66110

67-
dataLength.set(sdlPsm, MAX_DATA_LENGTH);
111+
dataLength.set(sdlPsm, MAX_DATA_LENGTH_V1);
68112
int STATE = (Integer) transitionOnInput.invoke(sdlPsm, rawByte, SdlPsm.DATA_SIZE_4_STATE);
69113

70114
assertEquals(TestValues.MATCH, SdlPsm.DATA_PUMP_STATE, STATE);
71115
} catch (Exception e) {
116+
Assert.fail(e.toString());
117+
Log.e(TAG, e.toString());
118+
}
119+
}
120+
121+
public void testMaximumControlFrameForVersion2Plus() {
122+
try {
123+
rawByte = 0x0;
124+
version.set(sdlPsm, 2);
125+
controlFrameInfo.set(sdlPsm, SdlPacket.FRAME_INFO_START_SERVICE);
126+
frameType.set(sdlPsm, SdlPacket.FRAME_TYPE_CONTROL);
127+
128+
dataLength.set(sdlPsm, MAX_DATA_LENGTH_V2);
129+
int STATE = (Integer) transitionOnInput.invoke(sdlPsm, rawByte, SdlPsm.DATA_SIZE_4_STATE);
130+
131+
assertEquals(TestValues.MATCH, SdlPsm.MESSAGE_1_STATE, STATE);
132+
} catch (Exception e) {
133+
Assert.fail(e.toString());
72134
Log.e(TAG, e.toString());
73135
}
74136
}
@@ -80,14 +142,117 @@ public void testOutOfMemoryDS4() {
80142
frameType.set(sdlPsm, SdlPacket.FRAME_TYPE_SINGLE);
81143

82144
dataLength.set(sdlPsm, 2147483647);
83-
int STATE = (Integer) transitionOnInput.invoke(sdlPsm, rawByte, SdlPsm.DATA_SIZE_4_STATE);
145+
int STATE = (Integer) transitionOnInput.invoke(sdlPsm, rawByte, SdlPsm.MESSAGE_4_STATE);
84146

85147
assertEquals(TestValues.MATCH, SdlPsm.ERROR_STATE, STATE);
86148
} catch (Exception e) {
149+
Assert.fail(e.toString());
87150
Log.e(TAG, e.toString());
88151
}
89152
}
90153

154+
public void testNegativeDataSize() {
155+
byte[] packetBytes = startServiceACK.constructPacket();
156+
157+
SdlPsm sdlPsm = new SdlPsm();
158+
boolean didTransition = false;
159+
160+
for (byte packetByte : packetBytes) {
161+
int state = sdlPsm.getState();
162+
switch (state) {
163+
case SdlPsm.MESSAGE_4_STATE:
164+
didTransition = sdlPsm.handleByte(packetByte);
165+
assertFalse(didTransition);
166+
assertEquals(SdlPsm.ERROR_STATE, sdlPsm.getState());
167+
return;
168+
case SdlPsm.DATA_SIZE_1_STATE:
169+
case SdlPsm.DATA_SIZE_2_STATE:
170+
case SdlPsm.DATA_SIZE_3_STATE:
171+
case SdlPsm.DATA_SIZE_4_STATE:
172+
didTransition = sdlPsm.handleByte((byte) 0xFF);
173+
assertTrue(didTransition);
174+
break;
175+
default:
176+
didTransition = sdlPsm.handleByte(packetByte);
177+
assertTrue(didTransition);
178+
}
179+
}
180+
}
181+
182+
public void testIncorrectVersion() {
183+
SdlPacket startServiceACK = SdlPacketFactory.createStartSessionACK(SessionType.RPC, (byte) 0x01, (byte) 0x05, (byte) 0x06);
184+
startServiceACK.putTag(ControlFrameTags.RPC.StartServiceACK.HASH_ID, "3bb34978fe3a");
185+
startServiceACK.putTag(ControlFrameTags.RPC.StartServiceACK.MTU, "150000");
186+
startServiceACK.putTag(ControlFrameTags.RPC.StartServiceACK.PROTOCOL_VERSION, "5.1.0");
187+
byte[] packetBytes = startServiceACK.constructPacket();
188+
189+
SdlPsm sdlPsm = new SdlPsm();
190+
boolean didTransition = sdlPsm.handleByte(packetBytes[0]);
191+
assertFalse(didTransition);
192+
}
193+
194+
public void testIncorrectService() {
195+
196+
byte[] packetBytes = startServiceACK.constructPacket();
197+
198+
SdlPsm sdlPsm = new SdlPsm();
199+
boolean didTransition = false;
200+
201+
for (byte packetByte : packetBytes) {
202+
int state = sdlPsm.getState();
203+
switch (state) {
204+
case SdlPsm.SERVICE_TYPE_STATE:
205+
didTransition = sdlPsm.handleByte((byte) 0xFF);
206+
assertFalse(didTransition);
207+
assertEquals(SdlPsm.ERROR_STATE, sdlPsm.getState());
208+
return;
209+
default:
210+
didTransition = sdlPsm.handleByte(packetByte);
211+
assertTrue(didTransition);
212+
}
213+
}
214+
}
215+
216+
public void testRecovery() {
217+
byte[] packetBytes = startServiceACK.constructPacket();
218+
byte[] processingBytes = new byte[packetBytes.length + 15];
219+
220+
System.arraycopy(packetBytes, 10, processingBytes, 0, 15);
221+
System.arraycopy(packetBytes, 0, processingBytes, 15, packetBytes.length);
222+
223+
224+
SdlPsm sdlPsm = new SdlPsm();
225+
boolean didTransition = false;
226+
byte packetByte;
227+
int state = SdlPsm.START_STATE, i = 0, limit = 0;
228+
229+
while (state != SdlPsm.FINISHED_STATE && limit < 10) {
230+
231+
packetByte = processingBytes[i];
232+
didTransition = sdlPsm.handleByte(packetByte);
233+
state = sdlPsm.getState();
234+
if (!didTransition) {
235+
assertEquals(SdlPsm.ERROR_STATE, state);
236+
sdlPsm.reset();
237+
} else if (state == SdlPsm.FINISHED_STATE) {
238+
break;
239+
}
240+
241+
if (i == processingBytes.length - 1) {
242+
i = 0;
243+
limit++;
244+
} else {
245+
i++;
246+
}
247+
}
248+
249+
assertEquals(SdlPsm.FINISHED_STATE, sdlPsm.getState());
250+
SdlPacket parsedPacket = sdlPsm.getFormedPacket();
251+
assertNotNull(parsedPacket);
252+
253+
}
254+
255+
91256
protected void tearDown() throws Exception {
92257
super.tearDown();
93258
}

‎base/src/main/java/com/smartdevicelink/transport/SdlPsm.java‎

Lines changed: 52 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,18 @@
3232
package com.smartdevicelink.transport;
3333

3434
import com.smartdevicelink.protocol.SdlPacket;
35+
import com.smartdevicelink.util.DebugTool;
3536

3637
import static com.smartdevicelink.protocol.SdlProtocol.V1_HEADER_SIZE;
3738
import static com.smartdevicelink.protocol.SdlProtocol.V1_V2_MTU_SIZE;
39+
import static com.smartdevicelink.protocol.SdlProtocol.V2_HEADER_SIZE;
40+
import static com.smartdevicelink.protocol.SdlProtocol.V3_V4_MTU_SIZE;
3841

3942

4043
public class SdlPsm {
41-
//private static final String TAG = "Sdl PSM";
42-
//Each state represents the byte that should be incoming
44+
private static final String TAG = "Sdl PSM";
4345

46+
//Each state represents the byte that should be incoming
4447
public static final int START_STATE = 0x0;
4548
public static final int SERVICE_TYPE_STATE = 0x02;
4649
public static final int CONTROL_FRAME_INFO_STATE = 0x03;
@@ -83,8 +86,13 @@ public SdlPsm() {
8386
}
8487

8588
public boolean handleByte(byte data) {
86-
//Log.trace(TAG, data + " = incoming");
87-
state = transitionOnInput(data, state);
89+
try {
90+
state = transitionOnInput(data, state);
91+
} catch (Exception e) {
92+
DebugTool.logError(TAG, "Exception thrown while parsing byte - " + data, e);
93+
state = ERROR_STATE;
94+
return false;
95+
}
8896

8997
return state != ERROR_STATE;
9098
}
@@ -93,18 +101,11 @@ private int transitionOnInput(byte rawByte, int state) {
93101
switch (state) {
94102
case START_STATE:
95103
version = (rawByte & (byte) VERSION_MASK) >> 4;
96-
//Log.trace(TAG, "Version: " + version);
97-
if (version == 0) { //It should never be 0
98-
return ERROR_STATE;
99-
}
100104
encrypted = (1 == ((rawByte & (byte) ENCRYPTION_MASK) >> 3));
101-
102-
103105
frameType = rawByte & (byte) FRAME_TYPE_MASK;
104-
//Log.trace(TAG, rawByte + " = Frame Type: " + frameType);
105106

106-
if ((version < 1 || version > 5) //These are known versions supported by this library.
107-
&& frameType != SdlPacket.FRAME_TYPE_CONTROL) {
107+
if ((version < 1 || version > 5)) {
108+
//These are known versions supported by this library.
108109
return ERROR_STATE;
109110
}
110111

@@ -116,7 +117,16 @@ private int transitionOnInput(byte rawByte, int state) {
116117

117118
case SERVICE_TYPE_STATE:
118119
serviceType = (int) (rawByte & 0xFF);
119-
return CONTROL_FRAME_INFO_STATE;
120+
switch (serviceType) {
121+
case 0x00: //SessionType.CONTROL:
122+
case 0x07: //SessionType.RPC:
123+
case 0x0A: //SessionType.PCM (Audio):
124+
case 0x0B: //SessionType.NAV (Video):
125+
case 0x0F: //SessionType.BULK (Hybrid):
126+
return CONTROL_FRAME_INFO_STATE;
127+
default:
128+
return ERROR_STATE;
129+
}
120130

121131
case CONTROL_FRAME_INFO_STATE:
122132
controlFrameInfo = (int) (rawByte & 0xFF);
@@ -203,19 +213,34 @@ private int transitionOnInput(byte rawByte, int state) {
203213
default:
204214
return ERROR_STATE;
205215
}
206-
if (version == 1) { //Version 1 packets will not have message id's
207-
if (dataLength == 0) {
208-
return FINISHED_STATE; //We are done if we don't have any payload
209-
}
210-
if (dataLength <= V1_V2_MTU_SIZE - V1_HEADER_SIZE) { // sizes from protocol/WiProProtocol.java
211-
payload = new byte[dataLength];
212-
} else {
213-
return ERROR_STATE;
214-
}
215-
dumpSize = dataLength;
216-
return DATA_PUMP_STATE;
217-
} else {
218-
return MESSAGE_1_STATE;
216+
switch (version) {
217+
case 1:
218+
//Version 1 packets will not have message id's
219+
if (dataLength == 0) {
220+
return FINISHED_STATE; //We are done if we don't have any payload
221+
}
222+
if (dataLength <= V1_V2_MTU_SIZE - V1_HEADER_SIZE) { // sizes from SDL protocol
223+
payload = new byte[dataLength];
224+
} else {
225+
return ERROR_STATE;
226+
}
227+
dumpSize = dataLength;
228+
return DATA_PUMP_STATE;
229+
case 2:
230+
if (dataLength <= V1_V2_MTU_SIZE - V2_HEADER_SIZE) {
231+
return MESSAGE_1_STATE;
232+
} else {
233+
return ERROR_STATE;
234+
}
235+
case 3:
236+
case 4:
237+
if (dataLength <= V3_V4_MTU_SIZE - V2_HEADER_SIZE) {
238+
return MESSAGE_1_STATE;
239+
} else {
240+
return ERROR_STATE;
241+
}
242+
default:
243+
return MESSAGE_1_STATE;
219244
}
220245

221246
case MESSAGE_1_STATE:

0 commit comments

Comments
 (0)