[MPlayer-dev-eng] "lockless" cache2 looks buggy

[Rich Felker]

On Fri, Aug 25, 2006 at 10:46:58PM +0200, Denis Vlasenko wrote:
> Hello mplayer coders,
> 
> I was looking into cache2.c. It says that it is a lockless
> cache implementation.
> 
> I see s few issues with the code. First issue is the most serious,
> since it looks like real bug.
> 
> (1) Non-atomic updated of metadata.
> 
> Main thread reads cached data using cache_read(), which uses
> off_t variables s->min_filepos, s->max_filepos etc.
> They are 64-bit variables on most systems:
> 
>     newb=s->max_filepos-s->read_filepos; // new bytes in the buffer
>     pos=s->read_filepos - s->offset;
>     if(pos<0) pos+=s->buffer_size; else
>     if(pos>=s->buffer_size) pos-=s->buffer_size;
>     if(newb>s->buffer_size-pos) newb=s->buffer_size-pos; // handle wrap...
>     if(newb>size) newb=size;
>     memcpy(buf,&s->buffer[pos],newb);
> 
> However, caching thread reads new data into circular buffer
> and updates those same variables in cache_fill():
> 
>   back2=s->buffer_size-(space+newb); // max back size
>   if(s->min_filepos<(read-back2)) s->min_filepos=read-back2;
>   len=stream_read(s->stream,&s->buffer[pos],space);
>   if(!len) s->eof=1;
>   s->max_filepos+=len;
>   if(pos+len>=s->buffer_size){
>       // wrap...
>       s->offset+=s->buffer_size;
>   }
> 
> These updates are not atomic on 32-bit architectures (i386).
> Therefore caching thread can update lower half of a value, then it
> may get rescheduled with CPU given to main thread, which then
> will use bogus value of half-updated variable.
> 
> If you don't believe me, add asm() statements as shown below:
> 
> asm("#1");
>   s->max_filepos+=len;
> asm("#1");

Maybe this works:

union { off_t o; double d; } foo = { s->max_filepos + len };
*(double *)&s->max_filepos = foo.d;

?? I'm sure there are better implementations of the same idea.

Rich