search

P1sec / pycrate

A Python library to ease the development of encoders and decoders for various protocols and file formats; contains ASN.1 and CSN.1 compilers.
GNU Lesser General Public License v2.1
381 stars 132 forks source link

Fixed hardcoded TBCD filler byte in BufBCD.decode() #136

Closed domi007 closed 3 years ago

domi007 commented 3 years ago

When working with TBCD using a filler byte different than 0xF I came across this little bug, which was caused by using a hardcoded value instead of the _filler variable. Sorry for the polluted pull request, hope it could still be merged, or at least the change could be adopted from it in a new commit.

Kind regards, Domi

p1-bmu commented 3 years ago

Hi Domi, thanks for pointing this out. We must be careful however, as this BufBCD structure is used in many places in pycrate: ./pycrate_mobile/ISUP.py ./pycrate_mobile/SCCP.py ./pycrate_mobile/TS23040_SMS.py ./pycrate_mobile/TS24008_IE.py ./pycrate_mobile/TS24011_PPSMS.py ./pycrate_mobile/TS24301_IE.py ./pycrate_mobile/TS24501_IE.py ./pycrate_mobile/TS29002_MAPIE.py ./pycrate_mobile/TS29274_GTPC.py

And I know that at least in SCCP and ISUP, the filler value for BCD numbers is 0x0. This does not not look very smart at first, but ITU-T specs have hopefully an external indicator that says if the BCD number is odd or even. With your patch, this will break the decoding of SCCP / ISUP numbers (such as in Global Title) which have a 0 value inside. I need to investigate this a little bit more. Maybe another solution would be to set another class attribute such as _stop which is used for exiting the decoding routine, where we keep the _filler for encoding. I am curious about your case which requires this change, can you tell me more ?

domi007 commented 3 years ago

Hi Benoit,

I failed to realize earlier I could do this:

sc["CallingPartyAddr"]["Value"]["GT"].get_alt().get_addr()

So in all my previous attempts I kept seeing the ending zero in my SCCP GTs, so I thought it is a decoding issue. Now I also see that my change would break numbers with zeros. I'll close this request and revert my changes on my own codebase as well. And yes, it is a weird choice by ITU to use 0x0 as a filler :-)