[FFmpeg-devel] [PATCH] Add XPM decoder

Paul B Mahol onemda at gmail.com
Sun Mar 12 21:24:18 EET 2017


On 3/12/17, Nicolas George <george at nsup.org> wrote:
> Le duodi 22 ventose, an CCXXV, Paras Chadha a ecrit :
>> Xpm decoder was added
>> Also added xpm_pipe demuxer with its probe function
>>

[...]

>> +typedef struct XPMContext {
>
>> +    uint32_t  *pixels;
>> +    int      pixels_size;
>
> The spacing is strange.

OK.

>
>> +} XPMDecContext;
>> +
>> +typedef struct ColorEntry {
>> +    const char *name;            ///< a string representing the name of
>> the color
>> +    uint32_t    rgb_color;    ///< RGB values for the color
>> +} ColorEntry;
>> +
>> +static int color_table_compare(const void *lhs, const void *rhs)
>> +{
>> +    return av_strcasecmp(lhs, ((const ColorEntry *)rhs)->name);
>> +}
>> +
>> +static const ColorEntry color_table[] = {
>
> <snip>
>
> The code duplication with parseutils is unacceptable, and I find the
> reasons given by Paul weak. In particular, I do not see any conflict
> with the database on my X.org version.

Because conflicting entries have not been added yet. Last time I
compared it was different.
Also when Last time I tried it was soo slow that made 10k colors very
slow to decode.

>
>> +};
>> +
>
>> +static int convert(uint8_t x)
>
> convert is not a very good name.

OK, what you propose?

>
>> +{
>
>> +    if (x >= 'a') {
>> +        x -= 87;
>> +    } else if (x >= 'A') {
>> +        x -= 55;
>> +    } else {
>
> Avoid magic numbers in the code; x - 87 = x - 'a' + 10,
> x - 55 = x - 'A' + 10, and "& ~32" can avoid making two cases anyway.

OK

>
>> +        x -= '0';
>> +    }
>> +    return x;
>> +}
>> +
>
>> +/*
>> +**  functions same as strcspn but ignores characters in reject if they
>> are inside a C style comment...
>> +**  @param string, reject - same as that of strcspn
>> +**  @return length till any character in reject does not occur in string
>> +*/
>
> This is not a valid Doxygen comment.

OK

>
>> +static size_t mod_strcspn(const char *string, const char *reject)
>> +{
>> +    int i, j;
>> +
>
>> +    for (i = 0; string && string[i]; i++) {
>
> The first clause of the condition is silly.

Yes, correct.

>
>> +        if (string[i] == '/' && string[i+1] == '*') {
>> +            i += 2;
>
>> +            while ( string && string[i] && (string[i] != '*' ||
>> string[i+1] != '/') )
>
> Nit: no spaces within parentheses. And ditto for the first clause.

OK

>
>> +                i++;
>
>> +            i++;
>
> If the loop exits due to the "string[i]" part, this leaves I beyond the
> end of the string, causing an illegal access on the next rounds.

OK

>
>> +        } else if (string[i] == '/' && string[i+1] == '/') {
>> +            i += 2;
>
>> +            while ( string && string[i] && string[i] != '\n' )
>
> Ditto for the first clause.

OK

>
>> +                i++;
>> +        } else {
>
>> +            for (j = 0; reject && reject[j]; j++) {
>> +                if (string[i] == reject[j])
>> +                    break;
>
> Use strchr().

That is slower.

>
>> +            }
>> +            if (reject && reject[j])
>> +                break;
>> +        }
>> +    }
>> +    return i;
>> +}
>> +
>
>> +static uint32_t hexstring_to_rgba(const char *p, int len)
>
> This is a misnomer.

What it should be instead?

>
>> +{
>> +    uint32_t ret = 0xFF000000;
>> +    const ColorEntry *entry;
>> +    char color_name[100];
>> +
>> +    if (*p == '#') {
>> +        p++;
>> +        len--;
>> +        if (len == 3) {
>> +            ret |= (convert(p[2]) <<  4) |
>> +                   (convert(p[1]) << 12) |
>> +                   (convert(p[0]) << 20);
>> +        } else if (len == 4) {
>> +            ret  = (convert(p[3]) <<  4) |
>> +                   (convert(p[2]) << 12) |
>> +                   (convert(p[1]) << 20) |
>> +                   (convert(p[0]) << 28);
>> +        } else if (len == 6) {
>> +            ret |=  convert(p[5])        |
>> +                   (convert(p[4]) <<  4) |
>> +                   (convert(p[3]) <<  8) |
>> +                   (convert(p[2]) << 12) |
>> +                   (convert(p[1]) << 16) |
>> +                   (convert(p[0]) << 20);
>> +        } else if (len == 8) {
>> +            ret  =  convert(p[7])        |
>> +                   (convert(p[6]) <<  4) |
>> +                   (convert(p[5]) <<  8) |
>> +                   (convert(p[4]) << 12) |
>> +                   (convert(p[3]) << 16) |
>> +                   (convert(p[2]) << 20) |
>> +                   (convert(p[1]) << 24) |
>> +                   (convert(p[0]) << 28);
>> +        }
>> +    } else {
>
>> +        strncpy(color_name, p, len);
>> +        color_name[len] = '\0';
>
> This is completely wrong.

What should it be instead?

>
>> +
>> +        entry = bsearch(color_name,
>> +                        color_table,
>> +                        (sizeof(color_table)/sizeof(color_table[0])),
>> +                        sizeof(ColorEntry),
>> +                        color_table_compare);
>> +
>> +        if (!entry)
>> +            return ret;
>> +
>> +        ret = entry->rgb_color;
>> +    }
>
>> +    return ret;
>
> Is it specified somewhere that unknown colors should yield black?
> Otherwise, an error code should be returned.

Better not return error and instead display what is already decoded.

>
> Also, you forgot to parse colors in standard X11 scheme
> "rgb:RRRR/GGGG/BBBB".

Are there such files?

>
>> +}
>> +
>> +static int ascii2index(const uint8_t *cpixel, int cpp)
>> +{
>> +    const uint8_t *p = cpixel;
>> +    int n = 0, m = 1, i;
>> +
>> +    for (i = 0; i < cpp; i++) {
>> +        if (*p < ' ' || *p > '~')
>> +            return AVERROR_INVALIDDATA;
>> +        n += (*p++ - ' ') * m;
>> +        m *= 95;
>> +    }
>> +    return n;
>> +}
>> +
>> +static int xpm_decode_frame(AVCodecContext *avctx, void *data,
>> +                            int *got_frame, AVPacket *avpkt)
>> +{
>> +    XPMDecContext *x = avctx->priv_data;
>> +    AVFrame *p=data;
>> +    const uint8_t *end, *ptr = avpkt->data;
>> +    int ncolors, cpp, ret, i, j;
>> +    int64_t size;
>> +    uint32_t *dst;
>> +
>> +    avctx->pix_fmt = AV_PIX_FMT_BGRA;
>> +
>> +    end = avpkt->data + avpkt->size;
>> +    if (memcmp(ptr, "/* XPM */", 9)) {
>> +        av_log(avctx, AV_LOG_ERROR, "missing signature\n");
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>
>> +    ptr += mod_strcspn(ptr, "\"");
>
> If I read this correctly, you are skipping random characters until a
> quote is found. This is not how a robust parser should be written.

Come on.

>
>
>> +    if (sscanf(ptr, "\"%u %u %u %u\",",
>> +               &avctx->width, &avctx->height, &ncolors, &cpp) != 4) {
>
> This is not properly checking the final quote and comma.

Really?

>
>> +        av_log(avctx, AV_LOG_ERROR, "missing image parameters\n");
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    if ((ret = ff_set_dimensions(avctx, avctx->width, avctx->height)) <
>> 0)
>> +        return ret;
>> +
>> +    if ((ret = ff_get_buffer(avctx, p, 0)) < 0)
>> +        return ret;
>> +
>> +    if (ncolors <= 0) {
>> +        av_log(avctx, AV_LOG_ERROR, "invalid number of colors: %d\n",
>> ncolors);
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>
>> +    if (cpp <= 0) {
>> +        av_log(avctx, AV_LOG_ERROR, "invalid number of chars per pixel:
>> %d\n", cpp);
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    size = 1;
>> +    j = 1;
>> +    for (i = 0; i < cpp; i++) {
>> +        size += j*94;
>> +        j *= 95;
>> +    }
>> +    size *= 4;
>
> This is a DoS waiting to happen.

Come on. I fuzzed this with afl-fuzzer up to 25 cycles.

>
>> +
>
>> +    if (size < 0) {
>
> This is deliberately invoking an undefined behaviour.

How?

>
>> +        av_log(avctx, AV_LOG_ERROR, "unsupported number of chars per
>> pixel: %d\n", cpp);
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    av_fast_padded_malloc(&x->pixels, &x->pixels_size, size);
>> +    if ( !x->pixels ) {
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>
>> +    ptr += mod_strcspn(ptr, ",") + 1;
>
> Same remark as above: skipping random contents. Same for other uses of
> mod_strcspn().

It is not skipping random contents.

>
>> +    for (i = 0; i < ncolors; i++) {
>> +        const uint8_t *index;
>> +        int len;
>> +
>> +        ptr += mod_strcspn(ptr, "\"") + 1;
>> +        if (ptr + cpp > end)
>> +            return AVERROR_INVALIDDATA;
>
>> +        index = ptr;
>
> index looks like a misnomer.

It is name for index to pixels in XPM structure.

>
>> +        ptr += cpp;
>> +
>> +        ptr = strstr(ptr, "c ");
>> +        if (ptr) {
>> +            ptr += 2;
>> +        } else {
>> +            return AVERROR_INVALIDDATA;
>> +        }
>> +
>> +        len = strcspn(ptr, "\" ");
>> +
>> +        if ((ret = ascii2index(index, cpp)) < 0)
>> +            return ret;
>> +
>> +        x->pixels[ret] = hexstring_to_rgba(ptr, len);
>> +        ptr += mod_strcspn(ptr, ",") + 1;
>> +    }
>> +
>> +    for (i = 0; i < avctx->height; i++) {
>> +        dst = (uint32_t *)(p->data[0] + i * p->linesize[0]);
>
>> +        ptr += mod_strcspn(ptr, "\"") + 1;
>> +
>> +        for (j = 0; j < avctx->width; j++) {
>> +            if (ptr + cpp > end)
>> +                return AVERROR_INVALIDDATA;
>> +
>> +            if ((ret = ascii2index(ptr, cpp)) < 0)
>> +                return ret;
>> +
>> +            *dst++ = x->pixels[ret];
>> +            ptr += cpp;
>> +        }
>> +        ptr += mod_strcspn(ptr, ",") + 1;
>
> This whole loop can go beyond the end of the input buffer very easily.

Input buffer is padded with NULLs.


More information about the ffmpeg-devel mailing list