[FFmpeg-devel] Add XPM decoder

Xpm decoder was added
Also added xpm_pipe demuxer with its probe function

Signed-off-by: Paras Chadha <paraschadha18@gmail.com>
---
 Changelog                |   1 +
 doc/general.texi         |   2 +
 libavcodec/Makefile      |   1 +
 libavcodec/allcodecs.c   |   1 +
 libavcodec/avcodec.h     |   1 +
 libavcodec/codec_desc.c  |   7 +
 libavcodec/version.h     |   4 +-
 libavcodec/xpmdec.c      | 426 +++++++++++++++++++++++++++++++++++++++++++++++
 libavformat/Makefile     |   1 +
 libavformat/allformats.c |   1 +
 libavformat/img2.c       |   1 +
 libavformat/img2dec.c    |  10 ++
 12 files changed, 454 insertions(+), 2 deletions(-)
 create mode 100644 libavcodec/xpmdec.c

--
2.4.11

Le duodi 22 ventôse, an CCXXV, Paras Chadha a écrit :
> Xpm decoder was added
> Also added xpm_pipe demuxer with its probe function
> 
> Signed-off-by: Paras Chadha <paraschadha18@gmail.com>
> ---
>  Changelog                |   1 +
>  doc/general.texi         |   2 +
>  libavcodec/Makefile      |   1 +
>  libavcodec/allcodecs.c   |   1 +
>  libavcodec/avcodec.h     |   1 +
>  libavcodec/codec_desc.c  |   7 +
>  libavcodec/version.h     |   4 +-
>  libavcodec/xpmdec.c      | 426 +++++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/img2.c       |   1 +
>  libavformat/img2dec.c    |  10 ++
>  12 files changed, 454 insertions(+), 2 deletions(-)
>  create mode 100644 libavcodec/xpmdec.c
> 
> diff --git a/Changelog b/Changelog
> index 13628ca..716b6ff 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -26,6 +26,7 @@ version <next>:
>  - native Opus encoder
>  - ScreenPressor decoder
>  - incomplete ClearVideo decoder
> +- XPM decoder
> 
>  version 3.2:
>  - libopenmpt demuxer
> diff --git a/doc/general.texi b/doc/general.texi
> index 30450c0..83f54b3 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -607,6 +607,8 @@ following image formats are supported:
>      @tab WebP image format, encoding supported through external library libwebp
>  @item XBM  @tab X @tab X
>      @tab X BitMap image format
> +@item XPM  @tab X @tab X
> +    @tab X PixMap image format
>  @item XFace @tab X @tab X
>      @tab X-Face image format
>  @item XWD  @tab X @tab X
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 65ccbad..b8d7a00 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -650,6 +650,7 @@ OBJS-$(CONFIG_XFACE_ENCODER)           += xfaceenc.o xface.o
>  OBJS-$(CONFIG_XL_DECODER)              += xl.o
>  OBJS-$(CONFIG_XMA1_DECODER)            += wmaprodec.o wma.o wma_common.o
>  OBJS-$(CONFIG_XMA2_DECODER)            += wmaprodec.o wma.o wma_common.o
> +OBJS-$(CONFIG_XPM_DECODER)             += xpmdec.o
>  OBJS-$(CONFIG_XSUB_DECODER)            += xsubdec.o
>  OBJS-$(CONFIG_XSUB_ENCODER)            += xsubenc.o
>  OBJS-$(CONFIG_XWD_DECODER)             += xwddec.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 074efd4..b7d03ad 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -378,6 +378,7 @@ static void register_all(void)
>      REGISTER_ENCDEC (XBM,               xbm);
>      REGISTER_ENCDEC (XFACE,             xface);
>      REGISTER_DECODER(XL,                xl);
> +    REGISTER_DECODER(XPM,               xpm);
>      REGISTER_ENCDEC (XWD,               xwd);
>      REGISTER_ENCDEC (Y41P,              y41p);
>      REGISTER_DECODER(YLC,               ylc);
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 30ac236..e32f579 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -439,6 +439,7 @@ enum AVCodecID {
>      AV_CODEC_ID_FMVC,
>      AV_CODEC_ID_SCPR,
>      AV_CODEC_ID_CLEARVIDEO,
> +    AV_CODEC_ID_XPM,
> 
>      /* various PCM "codecs" */
>      AV_CODEC_ID_FIRST_AUDIO = 0x10000,     ///< A dummy id pointing at the start of audio codecs
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 06bcfc3..88cfddb 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -1591,6 +1591,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .props     = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS,
>      },
>      {
> +        .id        = AV_CODEC_ID_XPM,
> +        .type      = AVMEDIA_TYPE_VIDEO,
> +        .name      = "xpm",
> +        .long_name = NULL_IF_CONFIG_SMALL("XPM (X PixMap) image"),
> +        .props     = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS,
> +    },
> +    {
>          .id        = AV_CODEC_ID_XWD,
>          .type      = AVMEDIA_TYPE_VIDEO,
>          .name      = "xwd",
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index b00e011..3ed5a71 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,8 +28,8 @@
>  #include "libavutil/version.h"
> 
>  #define LIBAVCODEC_VERSION_MAJOR  57
> -#define LIBAVCODEC_VERSION_MINOR  82
> -#define LIBAVCODEC_VERSION_MICRO 102
> +#define LIBAVCODEC_VERSION_MINOR  83
> +#define LIBAVCODEC_VERSION_MICRO 100
> 
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>                                                 LIBAVCODEC_VERSION_MINOR, \
> diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c
> new file mode 100644
> index 0000000..7a1f4e1
> --- /dev/null
> +++ b/libavcodec/xpmdec.c
> @@ -0,0 +1,426 @@
> +/*
> + * XPM image format
> + *
> + * Copyright (c) 2012 Paul B Mahol
> + * Copyright (c) 2017 Paras Chadha
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/parseutils.h"
> +#include "libavutil/avstring.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +
> +typedef struct XPMContext {

> +    uint32_t  *pixels;
> +    int      pixels_size;

The spacing is strange.

> +} 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.

> +};
> +

> +static int convert(uint8_t x)

convert is not a very good name.

> +{

> +    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.

> +        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.

> +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.

> +        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.

> +                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.

> +        } else if (string[i] == '/' && string[i+1] == '/') {
> +            i += 2;

> +            while ( string && string[i] && string[i] != '\n' )

Ditto for the first clause.

> +                i++;
> +        } else {

> +            for (j = 0; reject && reject[j]; j++) {
> +                if (string[i] == reject[j])
> +                    break;

Use strchr().

> +            }
> +            if (reject && reject[j])
> +                break;
> +        }
> +    }
> +    return i;
> +}
> +

> +static uint32_t hexstring_to_rgba(const char *p, int len)

This is a misnomer.

> +{
> +    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.

> +
> +        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.

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

> +}
> +
> +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.


> +    if (sscanf(ptr, "\"%u %u %u %u\",",
> +               &avctx->width, &avctx->height, &ncolors, &cpp) != 4) {

This is not properly checking the final quote and comma.

> +        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.

> +

> +    if (size < 0) {

This is deliberately invoking an undefined behaviour.

> +        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().

> +    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.

> +        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.

> +    }
> +
> +    p->key_frame = 1;
> +    p->pict_type = AV_PICTURE_TYPE_I;
> +
> +    *got_frame = 1;
> +
> +    return avpkt->size;
> +}
> +
> +static av_cold int xpm_decode_close(AVCodecContext *avctx)
> +{
> +    XPMDecContext *x = avctx->priv_data;
> +    av_freep(&x->pixels);
> +
> +    return 0;
> +}
> +
> +AVCodec ff_xpm_decoder = {
> +    .name           = "xpm",
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_XPM,
> +    .priv_data_size = sizeof(XPMDecContext),
> +    .close          = xpm_decode_close,
> +    .decode         = xpm_decode_frame,
> +    .capabilities   = CODEC_CAP_DR1,
> +    .long_name      = NULL_IF_CONFIG_SMALL("XPM (X PixMap) image")
> +};
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index fc2d760..f56ef16 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -241,6 +241,7 @@ OBJS-$(CONFIG_IMAGE_SGI_PIPE_DEMUXER)     += img2dec.o img2.o
>  OBJS-$(CONFIG_IMAGE_SUNRAST_PIPE_DEMUXER) += img2dec.o img2.o
>  OBJS-$(CONFIG_IMAGE_TIFF_PIPE_DEMUXER)    += img2dec.o img2.o
>  OBJS-$(CONFIG_IMAGE_WEBP_PIPE_DEMUXER)    += img2dec.o img2.o
> +OBJS-$(CONFIG_IMAGE_XPM_PIPE_DEMUXER)     += img2dec.o img2.o
>  OBJS-$(CONFIG_INGENIENT_DEMUXER)         += ingenientdec.o rawdec.o
>  OBJS-$(CONFIG_IPMOVIE_DEMUXER)           += ipmovie.o
>  OBJS-$(CONFIG_IRCAM_DEMUXER)             += ircamdec.o ircam.o pcm.o
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index 132e58b..09e62c3 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -372,6 +372,7 @@ static void register_all(void)
>      REGISTER_DEMUXER (IMAGE_SUNRAST_PIPE,    image_sunrast_pipe);
>      REGISTER_DEMUXER (IMAGE_TIFF_PIPE,       image_tiff_pipe);
>      REGISTER_DEMUXER (IMAGE_WEBP_PIPE,       image_webp_pipe);
> +    REGISTER_DEMUXER (IMAGE_XPM_PIPE,        image_xpm_pipe);
> 
>      /* external libraries */
>      REGISTER_MUXER   (CHROMAPRINT,      chromaprint);
> diff --git a/libavformat/img2.c b/libavformat/img2.c
> index f9f53ff..29df4f0 100644
> --- a/libavformat/img2.c
> +++ b/libavformat/img2.c
> @@ -75,6 +75,7 @@ const IdStrMap ff_img_tags[] = {
>      { AV_CODEC_ID_V210X,      "yuv10"    },
>      { AV_CODEC_ID_WEBP,       "webp"     },
>      { AV_CODEC_ID_XBM,        "xbm"      },
> +    { AV_CODEC_ID_XPM,        "xpm"      },
>      { AV_CODEC_ID_XFACE,      "xface"    },
>      { AV_CODEC_ID_XWD,        "xwd"      },
>      { AV_CODEC_ID_NONE,       NULL       }
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index c3c2cf3..b454071 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
> @@ -943,6 +943,15 @@ static int pam_probe(AVProbeData *p)
>      return pnm_magic_check(p, 7) ? pnm_probe(p) : 0;
>  }
> 
> +static int xpm_probe(AVProbeData *p)
> +{
> +    const uint8_t *b = p->buf;
> +
> +    if (AV_RB64(b) == 0x2f2a2058504d202a && *(b+8) == '/')
> +        return AVPROBE_SCORE_MAX - 1;
> +    return 0;
> +}
> +
>  #define IMAGEAUTO_DEMUXER(imgname, codecid)\
>  static const AVClass imgname ## _class = {\
>      .class_name = AV_STRINGIFY(imgname) " demuxer",\
> @@ -983,3 +992,4 @@ IMAGEAUTO_DEMUXER(sgi,     AV_CODEC_ID_SGI)
>  IMAGEAUTO_DEMUXER(sunrast, AV_CODEC_ID_SUNRAST)
>  IMAGEAUTO_DEMUXER(tiff,    AV_CODEC_ID_TIFF)
>  IMAGEAUTO_DEMUXER(webp,    AV_CODEC_ID_WEBP)
> +IMAGEAUTO_DEMUXER(xpm,     AV_CODEC_ID_XPM)

Regards,

On 3/12/17, Nicolas George <george@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.

Le duodi 22 ventôse, an CCXXV, Paul B Mahol a écrit :
> Because conflicting entries have not been added yet. Last time I
> compared it was different.

Well, unlike some people on this mailing-list, I actually check my facts
before sending a mail. And I repeat, I did not see any conflict.

> Also when Last time I tried it was soo slow that made 10k colors very
> slow to decode.

Then make it faster, since obviously you are capable of it, but
duplicating it is unacceptable.

> > convert is not a very good name.
> 
> OK, what you propose?

hex_char_to_number(), for example.

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

I very much doubt that.

> >> +static uint32_t hexstring_to_rgba(const char *p, int len)
> > This is a misnomer.
> What it should be instead?

Probably "color_string_to_rgba()".

> >> +        strncpy(color_name, p, len);
> >> +        color_name[len] = '\0';
> > This is completely wrong.
> What should it be instead?

It should check len against sizeof(color_name), obviously. Could you not
find it yourself? The magic number in the size of the array should have
been a dead giveaway.

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

I strongly disagree.

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

Of course.

> > 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.

Are you trying to communicate?

> >> +    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?

Yes, really. Check the man page of sscanf() if you do not remember how
it works.

> >> +    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.

You should have given 25 seconds of thought instead. An attacker has
only to make cpp just large enough to eat all memory and give a few
colors to force the allocation of very sparse entries to actually access
it to make a DoS.

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

The arithmetic can not make size negative, only an integer overflow.

Furthermore, if there are several integer overflows, size can come back
positive but smaller than what will be accessed later, which is really
really bad.

Actually, I think you just pushed an exploitable security hole.

> >> +    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.

Oh? Then pray tell me what part of the code detects an invalid file with
random text at between the quote and the comma?

> > index looks like a misnomer.
> It is name for index to pixels in XPM structure.

An index is a number, this is not a number.

> > This whole loop can go beyond the end of the input buffer very easily.
> Input buffer is padded with NULLs.

My bad, re-reading it more carefully, you were right on this instance.

Regards,

On 3/12/17, Nicolas George <george@nsup.org> wrote:
> Le duodi 22 ventose, an CCXXV, Paul B Mahol a ecrit :
>> Because conflicting entries have not been added yet. Last time I
>> compared it was different.
>
> Well, unlike some people on this mailing-list, I actually check my facts
> before sending a mail. And I repeat, I did not see any conflict.

https://en.wikipedia.org/wiki/X11_color_names#Clashes_between_web_and_X11_colors_in_the_CSS_color_scheme

>
>> Also when Last time I tried it was soo slow that made 10k colors very
>> slow to decode.
>
> Then make it faster, since obviously you are capable of it, but
> duplicating it is unacceptable.

Please, give yourself a big break.

>
>> > convert is not a very good name.
>>
>> OK, what you propose?
>
> hex_char_to_number(), for example.

OK

>
>> >> +            for (j = 0; reject && reject[j]; j++) {
>> >> +                if (string[i] == reject[j])
>> >> +                    break;
>> > Use strchr().
>> That is slower.
>
> I very much doubt that.
>
>> >> +static uint32_t hexstring_to_rgba(const char *p, int len)
>> > This is a misnomer.
>> What it should be instead?
>
> Probably "color_string_to_rgba()".

OK

>
>> >> +        strncpy(color_name, p, len);
>> >> +        color_name[len] = '\0';
>> > This is completely wrong.
>> What should it be instead?
>
> It should check len against sizeof(color_name), obviously. Could you not
> find it yourself? The magic number in the size of the array should have
> been a dead giveaway.

OK

>
>> Better not return error and instead display what is already decoded.
>
> I strongly disagree.

I absolutely disagree with you on this one.

>
>> > Also, you forgot to parse colors in standard X11 scheme
>> > "rgb:RRRR/GGGG/BBBB".
>> Are there such files?
>
> Of course.

I yet have to find such files in wild.

>
>> > 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.
>
> Are you trying to communicate?

I will just ignore this one.

>
>> >> +    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?
>
> Yes, really. Check the man page of sscanf() if you do not remember how
> it works.

I will just ignore this one.

>
>> >> +    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.
>
> You should have given 25 seconds of thought instead. An attacker has
> only to make cpp just large enough to eat all memory and give a few
> colors to force the allocation of very sparse entries to actually access
> it to make a DoS.

I will just ignore this one.

>
>> >> +    if (size < 0) {
>> > This is deliberately invoking an undefined behaviour.
>> How?
>
> The arithmetic can not make size negative, only an integer overflow.
>
> Furthermore, if there are several integer overflows, size can come back
> positive but smaller than what will be accessed later, which is really
> really bad.
>
> Actually, I think you just pushed an exploitable security hole.

I dont think so. But anyway will be "fixed".

>
>> >> +    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.
>
> Oh? Then pray tell me what part of the code detects an invalid file with
> random text at between the quote and the comma?

Better to be robust and show image instead on breaking on corrupted stuff
like nonascii chars in comments.

Le duodi 22 ventôse, an CCXXV, Paul B Mahol a écrit :
> I absolutely disagree with you on this one.

I may have formulated the things a bit sarcastically, but the technical
facts remain, and you need to address them.

Regards,

On 3/12/17, Nicolas George <george@nsup.org> wrote:
> Le duodi 22 ventose, an CCXXV, Paul B Mahol a ecrit :
>> I absolutely disagree with you on this one.
>
> I may have formulated the things a bit sarcastically, but the technical
> facts remain, and you need to address them.

Yes, I addressed this specific one - Ignoring it.

Dumping all data because of single corrupted byte is bad idea.

On Sun, 12 Mar 2017, Paul B Mahol wrote:

> On 3/12/17, Nicolas George <george@nsup.org> wrote:
>> Le duodi 22 ventose, an CCXXV, Paul B Mahol a ecrit :
>>> I absolutely disagree with you on this one.
>>
>> I may have formulated the things a bit sarcastically, but the technical
>> facts remain, and you need to address them.
>
> Yes, I addressed this specific one - Ignoring it.
>
> Dumping all data because of single corrupted byte is bad idea.

You should set either AVFrame->flags to AV_FRAME_FLAG_CORRUPT or 
AVFrame->decode_error_flags to something, so the API user can distinguish 
between a frame with possible guessed colors and a frame without such 
guesses.

Regards,
Marton

Le tridi 23 ventôse, an CCXXV, Marton Balint a écrit :
> You should set either AVFrame->flags to AV_FRAME_FLAG_CORRUPT or
> AVFrame->decode_error_flags to something, so the API user can distinguish
> between a frame with possible guessed colors and a frame without such
> guesses.

Indeed, that would be the correct way of doing things. I forgot about
this flag, thanks.

Regards,