[Ffmpeg-devel] [PATCH] OAEP

[Vladimir Serbinenko]

Michael Niedermayer a ?crit :
> Hi
>
> On Thu, Mar 15, 2007 at 06:39:26PM +0100, Vladimir Serbinenko wrote:
>   
>> Sorry. A fault of attention. I send the new patch
>> Ismail D?nmez a ?crit :
>>     
>>> On Thursday 15 March 2007 19:06:16 Vladimir Serbinenko wrote:
>>>   
>>>       
>>>> Hello Guys. I wrote a patch for supporting OAEP and I also corrected
>>>> SHA1 which din't work when compiled in avutil because of be2me_*. I
>>>> replaced them by AV_(R/W)B* I also added AV_WB64 to intreadwrite.h. RSA
>>>>         
>
> the OEAP stuff should be in a seperate patch not in the same which fixes
> SHA1 bugs
>
>
>   
Well actualy it's in the same just because I discovered theese bugs
writing OAEP support
> [...]
>
>   
>> Index: sha1.h
>> ===================================================================
>> --- sha1.h	(r??vision 8386)
>> +++ sha1.h	(copie de travail)
>> @@ -1,10 +1,15 @@
>>  #ifndef AV_SHA1_H
>>  #define AV_SHA1_H
>>  
>> -extern const int av_sha1_size;
>> +#define AV_SHA1_SIZE 20
>>  
>> -struct AVSHA1;
>> +typedef struct AVSHA1 {
>> +    uint64_t count;
>> +    uint8_t buffer[64];
>> +    uint32_t state[5];
>> +} AVSHA1;
>>     
>
> this breaks the seperation of implementation and interface
> which requires a seperate disscussion if you think this has some advantages
>
>   
The code written in sha1 never allocates the context so we have either
change init or put AVSHA1 in sha1.h. I personally prefer the first
possibility (it was actually a bugfix so I changed as few as possible)
>   
>>  
>> +
>>  void av_sha1_init(struct AVSHA1* context);
>>     
>
> cosmetic change
>
>   
Even not intended
> [...]
>   
>> Index: intreadwrite.h
>> ===================================================================
>> --- intreadwrite.h	(r??vision 8386)
>> +++ intreadwrite.h	(copie de travail)
>> @@ -69,7 +69,17 @@
>>                      ((uint8_t*)(p))[2] = (d)>>8; \
>>                      ((uint8_t*)(p))[1] = (d)>>16; \
>>                      ((uint8_t*)(p))[0] = (d)>>24; }
>> +#define AV_WB64(p, d) { \
>> +                    ((uint8_t*)(p))[7] = (d); \
>> +                    ((uint8_t*)(p))[6] = (d)>>8; \
>> +                    ((uint8_t*)(p))[5] = (d)>>16; \
>> +                    ((uint8_t*)(p))[4] = (d)>>24; \
>> +                    ((uint8_t*)(p))[3] = (d)>>32; \
>> +                    ((uint8_t*)(p))[2] = (d)>>40; \
>> +                    ((uint8_t*)(p))[1] = (d)>>48; \
>> +                    ((uint8_t*)(p))[0] = (d)>>56; }
>>  
>>     
>
> this should be in a seperate patch and should rather look like
> #define AV_WB64(p, d) { \
>     AV_WB32(           p   , (d)>>32)  \
>     AV_WB32((uint8_t*)(p)+4,  d     ) }
>
>
>
>   
>> +
>>  #define AV_RL8(x)  AV_RB8(x)
>>     
>
> cosmetic change
>
>
> [...]
>   
>> +/*
>> +  We have to calculate:
>> +  r=o1^LSHA1(o2)
>> +  o2^LSHA1(r)
>> +*/
>> +/* WARNING: after RSA deciphering first byte is not a part of OAEP */
>> +int
>> +av_parse_oaep(uint8_t *inbuf, uint8_t *outbuf, int inlen)
>>     
>
> comment is not doxygen compatible, also documentation for exported functions
> should be in the header not source
>
>
>   
>> +{
>> +  uint8_t *o1, *o2;
>> +  uint8_t r[AV_SHA1_SIZE*3];
>> +  uint8_t cd[AV_SHA1_SIZE];
>>     
>
> indention in ffmpeg should be 4 spaces
>
>   
>   
>> +  uint8_t *tbuf;
>> +  int i;
>> +  int outlen;
>> +  AVSHA1 hashctx;
>> +
>> +  if(inlen<=AV_SHA1_SIZE)
>> +    return -1;
>> +  /* Split the message into 2 parts*/
>> +  o1=inbuf;
>> +  o2=inbuf+AV_SHA1_SIZE;
>> +
>> +  /* Allocate tbuf*/
>> +  tbuf=av_malloc(inlen);
>>     
>
> id leave the allocation of the temporary buffer to the caller this would
> avoid a dependancy on av_malloc*() (less dependancies are good IMHO, makes
> using the code outside libavutil easier)
>
>   
Why should caller bother about our temporary buffers? It should know
only about input and output buffers
> [...]
>
>   
>> +uint8_t *fast_memcpy(uint8_t *a, uint8_t *b, int n)
>> +{
>> +  return memcpy(a,b,n);
>> +}
>>     
>
> this doesnt belong in here
>
>   
It was just for the test
> [...]
>
>   
>>  /* (R0+R1), R2, R3, R4 are the different operations used in SHA1 */
>> -#define blk0(i) (block[i] = be2me_32(((uint32_t*)buffer)[i]))
>> +#define blk0(i) (block[i] = AV_RB32(&((uint32_t*)buffer)[i]))
>>  #define blk(i) (block[i] = rol(block[i-3]^block[i-8]^block[i-14]^block[i-16],1))
>>  
>>  #define R0(v,w,x,y,z,i) z+=((w&(x^y))^y)    +blk0(i)+0x5A827999+rol(v,5);w=rol(w,30);
>> @@ -37,7 +31,7 @@
>>  #ifdef CONFIG_SMALL
>>      for(i=0; i<80; i++){
>>          int t;
>> -        if(i<16) t= be2me_32(((uint32_t*)buffer)[i]);
>> +        if(i<16) t= AV_RB32(((uint32_t*)buffer)[i]);
>>          else     t= rol(block[i-3]^block[i-8]^block[i-14]^block[i-16],1);
>>          block[i]= t;
>>          t+= e+rol(a,5);
>>     
>
> this change requires benchmarks and object size comparissions against a
> variant which never passes user arrays directly in av_sha1_update()
>
>
>   
>> @@ -114,7 +108,8 @@
>>  
>>  void av_sha1_final(AVSHA1* ctx, uint8_t digest[20]){
>>      int i;
>> -    uint64_t finalcount= be2me_64(ctx->count<<3);
>> +    uint64_t finalcount;
>> +    AV_WB64(&finalcount,(ctx->count<<3));
>>  
>>      av_sha1_update(ctx, "\200", 1);
>>      while ((ctx->count & 63) != 56) {
>> @@ -122,7 +117,7 @@
>>      }
>>      av_sha1_update(ctx, &finalcount, 8);  /* Should cause a transform() */
>>      for(i=0; i<5; i++)
>> -        ((uint32_t*)digest)[i]= be2me_32(ctx->state[i]);
>> +        AV_WB32(&((uint32_t*)digest)[i],(ctx->state[i]));
>>  }
>>     
>
> these are is unneeded
>
>   
Without this change it simply doesn't work
> [...]
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel