Re: Patch: SMSbox crash
On Thu, 2009-08-27 at 00:34, Donald Jackson wrote: Your patch looks fine but, the problem is that we have received an empty MO, and this will now not allow the sms-service to process it (maybe some sort of default process). I'm not sure this is the behavior users would expect, I think most users would expect that if they received an empty SMS they would still receive it. Obviously this is limited to some carriers who have this behavior in their data_sm PDU's. I think we should simply fix it in smsbox by converting a NULL msgdata into a octstr_create() and that should suffice without causing any unnecessary issues with the lower layers. Thoughts? Alot of writing for such a simple issue :D If you allow me to express my opinion: I'm not kannel developer, but from users perspective I agree with you. I expect from kannel to deliver message to applications even if the message is empty. 2009/8/27 Nikos Balkanas nbalka...@gmail.com Dear Alex, This is a trivial patch. However, I would like to suggest something to improve it: In thread sms_to_smsboxes it checks and if msg = NULL it drops it. I added a check if msgdata is NULL to drop it as well. This would avoid further processing (less overhead) of the sms and simplify further logic present and future, in that they wouldn't have to be concerned with it. I assume that empty MO sms don't need any sms-service or further processing. What do you think? PS. Compilation is clean. I don't have, though, any smsc connections to test it. @Hemant: Can you please test? BR, Nikos - Original Message - *From:* Alexander Malysh amal...@kannel.org *To:* Development mailing list devel@kannel.org *Cc:* Donald Jackson donaldjs...@gmail.com ; Hemant Gmailhbaan...@gmail.com; Nikos Balkanas nbalka...@gmail.com *Sent:* Wednesday, August 26, 2009 10:59 AM *Subject:* Re: Patch: SMSbox crash Hi all, ok, seems too many parts of kannel rely on the msgdata not to be NULL. @Donald: never use octstr_imm of you pass this to further processing. octstr_imm doesn't support all functions and you don't know what the user of such field will do with it. If this really the case that too much kannel parts rely on msgdata to be not NULL then your patch is not enough. Please try attached patch that will fix this issue for all available SMSC module. Thanks, Alexander Malysh -- Am 25.08.2009 um 20:24 schrieb Nikos Balkanas: Hi Donald, I guess intercepting it upstream was my intention, too, however I think you got it too far back. I was thinking more likely at the SMS router function to check if payload is NULL to quietly discard the SMS and pick the next one from Q, this way minimizing overhead. Plus at this stage it would apply to all MO SMS, not only SMPP. I changed subject to patch, so that Alex picks up on it and speaks his mind. BR, Nikos - Original Message - *From:* Donald Jackson donaldjs...@gmail.com *To:* Nikos Balkanas nbalka...@gmail.com *Cc:* Hemant Gmail hbaan...@gmail.com ; de...@vm1.kannel.org *Sent:* Tuesday, August 25, 2009 8:33 PM *Subject:* Re: SMSbox crashed Hi Nikos, Apologies I wasn't actually following the thread he emailed me privately :) The problem occurs in smsbox when it tries to find a translation for the message. The different thing about these messages, is they are NULL. This is because this operator is not sending the (TLV) message_payload parameter with the data_sm PDU if no text is specified. Smsbox/find_translation does no NULL checking on msgdata before trying the octstr_covert_range, which is why it fails the assertion. My patch simply sets the parameter to an empty Octstr inside the data_sm code, to prevent smsbox from falling over, and then still passing the upstream applications an empty string, instead of '(null)'. Let me know your thoughts, Thanks, Donald 2009/8/25 Nikos Balkanas nbalka...@gmail.com Thanks, Donald. I was personally holding out for Herman to try out patching, but yours is more than welcome. I have also not forgotten your store isuue. I have not been able to reproduce it with fakesmsc, so it seems to be exclusive to the at driver. I will have to add code to simulate the modem calls (don't have modem or other smsc links - I am a wap guy) and i wouldn't like to spend a lot of money to try it out on a real connection. Meanwhile could you provide us with some relevant bb logs from the problem? I believe you can increase in real time the log detail from the http administration. BR, Nikos - Original Message - *From:* Donald Jackson donaldjs...@gmail.com *To:* Development mailing list devel@kannel.org *Cc:* Hemant Gmail hbaan...@gmail.com ; de...@vm1.kannel.org *Sent:* Tuesday, August 25, 2009 4:40 PM *Subject:* Re: SMSbox crashed Hi Nikos, I have
Re: Patch: SMSbox crash
Yes, definitely a decision to take at the application layer, not on the gateway. Regards, -- Alejandro Guerrieri aguerri...@kannel.org On 27/08/2009, at 20:29, Milan P. Stanic wrote: On Thu, 2009-08-27 at 00:34, Donald Jackson wrote: Your patch looks fine but, the problem is that we have received an empty MO, and this will now not allow the sms-service to process it (maybe some sort of default process). I'm not sure this is the behavior users would expect, I think most users would expect that if they received an empty SMS they would still receive it. Obviously this is limited to some carriers who have this behavior in their data_sm PDU's. I think we should simply fix it in smsbox by converting a NULL msgdata into a octstr_create() and that should suffice without causing any unnecessary issues with the lower layers. Thoughts? Alot of writing for such a simple issue :D If you allow me to express my opinion: I'm not kannel developer, but from users perspective I agree with you. I expect from kannel to deliver message to applications even if the message is empty. 2009/8/27 Nikos Balkanas nbalka...@gmail.com Dear Alex, This is a trivial patch. However, I would like to suggest something to improve it: In thread sms_to_smsboxes it checks and if msg = NULL it drops it. I added a check if msgdata is NULL to drop it as well. This would avoid further processing (less overhead) of the sms and simplify further logic present and future, in that they wouldn't have to be concerned with it. I assume that empty MO sms don't need any sms-service or further processing. What do you think? PS. Compilation is clean. I don't have, though, any smsc connections to test it. @Hemant: Can you please test? BR, Nikos - Original Message - *From:* Alexander Malysh amal...@kannel.org *To:* Development mailing list devel@kannel.org *Cc:* Donald Jackson donaldjs...@gmail.com ; Hemant Gmailhbaan...@gmail.com ; Nikos Balkanas nbalka...@gmail.com *Sent:* Wednesday, August 26, 2009 10:59 AM *Subject:* Re: Patch: SMSbox crash Hi all, ok, seems too many parts of kannel rely on the msgdata not to be NULL. @Donald: never use octstr_imm of you pass this to further processing. octstr_imm doesn't support all functions and you don't know what the user of such field will do with it. If this really the case that too much kannel parts rely on msgdata to be not NULL then your patch is not enough. Please try attached patch that will fix this issue for all available SMSC module. Thanks, Alexander Malysh -- Am 25.08.2009 um 20:24 schrieb Nikos Balkanas: Hi Donald, I guess intercepting it upstream was my intention, too, however I think you got it too far back. I was thinking more likely at the SMS router function to check if payload is NULL to quietly discard the SMS and pick the next one from Q, this way minimizing overhead. Plus at this stage it would apply to all MO SMS, not only SMPP. I changed subject to patch, so that Alex picks up on it and speaks his mind. BR, Nikos - Original Message - *From:* Donald Jackson donaldjs...@gmail.com *To:* Nikos Balkanas nbalka...@gmail.com *Cc:* Hemant Gmail hbaan...@gmail.com ; de...@vm1.kannel.org *Sent:* Tuesday, August 25, 2009 8:33 PM *Subject:* Re: SMSbox crashed Hi Nikos, Apologies I wasn't actually following the thread he emailed me privately :) The problem occurs in smsbox when it tries to find a translation for the message. The different thing about these messages, is they are NULL. This is because this operator is not sending the (TLV) message_payload parameter with the data_sm PDU if no text is specified. Smsbox/find_translation does no NULL checking on msgdata before trying the octstr_covert_range, which is why it fails the assertion. My patch simply sets the parameter to an empty Octstr inside the data_sm code, to prevent smsbox from falling over, and then still passing the upstream applications an empty string, instead of '(null)'. Let me know your thoughts, Thanks, Donald 2009/8/25 Nikos Balkanas nbalka...@gmail.com Thanks, Donald. I was personally holding out for Herman to try out patching, but yours is more than welcome. I have also not forgotten your store isuue. I have not been able to reproduce it with fakesmsc, so it seems to be exclusive to the at driver. I will have to add code to simulate the modem calls (don't have modem or other smsc links - I am a wap guy) and i wouldn't like to spend a lot of money to try it out on a real connection. Meanwhile could you provide us with some relevant bb logs from the problem? I believe you can increase in real time the log detail from the http administration. BR, Nikos - Original Message - *From:* Donald Jackson donaldjs...@gmail.com *To:* Development mailing list devel@kannel.org *Cc:* Hemant Gmail hbaan...@gmail.com ; de...@vm1.kannel.org *Sent:* Tuesday, August 25, 2009
Re: [PATCH] Smsbox Crash
Wake up reviewers! :) M On 1/10/07, Mi Reflejo [EMAIL PROTECTED] wrote: How to reproduce it: # curl -H Content-type: text/html -d uelfrom=1010to=11text=Invalid Keywordbinfo= http://127.0.0.1:13002/cgi-bin/sendsms Logs: 2007-01-10 15:56:05 [13414] [3] PANIC: gwlib/octstr.c:2476: seems_valid_real: Assertion `ostr-data != NULL' failed. (Called from gwlib/octstr.c:318:octstr_destroy.) 2007-01-10 15:56:05 [13414] [1] DEBUG: HTTP: Destroying HTTPClient area 0x8159a90. 2007-01-10 15:56:05 [13414] [1] DEBUG: HTTP: Destroying HTTPClient for `127.0.0.1'. 2007-01-10 15:56:05 [13414] [3] PANIC: gw/smsbox(gw_panic+0xcc) [0x808845c] 2007-01-10 15:56:05 [13414] [3] PANIC: gw/smsbox [0x80896e8] 2007-01-10 15:56:05 [13414] [3] PANIC: gw/smsbox(octstr_destroy+0x2a) [0x8089dfa] 2007-01-10 15:56:05 [13414] [3] PANIC: gw/smsbox [0x805b3ff] 2007-01-10 15:56:05 [13414] [3] PANIC: gw/smsbox [0x807f070] 2007-01-10 15:56:05 [13414] [3] PANIC: /lib/libpthread.so.0 [0xb7f2e604] 2007-01-10 15:56:05 [13414] [3] PANIC: /lib/libc.so.6(__clone+0x5e) [0xb7c5c09e] Problem: The var body is filled with the pointer to text and then destroyed twice- Fixed in the attached Patch. Martin.
