Resend: [PATCH] Avoid use of uninitialized values in guid.c

[Chris Shoemaker]

Derek, did you notice that this email didn't make it to the list?  Did
it make it to you?

  -chris

On Sun, Feb 13, 2005 at 08:26:52PM -0500, Chris Shoemaker wrote:
> On Sun, Feb 13, 2005 at 06:13:33PM -0500, Derek Atkins wrote:
> > I'm a bit concerned about a few aspects of this patch, so I haven't
> > applied it.  More comments inline..
> > 
> > Chris Shoemaker <c.shoemaker at cox.net> writes:
> > 
> > > +    char *tmp = "NULLGUID.EMPTY.";
> > >
> > > +    /* 16th space for '\O' */
> > >      for (i = 0; i < 16; i++)
> > > -      null_guid.data[i] = 0;
> > > +      null_guid.data[i] = tmp[i];
> > 
> > What's your reasoning for changing the NULL GUID like this?
> 
> human-friendly memory signature.  It helped in debugging, and since
> the null guid isn't special, any value is as good as any other.
> 
> 
> > 
> > > -    md5_process_bytes(de, sizeof(struct dirent), &guid_context);
> > > -    total += sizeof(struct dirent);
> > > +    md5_process_bytes(de->d_name, strlen(de->d_name), &guid_context);
> > > +    total += strlen(de->d_name);
> > 
> > I'm concerned here that you're reducing the amount of entropy
> > included.  Whereas before you were including the whole dirent object,
> > now you're just including the filename.  That's leaving out the inode
> > number, offset, and recordlen, which can all add entropy to the mix.
> 
> The problem with the original is that some (perhaps undocumented)
> parts of the struct can be uninitialized.  IMHO all this entropy
> gathering is pointless anyway.  There are several places that are
> clearly silly superstition.
> 
> I mean, we only save 128 bytes of entropy, once.  Once we have it,
> hashing more random data doesn't add anything.  Why do we read 512
> bytes of presumably random data from /dev/urandom, and then hash it?
> Why hash data that's supposed to be already random?  And in a way,
> that's the _least_ strange thing we do.
> 
> I know much less about portable C coding than I do about PRNG, but I
> would be _very_ surprised if jumping through all these hoops provides
> _any_ more entropy than simply reading 128 bytes from /dev/random (not
> urandom).
> 
> If you agree, I'd be glad to write the patch.  Otherwise, I guess we
> could read each field of the struct individually, but I think it's
> pretty silly.
> 
> The only thing I intended was to avoid the uninitialized read.
> 
> > 
> > > -  guid_memchunk_init();
> > > +  /* Not needed: taken care of on first malloc.
> > > +  /* guid_memchunk_init(); */
> > 
> > The problem is that you don't know which one is called first,
> > guid_init() or guid_malloc().  If you want to protect the
> > memchunk_init() from being double-called, that's fine.
> > 
> 
> AFAICS, it's not a problem.  We don't use the memchunk until after the
> guid_malloc, so why init it earlier?
> 
> 
> > >  main (int argc, char **argv)
> > >  {
> > > -  scm_boot_guile(argc, argv, main_helper, NULL);
> > > +  /* scm_boot_guile(argc, argv, main_helper, NULL); */
> > > +  main_helper(NULL, argc, argv);
> > >    return 0;
> > 
> > Why did you remove the scheme loading here?
> 
> I don't remember.  Feel free to drop it.
> 
> Thanks for giving my patches attention.
> 
> -chris