vetss
commented
8 years ago Hello @AlerantAppNGIN ,
I have checked your comments, thank for your remarks.
My comments:
a) avoiding of decoding of 5b byte for "informationTransferCapability == _ITS_3_1_KHZ" I think reasonable. I do not have any wireshark data / experiense and this topic and think that your aproach is better.
b) I have added fields private boolean byte5aIsPresent; private boolean byte5bIsPresent; private boolean byte5cIsPresent; private boolean byte5dIsPresent; that will indicate that we have those fields decoded (need to encode). Default values are "false" so we will not encode them by default. This changes the current behaviour but I do not know the better solution for this complicated primitive
c) I also added data[] field for source encoded value. It is filled when decoding. If we set field before encoding the ISUP stack will use "data" field but ignore other values.
This is my update, clease check it. I have chenged interfaces and implementation but have not worked for tests: https://github.com/vetss/jss7/commit/162fe763d23ac5be9f719f5ad5dc1e8ec753fb6e I have not committed this into master and netty-2 branches so far.
Please check this update and if you think that the update covers all bugs I will commit into a netty-2 branches.
We also need to update of unit tests. @AlerantAppNGIN if you can provide any tests I will apretiate it.
PS: for enyone who has extra info for encoding / decoding of this primitive or has live trace data, please provide it.
T-REC-Q.931-199805-I!!PDF-E.pdf userserviceinfo_test.txt userserviceinfo_test_result.txt
Hello,
I’d like to report and discuss some errors found in the encoding/decoding of the ISUP UserServiceInformation message. I attached a copy of the ITU Q.931 standard. I’ll refer to chapter 4.5.5 „Bearer capability” (page 58). I also attached a text file containing a simple test method (userserviceinfo_test.txt) and another text file containing the test results (userserviceinfo_test_result.txt). One way to run the test is to add the main method to the UserServiceInformationTest class then run the file as a Java application. The values used in the tests were all extracted from real network traffic. The first issue I’d like to mention is in the „public int decode(byte[] b)” method of the UserServiceInformationBaseImpl class starting from line number 256: … // 5b - TODO: not sure that this is a correct decoding v = b[index++]; this.intermediateRate = (v >> 5) & 0x3; this.nicOnTx = (v >> 4) & 0x1; this.nicOnRx = (v >> 3) & 0x1; this.fcOnTx = (v >> 2) & 0x1; this.fcOnRx = (v >> 1) & 0x1; extBit = v & 0x80; … Here the program is decoding a message that has a l1userinformation field with a value of „G.711 A-law ” and the information transfer rate is „3.1kHz audio”. The 5a part of the message is already decoded. The value of extBit of the 5a element is zero which tells that there are more extensions coming. If you look into the Q.931 documentation it tells that 5b part shall only present in cases covered by Note-3 and Note-4, neither of which matches this case. Therefore I think that the above program logic results in bad decoding. For example in the test ‘90902360063b80’, octets 5a,5c and 5d are present but the program logic parses these values as octets 5a, 5b and 5c. My second remark is about encoding. The current logic encodes every possible message part. It also encodes ones that may not have been present in the original message. If you look into the attached test result you can find the following test case: The original message was ’9090a3’ which should be decoded this way: ’90’ – octet 3 - ITU-T standardized coding, Speech ’90’ – octet 4 - 3.1kHz audio ’a3’ - octet 5 - ext=1, G.711 A-law Here ext=1 means that none of the optional parts of octet 5 are present. However, when this decoded message was re-encoded using JSs7 the result was the following: ’90902300000080’. You can see that it encoded message parts 5a, 5b, 5c, 5d which I think has a different meaning that the original message has. I recommend that upon decoding the UserServiceInformation object it should track which optional message part was present in the message so when it encodes itself it is possible to recreate the same message as the original was.
In addition to this I also suggest to keep the original octets too. One reason for this is that it is possible to have elements of the message that can only be interpreted by the user of the stack. One example of this is the 5d message part which has a value range reserved for user specified information. This value couldn’t be decoded by JSs7 but its value has to be stored, be accessible by the user and should be re-encoded without losing information. The same considerations should be applied to the XML serialization and deserialization too.
On the AppNGIN fork we use an alternative implementation of UserServiceInformation which unfortunately has diverged from the Restcomm version so much that I couldn’t really merge it and send a solution proposal for this issue. Moreover I think that this issue is complex enough to discuss how it should be handled before we come up with a solution which does not fit into the Jss7 design. Please review my remarks and if you find them valid then let’s discuss how we should address them.
Thanks, Gabor