[Ffmpeg-devel] [PATCH] OAEP

Michael Niedermayer michaelni
Fri Mar 16 01:03:51 CET 2007


Hi

On Thu, Mar 15, 2007 at 08:46:39PM +0100, Vladimir Serbinenko wrote:
> 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

yes still i would prefer if you could split it and submit it seperately


> > [...]
> >
> >   
> >> 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)

you only have to use 
struct AVSHA1 *c=av_malloc(av_sha1_size);
...
av_free(c);
or
DECLARE_ALIGNED_16(uint8_t, tmp[av_sha1_size]);
struct AVSHA1 *c= tmp;

of course we can disscuss changing this API if you want, its main advanatge
is that AVSHA1 can be changed without breaking existing applications which
link to libavutil and at the same time doesnt require malloc() and gives
the user application full control of where AVSHA1 is located


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

well *malloc() and *free() need time, a temporary buffer allocated outside
would be faster, though i dont know how often this function gets called so
maybe the malloc() time is negligible?
if its negligible then iam fine with leaving the av_malloc() call in there


[...]
> >> @@ -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

well that is strange as the sha1 code passes its own selftest as it is
so please provide more information on what does not work

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070316/4646fd8b/attachment.pgp>



More information about the ffmpeg-devel mailing list