[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