<div dir="ltr"><div><div><div>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: <a href="https://github.com/blog/2288-git-2-11-has-been-released">https://github.com/blog/2288-git-2-11-has-been-released</a> </div> "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. </div>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. </div> <div><div>Regards </div><div>//Ernst </div><div> <div class="gmail_extra"> <div class="gmail_quote">2017-02-27 13:20 GMT+01:00 Marek Olšák <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>>: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Mon, Feb 27, 2017 at 10:57 AM, Michel Dänzer <<a href="mailto:michel@daenzer.net">michel@daenzer.net</a>> 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 >>> <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> 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 >>>>>> <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote: >>>>>>> >>>>>>> From: kdj0c <<a href="mailto:kdj0c@djinvi.net">kdj0c@djinvi.net</a>> >>>>>>> >>>>>>> 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. >> >> <a href="https://arstechnica.com/security/2017/02/watershed-sha1-collision-just-broke-the-webkit-repository-others-may-follow/" rel="noreferrer" target="_blank">https://arstechnica.com/security/2017/02/watershed-sha1-collision-just-broke-the-webkit-repository-others-may-follow/</a> > > 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. </div></div>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 <div class="gmail-HOEnZb"><div class="gmail-h5">_______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div></div></div>