[FFmpeg-cvslog] r29509 - trunk/libswscale/swscale.c

[Ramiro Polla]

On Thu, Aug 13, 2009 at 4:24 PM, Reimar
D?ffinger<Reimar.Doeffinger at gmx.de> wrote:
> On Thu, Aug 13, 2009 at 08:29:54PM +0200, ramiro wrote:
>> Author: ramiro
>> Date: Thu Aug 13 20:29:54 2009
>> New Revision: 29509
>>
>> Log:
>> Protect mmx2 filter code buffers so they are not executable and writeable at
>> the same time (only mmap for now).
>>
>> Modified:
>> ? ?trunk/libswscale/swscale.c
>>
>> Modified: trunk/libswscale/swscale.c
>> ==============================================================================
>> --- trunk/libswscale/swscale.c ? ? ? ?Thu Aug 13 20:28:55 2009 ? ? ? ?(r29508)
>> +++ trunk/libswscale/swscale.c ? ? ? ?Thu Aug 13 20:29:54 2009 ? ? ? ?(r29509)
>> @@ -2814,8 +2814,8 @@ SwsContext *sws_getContext(int srcW, int
>> ? ? ? ? ?if (c->canMMX2BeUsed && (flags & SWS_FAST_BILINEAR))
>> ? ? ? ? ?{
>> ?#ifdef MAP_ANONYMOUS
>> - ? ? ? ? ? ?c->lumMmx2FilterCode = mmap(NULL, MAX_MMX2_FILTER_CODE_SIZE, PROT_EXEC | PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
>> - ? ? ? ? ? ?c->chrMmx2FilterCode = mmap(NULL, MAX_MMX2_FILTER_CODE_SIZE, PROT_EXEC | PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
>> + ? ? ? ? ? ?c->lumMmx2FilterCode = mmap(NULL, MAX_MMX2_FILTER_CODE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
>> + ? ? ? ? ? ?c->chrMmx2FilterCode = mmap(NULL, MAX_MMX2_FILTER_CODE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
>> ?#elif HAVE_VIRTUALALLOC
>> ? ? ? ? ? ? ?c->lumMmx2FilterCode = VirtualAlloc(NULL, MAX_MMX2_FILTER_CODE_SIZE, MEM_COMMIT, PAGE_EXECUTE_READWRITE);
>> ? ? ? ? ? ? ?c->chrMmx2FilterCode = VirtualAlloc(NULL, MAX_MMX2_FILTER_CODE_SIZE, MEM_COMMIT, PAGE_EXECUTE_READWRITE);
>> @@ -2831,6 +2831,11 @@ SwsContext *sws_getContext(int srcW, int
>>
>> ? ? ? ? ? ? ?initMMX2HScaler( ? ? ?dstW, c->lumXInc, c->lumMmx2FilterCode, c->lumMmx2Filter, c->lumMmx2FilterPos, 8);
>> ? ? ? ? ? ? ?initMMX2HScaler(c->chrDstW, c->chrXInc, c->chrMmx2FilterCode, c->chrMmx2Filter, c->chrMmx2FilterPos, 4);
>> +
>> +#ifdef MAP_ANONYMOUS
>> + ? ? ? ? ? ?mprotect(c->lumMmx2FilterCode, MAX_MMX2_FILTER_CODE_SIZE, PROT_EXEC | PROT_READ);
>> + ? ? ? ? ? ?mprotect(c->chrMmx2FilterCode, MAX_MMX2_FILTER_CODE_SIZE, PROT_EXEC | PROT_READ);
>
> Both for mmap and mprotect: Why PROT_READ though? I don't think EXEC without read is possible on
> most architectures, but still we do not need read access in any case I
> think...

Mans, or anyone else that knows an extensive amount of platforms, can
you comment on this?

> Also I don't think that's ok, MAP_ANONYMOUS does not imply that mprotect is
> available, this would have to be an extra configure check or something.

FATE hasn't budged. Can we wait for such a system before adding another check?

> And the Windows code should be
> DWORD dummy;
> VirtualProtect(c->lumMmx2FilterCode, MAX_MMX2_FILTER_CODE_SIZE, PAGE_EXECUTE, &dummy);
> with VirtualAlloc using PAGE_READWRITE.
> And yes, for some idiotic reason, dummy is necessary.

> In all cases I also suggest to check for mprotect/VirtualProtect errors
> (VirtualProtext returns 0 on failure).

Hmm, we should also check that the allocation worked. I'll work on some patches.