From c791d97ed71541bd4168819faf7efa1e85b43322 Mon Sep 17 00:00:00 2001 From: Bostjan Meglic Date: Wed, 21 Dec 2022 08:57:50 +0000 Subject: [PATCH] [NF] Fix double-free crash when NF is under heavy load /init.c:_main() : ogs_pollset_poll() receives the time of the expiration of next timer as an argument. If this timeout is in very near future (1 millisecond), and if there are multiple events that need to be processed by ogs_pollset_poll(), these could take more than 1 millisecond for processing, resulting in the timer already passed the expiration. In case that another NF is under heavy load and responds to an SBI request with some delay of a few seconds, it can happen that ogs_pollset_poll() adds SBI responses to the event list for further processing, then ogs_timer_mgr_expire() is called which will add an additional event for timer expiration. When all events are processed one-by-one, the SBI xact would get deleted twice in a row, resulting in a crash. 0 __GI_abort () at ./stdlib/abort.c:107 1 0x00007f9de91693b1 in ?? () from /lib/x86_64-linux-gnu/libtalloc.so.2 2 0x00007f9de9a21745 in ogs_talloc_free (ptr=0x7f9d906c2c70, location=0x7f9de960bf41 "../lib/sbi/message.c:2423") at ../lib/core/ogs-memory.c:107 3 0x00007f9de95dbf31 in ogs_sbi_discovery_option_free (discovery_option=0x7f9d9090e670) at ../lib/sbi/message.c:2423 4 0x00007f9de95f7c47 in ogs_sbi_xact_remove (xact=0x7f9db630b630) at ../lib/sbi/context.c:1702 5 0x000055a482784846 in amf_state_operational (s=0x7f9d9488bbb0, e=0x7f9d90aecf20) at ../src/amf/amf-sm.c:604 6 0x00007f9de9a33cf0 in ogs_fsm_dispatch (fsm=0x7f9d9488bbb0, event=0x7f9d90aecf20) at ../lib/core/ogs-fsm.c:127 7 0x000055a48275b32e in amf_main (data=0x0) at ../src/amf/init.c:149 8 0x00007f9de9a249eb in thread_worker (arg=0x55a483d41d90) at ../lib/core/ogs-thread.c:67 9 0x00007f9de8fd2b43 in start_thread (arg=) at ./nptl/pthread_create.c:442 10 0x00007f9de9063bb4 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:100 --- src/amf/amf-sm.c | 10 ++++++++++ src/ausf/ausf-sm.c | 10 ++++++++++ src/bsf/bsf-sm.c | 10 ++++++++++ src/pcf/pcf-sm.c | 10 ++++++++++ src/scp/scp-sm.c | 10 ++++++++++ src/smf/smf-sm.c | 10 ++++++++++ src/udm/udm-sm.c | 10 ++++++++++ tests/af/af-sm.c | 10 ++++++++++ 8 files changed, 80 insertions(+) diff --git a/src/amf/amf-sm.c b/src/amf/amf-sm.c index 265115674..b62b5e7b1 100644 --- a/src/amf/amf-sm.c +++ b/src/amf/amf-sm.c @@ -600,6 +600,16 @@ void amf_state_operational(ogs_fsm_t *s, amf_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + sbi_object = sbi_xact->sbi_object; ogs_assert(sbi_object); diff --git a/src/ausf/ausf-sm.c b/src/ausf/ausf-sm.c index 81efab228..a7104997b 100644 --- a/src/ausf/ausf-sm.c +++ b/src/ausf/ausf-sm.c @@ -361,6 +361,16 @@ void ausf_state_operational(ogs_fsm_t *s, ausf_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + stream = sbi_xact->assoc_stream; ogs_assert(stream); diff --git a/src/bsf/bsf-sm.c b/src/bsf/bsf-sm.c index 9689a11cb..bba9e7558 100644 --- a/src/bsf/bsf-sm.c +++ b/src/bsf/bsf-sm.c @@ -345,6 +345,16 @@ void bsf_state_operational(ogs_fsm_t *s, bsf_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + stream = sbi_xact->assoc_stream; /* Here, we should not use ogs_assert(stream) * since 'namf-comm' service has no an associated stream. */ diff --git a/src/pcf/pcf-sm.c b/src/pcf/pcf-sm.c index 7ed547e07..3393622e9 100644 --- a/src/pcf/pcf-sm.c +++ b/src/pcf/pcf-sm.c @@ -591,6 +591,16 @@ void pcf_state_operational(ogs_fsm_t *s, pcf_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + sbi_object = sbi_xact->sbi_object; ogs_assert(sbi_object); diff --git a/src/scp/scp-sm.c b/src/scp/scp-sm.c index 9fce3ea12..1ac121664 100644 --- a/src/scp/scp-sm.c +++ b/src/scp/scp-sm.c @@ -247,6 +247,16 @@ void scp_state_operational(ogs_fsm_t *s, scp_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + stream = sbi_xact->assoc_stream; /* Here, we should not use ogs_assert(stream) * since 'namf-comm' service has no an associated stream. */ diff --git a/src/smf/smf-sm.c b/src/smf/smf-sm.c index 3b0258320..0980cbc9b 100644 --- a/src/smf/smf-sm.c +++ b/src/smf/smf-sm.c @@ -835,6 +835,16 @@ void smf_state_operational(ogs_fsm_t *s, smf_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + stream = sbi_xact->assoc_stream; /* Here, we should not use ogs_assert(stream) * since 'namf-comm' service has no an associated stream. */ diff --git a/src/udm/udm-sm.c b/src/udm/udm-sm.c index 371e21db7..25c03b6d0 100644 --- a/src/udm/udm-sm.c +++ b/src/udm/udm-sm.c @@ -402,6 +402,16 @@ void udm_state_operational(ogs_fsm_t *s, udm_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + stream = sbi_xact->assoc_stream; ogs_assert(stream); diff --git a/tests/af/af-sm.c b/tests/af/af-sm.c index e9ca38054..18780a91b 100644 --- a/tests/af/af-sm.c +++ b/tests/af/af-sm.c @@ -430,6 +430,16 @@ void af_state_operational(ogs_fsm_t *s, af_event_t *e) sbi_xact = e->h.sbi.data; ogs_assert(sbi_xact); + sbi_xact = ogs_sbi_xact_cycle(sbi_xact); + if (!sbi_xact) { + /* message was received and put into an event list, + * but not yet processed before timer expiration event + * was put into event list + */ + ogs_error("SBI transaction has already been removed"); + break; + } + stream = sbi_xact->assoc_stream; /* Here, we should not use ogs_assert(stream) * since 'namf-comm' service has no an associated stream. */