[FFmpeg-devel] [PATCH] Add XPM decoder

Paul B Mahol onemda at gmail.com
Sun Mar 12 23:30:30 EET 2017


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


More information about the ffmpeg-devel mailing list