Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

neels gerrit-no-reply at lists.osmocom.org
Mon Sep 2 23:35:47 UTC 2019


neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
......................................................................


Patch Set 6:

(1 comment)

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c 
File src/libmsc/msc_a.c:

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c@1411 
PS3, Line 1411: but this static msgb saves the extra allocation
> ACK. […]
I don't see how this can be a problem. None of the DTAP handling code is permitted to take ownership of the msgb.

What we would do here is allocate a msgb, dispatch it to DTAP, then deallocate it right after that here in this function. Even if it were allocated, it would still not be possible to put it in a queue. Nothing would be gained AFAICT.

This is the osmo-msc internal DTAP code, it is fully contained here: all handling of this msgb is in gsm_04_08.c. We're not talking about the general public or another osmocom library here.

The premise of DTAP decoding is to keep the incoming buffer unchanged, essentially a 'const' by convention. It is the incoming data and it is not a good idea to want to modify it.

Passing the msgb as 'const' is not necessary: even though this is a stack struct msgb, we are still free to write to it. We are not allowed to free it, but none of the DTAP code is allowed to free msgb passed to it -- it would lead to a double free.

If you guys still insist, this can become a dynamic allocation, but I am convinced that this patch is completely fine as it is now. The data is there, no need to allocate.

[BTW, talking of const msgb... I never added the 'const' to msgb args because the legacy gsm0408_* code neglected that. I would gladly change it to const, now that we have the time for it. But we do typically write l3h and cb items into the msgb, i.e. keep state for later in the code path. Also the msgb API from libosmocore often takes non-const msgb even for read-only API (e.g. msgb_get_u8()), which would need a lot of casting to non-const.]



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 6
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: fixeria <axilirator at gmail.com>
Gerrit-CC: laforge <laforge at gnumonks.org>
Gerrit-Comment-Date: Mon, 02 Sep 2019 23:35:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <axilirator at gmail.com>
Comment-In-Reply-To: laforge <laforge at gnumonks.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190902/64dd4969/attachment.htm>


More information about the gerrit-log mailing list