Notice that git worries about collisions between the 7 char abbreviated sha1s in large repositories, and sometime have to use 10 or 12 chars, etc: https://github.com/blog/2288-git-2-11-has-been-released
"This is exactly what happened in the Linux kernel repository; it now has over 5 million objects, meaning we'd expect collisions with names shorter than 12 hexadecimal characters." I think that speaks for itself. Also, SHA1 is very good at generating a completely different hash for small changes. So to generate a collision with X, you can't have X +/- a few bytes. Regards //Ernst 2017-02-27 13:20 GMT+01:00 Marek Olšák <[email protected]>: > On Mon, Feb 27, 2017 at 10:57 AM, Michel Dänzer <[email protected]> > wrote: > > On 25/02/17 01:56 PM, Timothy Arceri wrote: > >> On 24/02/17 21:02, Marek Olšák wrote: > >>> On Fri, Feb 24, 2017 at 3:18 AM, Timothy Arceri > >>> <[email protected]> wrote: > >>>> On 24/02/17 08:49, Timothy Arceri wrote: > >>>>> On 24/02/17 05:12, Marek Olšák wrote: > >>>>>> On Thu, Feb 23, 2017 at 3:09 AM, Timothy Arceri > >>>>>> <[email protected]> wrote: > >>>>>>> > >>>>>>> From: kdj0c <[email protected]> > >>>>>>> > >>>>>>> V2 (Timothy Arceri): > >>>>>>> - when loading from disk cache also binary insert into memory > cache. > >>>>>>> - check that the binary loaded from disk is the correct size. If > not > >>>>>>> delete the cache item and skip loading from cache. > >>>>>>> --- > >>>>>>> src/gallium/drivers/radeonsi/si_state_shaders.c | 69 > >>>>>>> ++++++++++++++++++++++--- > >>>>>>> 1 file changed, 62 insertions(+), 7 deletions(-) > >>>>>>> > >>>>>>> diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c > >>>>>>> b/src/gallium/drivers/radeonsi/si_state_shaders.c > >>>>>>> index f615aa8..71556f9 100644 > >>>>>>> --- a/src/gallium/drivers/radeonsi/si_state_shaders.c > >>>>>>> +++ b/src/gallium/drivers/radeonsi/si_state_shaders.c > >>>>>>> @@ -36,6 +36,9 @@ > >>>>>>> #include "util/u_memory.h" > >>>>>>> #include "util/u_prim.h" > >>>>>>> > >>>>>>> +#include "util/disk_cache.h" > >>>>>>> +#include "util/mesa-sha1.h" > >>>>>>> + > >>>>>>> /* SHADER_CACHE */ > >>>>>>> > >>>>>>> /** > >>>>>>> @@ -182,10 +185,12 @@ static bool si_load_shader_binary(struct > >>>>>>> si_shader *shader, void *binary) > >>>>>>> */ > >>>>>>> static bool si_shader_cache_insert_shader(struct si_screen > *sscreen, > >>>>>>> void *tgsi_binary, > >>>>>>> - struct si_shader *shader) > >>>>>>> + struct si_shader *shader, > >>>>>>> + bool > >>>>>>> insert_into_disk_cache) > >>>>>>> { > >>>>>>> void *hw_binary; > >>>>>>> struct hash_entry *entry; > >>>>>>> + uint8_t key[CACHE_KEY_SIZE]; > >>>>>>> > >>>>>>> entry = _mesa_hash_table_search(sscreen->shader_cache, > >>>>>>> tgsi_binary); > >>>>>>> if (entry) > >>>>>>> @@ -201,6 +206,12 @@ static bool si_shader_cache_insert_shader( > struct > >>>>>>> si_screen *sscreen, > >>>>>>> return false; > >>>>>>> } > >>>>>>> > >>>>>>> + if (sscreen->b.disk_shader_cache && > insert_into_disk_cache) { > >>>>>>> + _mesa_sha1_compute(tgsi_binary, *((uint32_t > >>>>>>> *)tgsi_binary), key); > >>>>>> > >>>>>> > >>>>>> What happens if we randomly get a sha1 collision? > >>>>> > >>>>> > >>>>> You should stop playing your game which will be rendering incorrectly > >>>>> and by a lotto ticket. > >>>>> > >>>>>> Shouldn't we store the whole key as well? > >>>>> > >>>>> > >>>>> Sure I can add that, its cheap to check here anyway. Although the > other > >>>>> cache stages rely on a collision being improbable. > >>>> > >>>> > >>>> > >>>> For some reason I thought the key was simpler than it is. It seems > >>>> excessive > >>>> to store and compare the tgsi again. I don't think git even worries > >>>> about > >>>> the possibility of a collision and we will be dealing with much > smaller > >>>> amounts of cache items then commits in a git repository. > >>>> > >>>> Thoughts? > >>> > >>> I'll let others comment on this. If nobody comments, checking only the > >>> key can stay. > >> > >> Seems SVN didn't used to worry about collisions either. > >> > >> https://arstechnica.com/security/2017/02/watershed- > sha1-collision-just-broke-the-webkit-repository-others-may-follow/ > > > > What scenario are we concerned about here? Getting a genuine SHA1 > > collision between two shader objects is still as unlikely as it ever was > > (virtually impossible?), and someone with access to the shader cache > > files can wreak havoc much more easily than via a hash collision. > > I'm concerned about the random scenario where 2 shaders can have the > same hash. That is, if the user doesn't do anything wrong and nobody > else corrupts the cache, can we say for sure that a collision will > never ever happen? > > The patch can land as-is, but the topic is still open. > > Marek > _______________________________________________ > mesa-dev mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
