Hi Holger,
I'm getting back to the following libosmocore commit introduced in 2015:
commit f558ed4bb9c0f00997b8f97c2b251a574c1a64c4 Author: Holger Hans Peter Freyther holger@moiji-mobile.com Date: Tue Jun 2 15:52:06 2015 +0200
I can see what you are doing, but I have absolutely no idea as to why.
AFAICT, the IPA CCM ID TLVs have the following structure:
* 16bit length field * one byte tag * optional payload
The length field *includes* the tag, so the actual payload length is the value encoded in the length field minus one.
This means that the existing/classic ipa_ccm_idtag_parse() always returns one byte too much for the length of the IPA payload. I'm trying to address this in https://gerrit.osmocom.org/#/c/libosmocore/+/10216/
Your commit introduces ipa_ccm_idtag_parse_off(), which introduces a noffset field. However, that offset is used not only to compute the actual "payload" size, but it's also used for computing the subsequent CCM information elements. Hence, I cannot use any non-zero offset to parse a CCM blob.
I also don't see any of our code using the ipa_ccm_idtag_parse_off() function, except the test case - where the test case seems to use a different encoding as seen on the wire, i.e. it uses only a single-byte length field.
So if the function was just added for that test case, why not structure the data used in the test to reflect the on-the-wire protocol reality?
There must be some genius rationale behind it, but I'm unable to figure it out.
Maybe you still remember? Thanks!
On 29. Jul 2018, at 10:33, Harald Welte laforge@gnumonks.org wrote:
I'm getting back to the following libosmocore commit introduced in 2015:
commit f558ed4bb9c0f00997b8f97c2b251a574c1a64c4 Author: Holger Hans Peter Freyther holger@moiji-mobile.com Date: Tue Jun 2 15:52:06 2015 +0200
I can see what you are doing, but I have absolutely no idea as to why.
So if the function was just added for that test case, why not structure the data used in the test to reflect the on-the-wire protocol reality?
There must be some genius rationale behind it, but I'm unable to figure it out.
Maybe you still remember? Thanks!
I might not have time until next week to replicate it (and my nanobts is not with me right now either). IIRC (my first memory but the commit message points me to it as well): I struggled a lot to parse ipaccess-find (IPAC_MSGT_ID_GET) from the nanoBTS.
It seemed it was disobeying a reasonable TLV structure and the closest I found back then seemed to have been this patch? Could you check if the testcase matches an ipaccess-find result?
I will try to find some next during the week to have a look.
holger
Hi Holger,
On Sun, Jul 29, 2018 at 11:11:13AM +0100, Holger Freyther wrote:
I might not have time until next week to replicate it (and my nanobts is not with me right now either). IIRC (my first memory but the commit message points me to it as well): I struggled a lot to parse ipaccess-find (IPAC_MSGT_ID_GET) from the nanoBTS.
Ah. ok. I've not looked at ipaccess-find/UDP in a long time. Only at the normal procedure at OML/RSL start-up.
It seemed it was disobeying a reasonable TLV structure and the closest I found back then seemed to have been this patch? Could you check if the testcase matches an ipaccess-find result?
Yes, I will check for that. The test case definitely does not match the IPA CCM seen inside OML/RSL from a nanoBTS, not even from the first traces I have from 2010.
I will try to find some next during the week to have a look.
I think the pointer to ipaccess-find was sufficient, thanks.
On 29. Jul 2018, at 11:51, Harald Welte laforge@gnumonks.org wrote:
Hi Holger,
I think the pointer to ipaccess-find was sufficient, thanks.
One more thing came to mind. It was when I implemented the sysmobts-mgr.
sysmobts_mgr_nl.c has this comment:
/* * The TLV structure in IPA messages in UDP packages is a bit * weird. First the header appears to have an extra NULL byte * and second the L16 of the L16TV needs to include +1 for the * tag. The default msgb/tlv and libosmo-abis routines do not * provide this. */
It is entirely possible that I missed something in the ipaccess protocol... it seemed weird (or an attribute of writing strings into there).
Hi Holger,
On Sun, Jul 29, 2018 at 12:51:47PM +0200, Harald Welte wrote:
On Sun, Jul 29, 2018 at 11:11:13AM +0100, Holger Freyther wrote:
It seemed it was disobeying a reasonable TLV structure and the closest I found back then seemed to have been this patch? Could you check if the testcase matches an ipaccess-find result?
Yes, I will check for that. The test case definitely does not match the IPA CCM seen inside OML/RSL from a nanoBTS, not even from the first traces I have from 2010.
I think the solution to the problem is that your test case parses the IDENTITY REQUESET format, which has 8bit length fields, while the function is normally (without the offset) used for the IDENTITY RESPONSE packets, where there are 16bit length fields.
See attached pcap file that I just created.
I'll look into this once I find time and make sure we have test cases for both, as well as fix the bug about the extraneous byte that my patch https://gerrit.osmocom.org/#/c/libosmocore/+/10216/ attempts to fix - and which is required to make external USSD entities in osmo-hlr work.
Regards, Harald
On 30. Jul 2018, at 21:03, Harald Welte laforge@gnumonks.org wrote:
Hi Holger,
Hey!
I think the solution to the problem is that your test case parses the IDENTITY REQUESET format, which has 8bit length fields, while the function is normally (without the offset) used for the IDENTITY RESPONSE packets, where there are 16bit length fields.
nothing genius in the end just poking in the blind trying to understand a binary protocol and getting it wrong... When looking at the traces I wondered if they got \0 termination of strings wrong and didn't consider that request/response would use different length fields. But 16bit length fields seem to make sense.
Nice find! We can clean this up in the osmo-bts code as well.
I'll look into this once I find time and make sure we have test cases for both, as well as fix the bug about the extraneous byte that my patch https://gerrit.osmocom.org/#/c/libosmocore/+/10216/ attempts to fix - and which is required to make external USSD entities in osmo-hlr work.
It looks like we can throw away the entire ipa_ccm_idtag_parse_off and time to create a TLV_TYPE_L16TV?
holger
-
Harald Welte -
Holger Freyther