[PATCH] ringbuffer: Fix a deadlock in readahead

[Lawrence Rust <lvr@…>]

There is a potential deadlock in ringbuffer::run() due to violation of the lock acquisition hierarchy when it calls safe_read().

If an error is encountered in fileringbuffer::safe_read() then poslock is read locked prior to calling Seek() but run() has already read locked rbwlock which leads to a violation of the lock hierarchy rwlock->poslock->rbrlock->rbwlock.

This fix releases rbwlock prior to calling safe_read.

NB this bug also affects fixes/0.24

[danielk]

Lawrence can you take a look at 9959-2.patch ? My concern is that with the lock undone during read_safe() the rbwpos could be changed during the read.. This was really already a problem because we unlocked rbwlock before we did the update, but I believe 9959-v2.patch might fix both problems by comparing rbwpos with rbwposcopy once we acquire the write lock.

[Lawrence Rust <lvr@…>]

OK, I see what you're trying to do - it adds a level of security in case something changes rbwpos while rbwlock is released. However, I think that you're worrying unnecessarily. The only code that changes rbwpos is ResetReadAhead? (protected) and this is only called from Seek() (public) which obtains a rwlock write lock at entry, or the caller of Seek has done so. Any attempt to write lock rwlock will block until readahead is idle.

What you propose is absolutely safe and protects against future changes. I think the change is for the best.

[Github]

Fixes #9959. This fixes a race deadlock in FileRingBuffer::safe_read(RemoteFile? *rf... and also makes sure we don't update rbwpos with read_return if rbspos has changed since we read in the data.

The first problem was discovered & fixed by Lawrence Rust. I discovered & fixed the other related problem when reviewing his patch.

Branch: master Changeset: 3733544bafeeb7f20bed1334e7f76b4581790429