Decoding process crashes when PDU length is odd
emilianodellacasa opened this issue · 21 comments
I have noticed that sometimes my GSM modem send me a PDU which length is odd.
This causes the library to crash, with following message:
File "/usr/lib/python2.6/site-packages/python_messaging-0.3.0-py2.6.egg/messaging/pdu.py", line 169, in decode_pdu
d = StringIO(bytes_to_str(unhexlify(pdu)))
TypeError: Odd-length string
Is it something related to my hardware or is it a bug? Hardware's technical support said PDU length can be odd and actually I can't find out anything on the internet saying that.
How may I resolve?
Hi,
We made some changes to detect and raise an exception earlier for this, but it won't alter the fact that the len(PDU) is odd and we can't decode it currently. Can you post a valid sample PDU that is odd length, please? Perhaps then we can decode it manually and figure out just where we are going wrong.
Thanks,
Andrew
Hi Andrew,
thanks for your support. This is the one of the guilty PDUs.
0791933385280200040C9193335922759200000160012121138082416650DA0C8192649D7408938192ED725A37ABCD72371B2C46ABCD6A33DB0D444DB7CB3A192C0673C16CAE180C1483E962325D8C0632A6F13A1ACD65ABE56E33560CE6CAD96EB11B681AA6EB6EA0275C2793C96031D0B0CC66EB6C325B0D3
Tell me if I can help in any way.
Emiliano
Hi Emiliano,
Thanks, that's great, but it'll be a few days before I can pick this up, I've a deadline to meet on another project. Maybe if Pablo's available he might pick it up in the meantime.
Thanks,
Andrew
Hi Emiliano,
even though the GSM spec does not mention it, the PDU length (in chars) must be even. Why you might ask? Well, the PDU is made of octets right? And each octet is composed of two hexadecimal chars, thus the PDU must be a multiple of two. Look at this two examples of decoding a PDU:
Having said this, I fed PDUspy.exe with your PDU and was able to decode it. This however wasn't flawless, the User Data Length section reads: "130 septets (should be 108)". I really think there's something wrong with the PDUs that your modem produces, can yo reproduce that PDU with another modem/device and the very same text?
Hi Pablo,
for first, let me thank you for this incredible library, I am using it with great satisfacion (and I am also going to modify it to allow sending WAP push messages for my project, tell me something if you are interested).
About this issue, I feel you are completely right regarding the PDU length, it makes completely sense. I downloaded PDUspy and verified that it is able to decode it, but let me say that (knowing what the destination format should look like), the payload of the message is incomplete as it's missing something like 10 chars.
Thus, I diefinitely think that it's my modem fault, I will ask a replacement for a better modem.
However, wouldn't it be better that the library decodes the PDU even though the length is odd?
Thanks,
Emiliano
Hi Emiliano,
Thanks for the feedback. It would be good to know which make and model of modem you have so that we may note that it's not working correctly?
Thanks,
Andrew
Hi Andrew,
it would be a pleasure for me.
Producer: Digicom
Model: Pocket GPRS Micro
I want to specify that this is not a recurring error: yesterday I made a test run with more than 1200 messages, 8 per minutes, without seeing any error.
I will try the new one as soon as they send it and report the results here.
Emiliano
One more thing! Regarding PDU conversion, I also used this:
http://smartposition.nl/resources/sms_pdu.html
It includes an online converter, that gave me same results as PDUspy.
Emiliano
Hi Emiliano,
Is this the item, it looks like nice product, perhaps there is a firmware update available?
http://www.digicom.it/digisit/prodotti.nsf/ENProdottiIDX/PocketGPRSMicro
Hi Andrew,
the manufacturer is italian and we are in contact with the technical support chief, that is behaving very well on our behalf.
They already updated a firmware for us to fix a more annoying bug with this modem (it was stopping receiving sms after 250 SMS), but regarding this issue they told me they would prefer to move to another modem as they feel this would better manage the high amount of received messages that my project requires.
Emiliano
Hi Emiliano,
we would be very interested on those WAP changes, feel free to submit a patch (please include unittests :)
Regards,
Pablo
Oh by the way, I'm leaving this ticket open as I agree that it would be better to replicate PDUspy behaviour and try to decode as much as we can (perhaps printing a warning somewhere), thanks for the report Emiliano
Just wanted to inform you that manufacturer is sending me the other modem (should be a Pocket GPRS http://www.digicom.it/digisit/prodotti.nsf/79f0df8e243cd596c12564c0003de06b/c35603f5ace701c3c12574e300527e75?OpenDocument ).
About the issue, if I can suggest an hack for that, it would be enough to add a "0" or a "F" at the end of the PDU if length is odd.
Andrew, Emiliano: what do you think it would be the correct approach?
- Add a '0' to the PDU if odd-length: This adds a '@' at the end of the decoded text
- Add a 'F' to the PDU if odd-length: This adds a 'x' at the end of the decoded text
- pdu = pdu[0:-1](remove last char of the PDU and make it even)
I'm leaning towards 3), the other two approaches either return a character that wasn't there in the original text or complicates the code with a sentinel value that if true would remove the last character of the decoded text, thus logically equivalent to 3) albeit more complex. Thoughts?
Emiliano: Would you be cool if I add the PDU that you pasted some messages above to our unittest suite ?
The only problem of 3) is that it is likely that a legitimate original char disappears, am I wrong?
Well, it would be better to hide just the telephone numbers with fake ones like "12345678" converted in semi-octets, for the rest of the message no problem.
Pablo,
From my point of view I see what we have now as correct; to raise an exception, and let the caller decide what to do about the odd length packet. If we implement padding or trimming of the PDU, not only do we have to guess what to do, but we have to return an indication that possibly the message is truncated in some way. Also as these PDUs may be 7 bit encoded, are you sure that adding '0' or 'F' to the end always results in the same char being appended to the text? I can't remember exactly how the GSM seven bit encoding works, but adding or trimming possibly affects more than just the last char of the message(preceding one too?).
@Emiliano: the last char cannot be decoded anyway, so you either skip it or the last char of the decoded SMS will be garbage anyway. What is better? To decode ok 99% of the text or having that 99% OK + a random character?
@andrew: I also think that the current code is OK, but the fact that other decoders seem to be able to decode the PDU while we can't worries me a bit.
Regarding PDUspy decoder, it seems that it follows approach 3) as it does not add any garbage (it seems that the final char is decoded as ' ' thou). Regarding the smartposition.nl decoder, it seems it does something similar to approach 1) as the decoded SMS ends with "@¥"
Thinking about it, neither PDUspy nor the smartposition.nl are used as production decoders, they are just PDU "analyzers" for reference, thus they can return funky stuff as "length: 130 septets (should be 108)" or even making up the text as smartposition.nl, while we are a production encoder/decoder and we cannot return meaningless results.
What should we do then?
Pablo,
Maybe we have a 'strict=True' parameter to decode_pdu(), that callers can set to False to use your method 3. I see the caller doing:
try:
msg = PDU.decode_pdu()
except ValueError:
msg = PDU.decode_pdu(strict=False)
Would that work?
I like the idea of the "strict" parameters, it makes a lot of sense to me!