[FFmpeg-devel] [PATCH] avformat: Add Pro-MPEG CoP #3-R2 FEC protocol

[Michael Niedermayer]

On Wed, May 18, 2016 at 02:46:33PM +0200, Vlad Tarca wrote:
> Fixed a type issue in xor added after the last modification and cleaned up unnecessary NULL checks for av_dict_free
[...]

> +static int prompeg_init(URLContext *h, const uint8_t *buf, int size) {
> +    PrompegContext *s = h->priv_data;
> +    uint32_t seed;
> +    int bitstring_len, rtp_buf_len, i;
> +
> +    s->packet_idx = 0;
> +    s->packet_idx_max = s->l * s->d;
> +    s->packet_size = size;
> +    s->recovery_len = size - 12;

I think size needs to be checked to avoid integer overflow
there is a check later but unless iam mising somthng thats too late

> +    bitstring_len = s->recovery_len + 8;
> +    s->bitstring_size = bitstring_len * sizeof (uint8_t);
> +    rtp_buf_len = 12 + 16 + s->recovery_len;

a check for size being too large is missing too i think


> +    s->rtp_buf_size = rtp_buf_len * sizeof (uint8_t);
> +    s->fec_buf_len = 1 + 2 * s->l; // row + column tmp + column out
> +
> +    if (h->flags & AVFMT_FLAG_BITEXACT) {
> +        s->rtp_col_sn = 0;
> +        s->rtp_row_sn = 0;
> +    } else {
> +        seed = av_get_random_seed();
> +        s->rtp_col_sn = seed & 0x0fff;
> +        s->rtp_row_sn = (seed >> 16) & 0x0fff;
> +    }
> +
> +    s->rtp_buf = NULL;
> +    s->fec_buf = av_malloc_array(s->fec_buf_len, sizeof (PrompegFec*));
> +    if (!s->fec_buf) {
> +        goto fail;
> +    }
> +    for (i = 0; i < s->fec_buf_len; i++) {
> +        s->fec_buf[i] = av_malloc(sizeof (PrompegFec));
> +        if (!s->fec_buf[i]) {
> +            goto fail;
> +        }
> +        s->fec_buf[i]->bitstring = av_malloc_array(bitstring_len, sizeof (uint8_t));
> +        if (!s->fec_buf[i]->bitstring) {
> +            av_freep(&s->fec_buf[i]);
> +            goto fail;
> +        }
> +    }
> +    s->fec_row = *s->fec_buf;
> +    s->fec_col = s->fec_buf + 1;
> +    s->fec_col_tmp = s->fec_buf + 1 + s->l;
> +
> +    s->rtp_buf = av_malloc_array(rtp_buf_len, sizeof (uint8_t));
> +    if (!s->rtp_buf) {
> +        goto fail;
> +    }
> +    memset(s->rtp_buf, 0, s->rtp_buf_size);
> +
> +    s->init = 0;
> +    s->first = 1;
> +
> +    return 0;
> +
> +fail:
> +    if (s->fec_buf) {
> +        for (i = 0; i < s->fec_buf_len; i++) {
> +            if (!s->fec_buf[i]) {
> +                break;
> +            }
> +            av_free(s->fec_buf[i]->bitstring);
> +            av_freep(&s->fec_buf[i]);
> +        }
> +        av_freep(&s->fec_buf);
> +    }

> +    if (s->rtp_buf) {
> +        av_freep(&s->rtp_buf);
> +    }

redudant NULL check


[...]
> +static int prompeg_close(URLContext *h) {
> +    PrompegContext *s = h->priv_data;
> +    int i;
> +
> +    ffurl_close(s->fec_col_hd);
> +    ffurl_close(s->fec_row_hd);
> +
> +    if (s->fec_buf) {
> +        for (i = 0; i < s->fec_buf_len; i++) {
> +            av_free(s->fec_buf[i]->bitstring);
> +            av_freep(&s->fec_buf[i]);
> +        }
> +        av_freep(&s->fec_buf);
> +    }
> +    if (s->rtp_buf)
> +        av_freep(&s->rtp_buf);

this looks duplicated, can this be factored with the similar previous
code ?


[...]
> @@ -377,6 +382,33 @@ static int rtp_open(URLContext *h, const char *uri, int flags)
>          }
>      }
>  
> +    if (s->fec_options_str) {
> +        p = s->fec_options_str;
> +
> +        if (!(fec_protocol = av_get_token(&p, "="))) {
> +            av_log(h, AV_LOG_ERROR, "Failed to parse the FEC protocol value\n");
> +            goto fail;
> +        }
> +        if (strcmp(fec_protocol, "prompeg")) {
> +            av_log(h, AV_LOG_ERROR, "Unsupported FEC protocol %s\n", fec_protocol);
> +            goto fail;
> +        }
> +
> +        p = s->fec_options_str + strlen(fec_protocol);
> +        while (*p && *p == '=') p++;
> +
> +        if (av_dict_parse_string(&fec_opts, p, "=", ":", 0) < 0) {
> +            av_log(h, AV_LOG_ERROR, "Failed to parse the FEC options\n");
> +            goto fail;
> +        }
> +        if (s->ttl > 0) {

> +            snprintf(buf, 256, "%d", s->ttl);

sizeif(buf) or something, hardcoded 256 is bad, that can be missed
if someone changes the array size

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160524/28b7faf8/attachment.sig>