[vlc-devel] [PATCHv4 1/3] access: Add satip access module

[Julian Scheel]

Hi Filip,

thanks for the review. Would you mind disabling HTML content for 
messages on the list?

On 27.05.2016 10:12, Filip Roséen wrote:
>> +static enum rtsp_result rtsp_handle(access_t *access) {
>> + [... ]
>> +
>> +    /* Parse header */
>> +    while (!have_header && (in = net_Gets(access, sys->tcp_sock)) != NULL) {
>> +        if (strstr(in, "RTSP/1.0 ") != NULL) {
>> +            rtsp_result = atoi(in + 9);
>
> The above logic is faulty. I assume the intent is to look for "RTSP/1.0 " and
> then treat the following bytes as the textual-representation of an integer,
> though this is not what is going on.

Good catch, the strstr should have been a strncmp, as the response 
header must start with the RTSP/1.0 prefix followed by the status code.

> If an implementation-specific (or similar) header contains "RTSP/1.0 " you would
> overwrite `rtsp_result` with a value with what just happens to be at `[9, ...)`
> in `in`, even if a correct value has already been found.
>
>     RTSP/1.0 200
>     Content-Base: ...
>     Implementation-Specific-Header: RTSP/1.0 korv
>
> The contents of *"Implementation-Specific-Header [...]"* will trigger
> `rtsp_result = atoi( in + 9 )`.
>
>> +        } else if (strncmp(in, "Content-Base", 12) == 0) {
>> +            free(sys->content_base);
>> +            sys->content_base = strdup(in + 14);
>
> Nothing in the above says that `in + 14` is even within the valid range of the
> string yield by `net_Gets`; you are really looking for "Content-Base: ", as such
> usage of `strncmp( in, "Content-Base: ", 14 )` is far more appropriate.
>
> `strdup( in + 14 )` would then, at most, `strdup` an empty string.

In fact it should be strncmp(in, "Content-Base:", 13) followed by a 
skip-whitespace loop, as the specification does not explicitly request a 
blank after the colon.
The same counts for all other header fields. Let me change that, to 
improve robustness.

>> +        } else if (strncmp(in, "Content-Length", 14) == 0) {
>> +            content_length = atoi(in + 16);
>
> See previous remark.
>
>> +        } else if (strncmp("Session:", in, 8) == 0) {
>> +            parse_session(in, sys->session_id, 64, &sys->keepalive_interval);
>> +        } else if (strncmp("Transport:", in, 10) == 0) {
>> +            parse_transport(access, in);
>> +        } else if (strncmp("com.ses.streamID:", in, 17) == 0) {
>> +            sys->stream_id = atoi(in + 17);
>> +        } else if (in[0] == '\0') {
>> +            have_header = true;
>> +        }
>
> ---------------------------------------------------------------------------------
>
>> +/* Bind two adjacent free ports, of which the first one is even (for RTP data)
>> + * and the second is odd (RTCP). This is a requirement of the satip
>> + * specification */
>> +static int satip_bind_ports(access_t *access)
>> +{
>> +    access_sys_t *sys = access->p_sys;
>> +    uint8_t rnd;
>> +
>> +    vlc_rand_bytes(&rnd, 1);
>> +    sys->udp_port = 9000 + (rnd * 2); /* randomly chosen, even start point */
>
> This following loop will, in the worse case, loop forever (given that
> `sys->udp_sock` can wrap from `65534` to `0` in each of the two `sys->udp_port
> += 2`).

And now I remember why I set udp_port to be an int instead of uint16_t 
before v3 ;)
Fixed that, thanks.

v5 will follow shortly.

-Julian

>> +    while (sys->udp_sock < 0 && sys->udp_port < 65535) {
>> +        sys->udp_sock = net_OpenDgram(access, "0.0.0.0", sys->udp_port, NULL,
>> +                0, IPPROTO_UDP);
>> +        if (sys->udp_sock < 0) {
>> +            sys->udp_port += 2;
>> +            continue;
>> +        }
>> +
>> +        sys->rtcp_sock = net_OpenDgram(access, "0.0.0.0", sys->udp_port + 1, NULL,
>> +                0, IPPROTO_UDP);
>> +        if (sys->rtcp_sock < 0) {
>> +            close(sys->udp_sock);
>> +            sys->udp_port += 2;
>> +            continue;
>> +        }
>> +    }
>> +
>> +    if (sys->udp_sock < 0) {
>> +        msg_Err(access, "Could not open two adjacent ports for RTP and RTCP data");
>> +        return VLC_EGENERIC;
>> +    }
>> +
>> +    return 0;
>> +}
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>