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_4e49a2af... : 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?