Attention is currently required from: pespin.
neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/37996?usp=email )
Change subject: coverity CID#358071
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
File src/osmo-hnbgw/hnbgw_hnbap.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/37996/comment/e3a5b744_6ecd15dc?u… :
PS1, Line 657: cause.present = HNBAP_Cause_PR_protocol;
> probably better to do here: […]
hehe this is a great moment in history, there have been days where i had to fight fierce battles in support of this struct assignment notation =)
If this were new code, I'd write it exactly as you suggest, but here i prefer a small patch size / less surface for merge conflicts; note, there is another use of 'cause' below.
The effect of our two variants is identical, it's a bikeshed.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/37996?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I8237bf7d4985e993bb10aaaa9370cde2ece3d812
Gerrit-Change-Number: 37996
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 04 Sep 2024 00:54:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: fixeria, pespin.
Hello Jenkins Builder, fixeria,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review+1 by fixeria, Verified+1 by Jenkins Builder
Change subject: coverity CID#216829
......................................................................
coverity CID#216829
<removed confused nonsense here>
Change-Id: I30beeac45ff2d8c08905986af9fabfda071ddc5b
---
M src/rtp.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/92/37992/2
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I30beeac45ff2d8c08905986af9fabfda071ddc5b
Gerrit-Change-Number: 37992
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Attention is currently required from: fixeria, pespin.
neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email )
Change subject: coverity
......................................................................
Patch Set 1:
(1 comment)
File src/rtp.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/37992/comment/1f92160e_4e49a2a… :
PS1, Line 120: payload_len = ((int)msg->len) - sizeof(struct rtp_hdr) - csrc_len;
> I think I misunderstood what coverity was complaining about. […]
coverity says
CID#216829
````
rtpxh = (struct rtp_x_hdr *)payload;
4. tainted_return_value: Function ntohs returns tainted data.
5. lower_bounds: Casting narrower unsigned ntohs(rtpxh->length) to wider signed type int effectively tests its lower bound.
6. var_assign_var: Assigning: x_len = ntohs(rtpxh->length) * 4 + 4UL. Both are now tainted.
133 x_len = ntohs(rtpxh->length) * 4 + sizeof(struct rtp_x_hdr);
7. var_assign_var: Compound assignment involving tainted variable x_len to variable payload taints payload.
134 payload += x_len;
8. var_assign_var: Compound assignment involving tainted variable x_len to variable payload_len taints payload_len.
135 payload_len -= x_len;
9. Condition payload_len < 0, taking false branch.
10. lower_bounds: Checking lower bounds of signed scalar payload_len by taking the false branch of payload_len < 0.
136 if (payload_len < 0) {
137 DEBUGPC(DLMUX, "received RTP frame too short, "
138 "extension header exceeds frame length\n");
139 return NULL;
140 }
141 }
11. Condition rtph->padding, taking true branch.
142 if (rtph->padding) {
12. Condition payload_len < 0, taking false branch.
13. lower_bounds: Checking lower bounds of signed scalar payload_len by taking the false branch of payload_len < 0.
143 if (payload_len < 0) {
144 DEBUGPC(DLMUX, "received RTP frame too short for "
145 "padding length\n");
146 return NULL;
147 }
CID 216829: (#1 of 1): Untrusted pointer read (TAINTED_SCALAR)
14. tainted_data: Using tainted variable payload_len - 1 as an index to pointer payload.
Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
````
The code does look sane, but indeed I find it a bit hard to get a grip on whether all necessary range checks on values from the wire are in place.
Maybe with less complex range checking, more like 'if (a < b)' instead of a subtraction operation with unsigned types, coverity will also pick up on it?
(Or there is an actual bug that coverity sees.)
TL;DR let's make it trivial "if (a < b)" checks, do you agree?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I30beeac45ff2d8c08905986af9fabfda071ddc5b
Gerrit-Change-Number: 37992
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 04 Sep 2024 00:42:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: fixeria, pespin.
neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email )
Change subject: coverity
......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://gerrit.osmocom.org/c/libosmo-netif/+/37992/comment/63055690_9636139… :
PS1, Line 7: coverity
> what CID#XXX?
yeah i lost the CID and thought well whatever, that web interface is so so slow... i have to look it up now anyway.
File src/rtp.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/37992/comment/bd16546b_8204736… :
PS1, Line 120: payload_len = ((int)msg->len) - sizeof(struct rtp_hdr) - csrc_len;
> are you sure this is needed? you already had an int csrc_len in the line, and you still have a unsig […]
I think I misunderstood what coverity was complaining about.
(
I also just learned that, after all, implicit type casts in C work differently than I thought I absolutely knew.
I always thought the implicit casting starts from the left operand, except for assignments.. so i my world this calculation would be done in uint16_t, and only later assigned to an int. So the result would never be negative. My little test program taught me otherwise, i'm a bit confused now.
So how *does* C order the implicit casts?
I have certainly had many many numeric range and signedness bugs that were fixed by casting the leftmost operand to the intended type. Do these bugs happen only with types larger than int, maybe? did i completely misunderstand this back as a teenager and it always worked out for me by random?
)
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/37992?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I30beeac45ff2d8c08905986af9fabfda071ddc5b
Gerrit-Change-Number: 37992
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 04 Sep 2024 00:34:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: laforge, pespin.
neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/libosmo-netif/+/37991?usp=email )
Change subject: coverity CID#322728
......................................................................
Patch Set 1:
(1 comment)
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/37991/comment/0642b7e5_9947d07… :
PS1, Line 282: ret = sctp_recvmsg(fd, msg->tail, msgb_tailroom(msg), NULL, NULL, &sinfo, &flags);
> See https://gerrit.osmocom.org/c/libosmo-netif/+/38004/1 for fixing the actual logical error.
yes, this patch series is indeed sideways development: i merely aim to prevent situations of potential memory corruption, in various areas where i do not understand the context very well.
abandoning this for 38004
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/37991?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ia4c6b7c6f16331932e3f584b800e86d422a4f019
Gerrit-Change-Number: 37991
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 03 Sep 2024 23:52:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: fixeria, laforge.
neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/libosmo-sccp/+/37994?usp=email )
Change subject: coverity CID#272968 CID#272939
......................................................................
Patch Set 1:
(1 comment)
File src/sccp.c:
https://gerrit.osmocom.org/c/libosmo-sccp/+/37994/comment/6e0270a2_d09eb640… :
PS1, Line 167: return 0;
> I'd agree.
i picked 0 because below it says
if (read + 1 >= room) {
LOGP(DSCCP, LOGL_ERROR, "no place for length\n");
return 0;
}
and other places also return 0 for length errors.
so are you guys sure?
I think this function returns the number of bytes processed, and it being static means that it may be very specialized to work with rc == 0.
It also returns -1 right at the bottom, so it seems to be pick n choose
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sccp/+/37994?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Change-Id: Ic6823cf077ef15ef1f6e209bf53384913911f93e
Gerrit-Change-Number: 37994
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 03 Sep 2024 23:46:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Attention is currently required from: laforge.
neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/libasn1c/+/37983?usp=email )
Change subject: coverity CID#57935
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> this shouldn't be fixed here but in the actual asn1c code, and then re-generate the templates here?
oof, gotcha.
--
To view, visit https://gerrit.osmocom.org/c/libasn1c/+/37983?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libasn1c
Gerrit-Branch: master
Gerrit-Change-Id: I23cfbb1cf1fc12ebd4db8e4b58c5e797b2834897
Gerrit-Change-Number: 37983
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Tue, 03 Sep 2024 23:36:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/38004?usp=email )
Change subject: src/stream.c: Use sctp_assoc_id 'canary' to detect misisng sinfo
......................................................................
src/stream.c: Use sctp_assoc_id 'canary' to detect misisng sinfo
sctp_rcvinfo() has a successful return path that does *not* fill
the caller-provided sinfo structure. This would be highly unexpected
as we cannot determine the stream_id and PPID then.
Our osmo_io code path already handles that situation correctly
by logging a related error message. The non-osmo-io path however
would silently hide that error.
See also: https://github.com/sctp/lksctp-tools/issues/37
Change-Id: Idf9c0fa4db65ab6ea34a9cb2011400aadf3dd54e
---
M src/stream.c
1 file changed, 7 insertions(+), 1 deletion(-)
Approvals:
pespin: Looks good to me, but someone else must approve
fixeria: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/src/stream.c b/src/stream.c
index f8cbed6..5033f4c 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -279,8 +279,14 @@
int flags = 0;
int ret;
+ /* Canary to detect if kernel returns sinfo; see https://github.com/sctp/lksctp-tools/issues/37 */
+ sinfo.sinfo_assoc_id = 0;
+
ret = sctp_recvmsg(fd, msg->tail, msgb_tailroom(msg), NULL, NULL, &sinfo, &flags);
- return stream_sctp_recvmsg_trailer(log_pfx, msg, ret, &sinfo, flags);
+ if (sinfo.sinfo_assoc_id)
+ return stream_sctp_recvmsg_trailer(log_pfx, msg, ret, &sinfo, flags);
+ else
+ return stream_sctp_recvmsg_trailer(log_pfx, msg, ret, NULL, flags);
}
/*! wrapper for osmo_io asynchronous recvmsg response */
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/38004?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Idf9c0fa4db65ab6ea34a9cb2011400aadf3dd54e
Gerrit-Change-Number: 38004
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
