Attention is currently required from: pespin.
falconia has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email )
Change subject: rtp2trau HR to DL: validate ToC octet of RFC 5993
......................................................................
Patch Set 2:
(1 comment)
File src/trau/trau_rtp_conv.c:
https://gerrit.osmocom.org/c/libosmo-abis/+/39626/comment/68357f53_e3d8a9c8… :
PS1, Line 671: if (data[0] & 0xD0)
> well, the purpose precisely is that I don't have to look at the exact structure of that byte each ti […]
The check in question needs to be performed in two functions, HR to TRAU-16k-DL and HR to TRAU-8k-DL. We could factor out a helper function for RFC 5993 validation that would act in the most straightforward manner: first check the F bit, then extract FT and validate it. But a one-line check of `(data[0] & 0xD0)` is way more efficient that calling (or even inlining) a helper function that does two separate bit-field checks.
In the new patch revision, I added a detailed comment explaining all of this logic. Can we please accept this version?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: Ibbaa1e1e12254eaf75a999dd1b58e2145eff158c
Gerrit-Change-Number: 39626
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 26 Feb 2025 20:30:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: falconia <falcon(a)freecalypso.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: falconia, pespin.
Hello Jenkins Builder, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review+1 by pespin, Verified+1 by Jenkins Builder
Change subject: rtp2trau HR to DL: validate ToC octet of RFC 5993
......................................................................
rtp2trau HR to DL: validate ToC octet of RFC 5993
This validation serves two purposes:
* Payloads that just happen to be 15 bytes long but aren't valid
RFC 5993 should be rejected;
* Super-5993 extensions of TW-TS-002 are valid only in UL, not in DL
- catch and block them.
Change-Id: Ibbaa1e1e12254eaf75a999dd1b58e2145eff158c
---
M src/trau/trau_rtp_conv.c
1 file changed, 26 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-abis refs/changes/26/39626/2
--
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: Ibbaa1e1e12254eaf75a999dd1b58e2145eff158c
Gerrit-Change-Number: 39626
Gerrit-PatchSet: 2
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Attention is currently required from: pespin.
fixeria has posted comments on this change by fixeria. ( https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631?usp=email )
Change subject: sctp_server: invalidate handler's Pid on 'EXIT'
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631/comment/3106e09b_9755… :
PS1, Line 11: happens, the current implementation sends an EOF to the eNB and simply
> can you explain what "sending an EOF to the eNB" mean?
I meant graceful termination of the SCTP association, which is performed by calling `gen_sctp:eof/2` (see https://www.erlang.org/docs/26/man/gen_sctp#eof-2). Maybe I should rephrase...
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: erlang/osmo-s1gw
Gerrit-Branch: master
Gerrit-Change-Id: I21fedf8579baa54dc1e7ade348abffd7ee9b04c4
Gerrit-Change-Number: 39631
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 26 Feb 2025 19:56:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: fixeria.
pespin has posted comments on this change by fixeria. ( https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631?usp=email )
Change subject: sctp_server: invalidate handler's Pid on 'EXIT'
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631/comment/db419ab3_5ef1… :
PS1, Line 11: happens, the current implementation sends an EOF to the eNB and simply
can you explain what "sending an EOF to the eNB" mean?
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: erlang/osmo-s1gw
Gerrit-Branch: master
Gerrit-Change-Id: I21fedf8579baa54dc1e7ade348abffd7ee9b04c4
Gerrit-Change-Number: 39631
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 26 Feb 2025 19:38:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: falconia.
pespin has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email )
Change subject: rtp2trau HR to DL: validate ToC octet of RFC 5993
......................................................................
Patch Set 1:
(1 comment)
File src/trau/trau_rtp_conv.c:
https://gerrit.osmocom.org/c/libosmo-abis/+/39626/comment/63160a5f_43132f69… :
PS1, Line 671: if (data[0] & 0xD0)
> The objective is to accept 0x00 or 0x20 and no others in the upper nibble, while accepting anything […]
well, the purpose precisely is that I don't have to look at the exact structure of that byte each time I'm checking this code.
But if it makes it a lot easier then fine. Maybe adding a quick reference to the byte structure in the comment would be enough. I would at least add a reference to the specific section of RFC 5993 where I could fine the byte structure.
Something like (again I didn't really look at the structure):
#define RFC_5993_HDR_F_BITMASK (0x01 << 3)
#define RFC_5993_HDR_FT_BITMASK (0x0f << 4)
#define RFC_5993_HDR_FT(num) (num << 5)
if (data[0] & RFC_5993_HDR_F_BITMASK)
return -EINVAL;
if ((data[0] & RFC_5993_HDR_FT_BITMASK) != RFC_5993_HDR_FT(0) ||
(data[0] & RFC_5993_HDR_FT_BITMASK) != RFC_5993_HDR_FT(2))
return -EINVAL;
This are just suggestions, feel free to improve as you see.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: Ibbaa1e1e12254eaf75a999dd1b58e2145eff158c
Gerrit-Change-Number: 39626
Gerrit-PatchSet: 1
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: falconia <falcon(a)freecalypso.org>
Gerrit-Comment-Date: Wed, 26 Feb 2025 19:34:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: falconia <falcon(a)freecalypso.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631?usp=email )
Change subject: sctp_server: invalidate handler's Pid on 'EXIT'
......................................................................
sctp_server: invalidate handler's Pid on 'EXIT'
An 'EXIT' signal indicates that a connection handling process is dead,
so the respective pid shall not be referenced anymore. When this
happens, the current implementation sends an EOF to the eNB and simply
erases the respective client state from the clients dictionary.
The problem with this approach is that we're bypassing the logic
in client_del/2 and, as a result, never decrementing the
?S1GW_GAUGE_S1AP_ENB_NUM_SCTP_CONNECTIONS. When in sctp_recv/2 we
receive a 'shutdown_comp' event and call client_del/2, the client
state is already gone.
Instead of erasing the client state on 'EXIT', invalidate handler's
pid by setting it to undefined and keep the clients dictionary intact.
Leave the task of dictionary manipulation up to client_{add,del} API.
Change-Id: I21fedf8579baa54dc1e7ade348abffd7ee9b04c4
---
M src/sctp_server.erl
1 file changed, 9 insertions(+), 3 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/erlang/osmo-s1gw refs/changes/31/39631/1
diff --git a/src/sctp_server.erl b/src/sctp_server.erl
index 9b10bbe..0a42011 100644
--- a/src/sctp_server.erl
+++ b/src/sctp_server.erl
@@ -69,7 +69,7 @@
}).
-record(client_state, {addr_port :: addr_port(),
- pid :: pid()
+ pid :: pid() | undefined
}).
@@ -153,10 +153,12 @@
#server_state{sock = Sock, clients = Clients} = S0) ->
?LOG_DEBUG("Child process ~p terminated with reason ~p", [Pid, Reason]),
case client_find(Pid, S0) of
- {ok, {Aid, _Client}} ->
+ {ok, {Aid, C0}} ->
%% gracefully close the eNB connection
gen_sctp:eof(Sock, #sctp_assoc_change{assoc_id = Aid}),
- S1 = S0#server_state{clients = dict:erase(Aid, Clients)},
+ %% invalidate pid in the client's state
+ C1 = C0#client_state{pid = undefined},
+ S1 = S0#server_state{clients = dict:store(Aid, C1, Clients)},
{noreply, S1};
error ->
{noreply, S0}
@@ -207,6 +209,9 @@
?LOG_DEBUG("eNB connection (id=~p, ~p:~p) -> MME: ~p", [Aid, FromAddr, FromPort, Data]),
s1gw_metrics:ctr_inc(?S1GW_CTR_S1AP_ENB_ALL_RX),
case dict:find(Aid, Clients) of
+ {ok, #client_state{pid = undefined}} ->
+ ?LOG_NOTICE("eNB connection (id=~p, ~p:~p) -> MME data ignored (no handler)",
+ [Aid, FromAddr, FromPort]);
{ok, #client_state{pid = Pid}} ->
Handler:send_data(Pid, Data);
error ->
@@ -247,6 +252,7 @@
s1gw_metrics:gauge_dec(?S1GW_GAUGE_S1AP_ENB_NUM_SCTP_CONNECTIONS),
S#server_state{clients = dict:erase(Aid, Clients)};
error ->
+ ?LOG_ERROR("eNB connection (id=~p) is not known to us?!?", [Aid]),
S
end.
--
To view, visit https://gerrit.osmocom.org/c/erlang/osmo-s1gw/+/39631?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: erlang/osmo-s1gw
Gerrit-Branch: master
Gerrit-Change-Id: I21fedf8579baa54dc1e7ade348abffd7ee9b04c4
Gerrit-Change-Number: 39631
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Attention is currently required from: pespin.
falconia has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email )
Change subject: rtp2trau HR to DL: validate ToC octet of RFC 5993
......................................................................
Patch Set 1:
(1 comment)
File src/trau/trau_rtp_conv.c:
https://gerrit.osmocom.org/c/libosmo-abis/+/39626/comment/f8d52039_565456dd… :
PS1, Line 671: if (data[0] & 0xD0)
> Having a bitmask of defines here would be a lot clearer.
The objective is to accept 0x00 or 0x20 and no others in the upper nibble, while accepting anything in the lower nibble. Masking with 0xD0 is the most efficient way to implement this check. In terms of the "bitmask of defines" you are asking for, can you please elaborate? Exactly how do you propose I change this code?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/39626?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: Ibbaa1e1e12254eaf75a999dd1b58e2145eff158c
Gerrit-Change-Number: 39626
Gerrit-PatchSet: 1
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 26 Feb 2025 19:16:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: pespin.
falconia has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/libosmo-abis/+/39621?usp=email )
Change subject: tests: add unit tests for osmo_rtp2trau()
......................................................................
Patch Set 1:
(1 comment)
File tests/trau_conv/tw5reader.c:
https://gerrit.osmocom.org/c/libosmo-abis/+/39621/comment/f5fc0d0c_b78a0c36… :
PS1, Line 37: int twts005_read_frame(FILE *hexf, unsigned *lineno, uint8_t *frame,
> In case you don't know about it, you may want to have a look at osmo_hexparse().
I am aware of that libosmocore function and have used it in other similar unit tests. However, in the present case because I am reading not just any hex format but specifically TW-TS-005 (besides just saying "it is hex", the spec defines maximum line length and NULL representation for 0-length RTP payloads), it was easier to copy/adapt the existing TW-TS-005 reader module/function from `gsm-codec-lib` than to reimplement a new one using `osmo_hexparse()`.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/39621?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-Change-Id: Ia5ca8af6bd3a899253bbcc718b70e43f2265b495
Gerrit-Change-Number: 39621
Gerrit-PatchSet: 1
Gerrit-Owner: falconia <falcon(a)freecalypso.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 26 Feb 2025 18:58:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-sigtran/+/39630?usp=email )
Change subject: xua_rkm: Fix dynamic AS destroyed when multiple ASPs are associated
......................................................................
xua_rkm: Fix dynamic AS destroyed when multiple ASPs are associated
If more than 1 ASP was assigned to a dynamic ASP, when one of the ASP
would go down it would incorrectly tear down the associated dynamic AS.
This is wrong and should only happen when the last of the ASPs
associated to the AS is destroyed.
Change-Id: I986044944282cea9a13ed59424f2220fee6fe567
---
M src/osmo_ss7_as.c
M src/ss7_as.h
M src/xua_rkm.c
3 files changed, 20 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-sigtran refs/changes/30/39630/1
diff --git a/src/osmo_ss7_as.c b/src/osmo_ss7_as.c
index eb655ca..076cc35 100644
--- a/src/osmo_ss7_as.c
+++ b/src/osmo_ss7_as.c
@@ -199,6 +199,21 @@
return false;
}
+/*! Determine amount of ASPs associated to an AS.
+ * \param[in] as Application Server.
+ * \returns number of ASPs associated to as */
+unsigned int osmo_ss7_as_count_asp(const struct osmo_ss7_as *as)
+{
+ unsigned int i;
+ unsigned int cnt = 0;
+
+ for (i = 0; i < ARRAY_SIZE(as->cfg.asps); i++) {
+ if (as->cfg.asps[i])
+ cnt++;
+ }
+ return cnt;
+}
+
/*! Determine if given AS is in the active state.
* \param[in] as Application Server.
* \returns true in case as is active; false otherwise. */
diff --git a/src/ss7_as.h b/src/ss7_as.h
index 504818c..9f4be25 100644
--- a/src/ss7_as.h
+++ b/src/ss7_as.h
@@ -67,5 +67,7 @@
} cfg;
};
+unsigned int osmo_ss7_as_count_asp(const struct osmo_ss7_as *as);
+
#define LOGPAS(as, subsys, level, fmt, args ...) \
_LOGSS7((as)->inst, subsys, level, "as-%s: " fmt, (as)->cfg.name, ## args)
diff --git a/src/xua_rkm.c b/src/xua_rkm.c
index 9a3918e..dd07db2 100644
--- a/src/xua_rkm.c
+++ b/src/xua_rkm.c
@@ -601,10 +601,11 @@
llist_for_each_entry_safe(as, as2, &inst->as_list, list) {
if (!osmo_ss7_as_has_asp(as, asp))
continue;
- /* FIXME: check if there are no other ASPs! */
if (!as->rkm_dyn_allocated)
continue;
- osmo_ss7_as_destroy(as);
+ /* If there are no other ASPs, destroy the AS: */
+ if (osmo_ss7_as_count_asp(as) == 1)
+ osmo_ss7_as_destroy(as);
}
}
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sigtran/+/39630?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: libosmo-sigtran
Gerrit-Branch: master
Gerrit-Change-Id: I986044944282cea9a13ed59424f2220fee6fe567
Gerrit-Change-Number: 39630
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>