[PATCH] fixing possible PANIC condition

[Stipe Tolj]

Hi list,

attached is a patch that fixes a PANIC condition we have been reported:

2018-09-07 11:38:07.241 [25148] [9] PANIC: gwlib/conn.c:889:
conn_unregister: Assertion `conn != NULL' failed.
2018-09-07 11:38:07.255 [25148] [9] PANIC:
/opt/kannel/sbin/smsbox(gw_backtrace+0xbe) [0x44e82e]
2018-09-07 11:38:07.255 [25148] [9] PANIC:
/opt/kannel/sbin/smsbox(gw_panic+0x1ce) [0x44ea0e]
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox()
[0x44a21c]
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox()
[0x43fba1]
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox()
[0x443783]

where:

# addr2line -e /opt/kannel/sbin/smsbox 0x44a21c 0x43fba1 0x443783
/tmp/kannel/smppbox-1.9.4/gateway-ha/gwlib/http.c:1213
/tmp/kannel/smppbox-1.9.4/gateway-ha/gwlib/fdset.c:354
/tmp/kannel/smppbox-1.9.4/gateway-ha/gwlib/gwthread-pthread.c:165

so this was coming from gwlib/http.c:handle_transaction().

In fact we DO conn_destroy in gwlib/http.c:/send_request() if a
conn_write() fails, and then again in handle_transaction() bail out and
try to conn_unregister() and conn_destroy().

The conn_destroy() is safe as it tests for conn == NULL, but the
conn_unregister() would raise an assertion panic here.

Suggested patch simply removes the gw_assert() check and leaves it to
the function to test (what it does) to bail out if the conn is NULL.

If no objections arise, will commit to svn trunk.

Stipe
--
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation tolj.org system architecture
http://www.kannel.org/ http://www.tolj.org/

stolj at kannel.org st at tolj.org
-------------------------------------------------------------------

[a***@kannel.org]

Hi Stipe,

is it not easier just check for conn != NULL at the place it passed to conn_unregister ?
assertion is very interesting at this place IMHO because it can show that someone try to unregister connection which is null but should not be null.
This prevents from resources leak.

Alex

Post by Stipe Tolj
Hi list,
2018-09-07 11:38:07.241 [25148] [9] PANIC: gwlib/conn.c:889: conn_unregister: Assertion `conn != NULL' failed.
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox(gw_backtrace+0xbe) [0x44e82e]
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox(gw_panic+0x1ce) [0x44ea0e]
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox() [0x44a21c]
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox() [0x43fba1]
2018-09-07 11:38:07.255 [25148] [9] PANIC: /opt/kannel/sbin/smsbox() [0x443783]
# addr2line -e /opt/kannel/sbin/smsbox 0x44a21c 0x43fba1 0x443783
/tmp/kannel/smppbox-1.9.4/gateway-ha/gwlib/http.c:1213
/tmp/kannel/smppbox-1.9.4/gateway-ha/gwlib/fdset.c:354
/tmp/kannel/smppbox-1.9.4/gateway-ha/gwlib/gwthread-pthread.c:165
so this was coming from gwlib/http.c:handle_transaction().
In fact we DO conn_destroy in gwlib/http.c:/send_request() if a conn_write() fails, and then again in handle_transaction() bail out and try to conn_unregister() and conn_destroy().
The conn_destroy() is safe as it tests for conn == NULL, but the conn_unregister() would raise an assertion panic here.
Suggested patch simply removes the gw_assert() check and leaves it to the function to test (what it does) to bail out if the conn is NULL.
If no objections arise, will commit to svn trunk.
Stipe
--
Best Regards,
Stipe Tolj
-------------------------------------------------------------------
Düsseldorf, NRW, Germany
Kannel Foundation tolj.org system architecture
http://www.kannel.org/ http://www.tolj.org/
stolj at kannel.org st at tolj.org
-------------------------------------------------------------------
<workspace-gateway-conn-unreg.diff>

[Stipe Tolj]

Post by a***@kannel.org
Hi Stipe,
is it not easier just check for conn != NULL at the place it passed to conn_unregister ?
assertion is very interesting at this place IMHO because it can show that someone try to unregister connection which is null but should not be null.
This prevents from resources leak.

Hi Alex,

hope you're doing fine.

I agree, committed a fix for the possible PANIC condition by
pre-checking before calling conn_unregister() in the handle_transaction().

Stipe
--
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation tolj.org system architecture
http://www.kannel.org/ http://www.tolj.org/

stolj at kannel.org st at tolj.org
-------------------------------------------------------------------

[Alexander Malysh]

Thanks a lot.

Alex

Post by a***@kannel.org

Post by a***@kannel.org
Hi Stipe,
is it not easier just check for conn != NULL at the place it passed to

conn_unregister ?

Post by a***@kannel.org
assertion is very interesting at this place IMHO because it can show

that someone try to unregister connection which is null but should not be
null.

Post by a***@kannel.org
This prevents from resources leak.

Hi Alex,
hope you're doing fine.
I agree, committed a fix for the possible PANIC condition by
pre-checking before calling conn_unregister() in the handle_transaction().
Stipe
--
Best Regards,
Stipe Tolj
-------------------------------------------------------------------
DÃŒsseldorf, NRW, Germany
Kannel Foundation tolj.org system architecture
http://www.kannel.org/ http://www.tolj.org/
stolj at kannel.org st at tolj.org
-------------------------------------------------------------------