On 3/12/17, Nicolas George <geo...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel