Re: [Bug-wget] [PATCH] Update HSTS info

[Tim Ruehsen]

Hi Ander,

nice work and thanks for your contribution !

Just a few little remarks...

Please drop the second line.
           xfree (store);
           store = NULL;

Could you rename f to fp ? Just fits better with the rest of the code.
+  FILE *f = NULL;


+          flock (fd, LOCK_UN);
+          fclose (f);
You are using buffered I/O, fclose() is effectively a write() + close(). 
That opens a hole here if you unlock the file before write().
IMO, to avoid that hole you could just drop the explicit unlock. It will be 
performed automatically when the file is closed by fclose().


Regards,

Tim

On Wednesday 07 October 2015 22:13:02 Ander Juaristi wrote:
> Hi,
> 
> I've written two patches for the HSTS code. Please review them when you have
> time.
> 
> In august, dkg catched a potential race condition in Wget's HSTS code
> (replayed message below). The first patch aims to solve that issue using
> flock(2). It works well on my machine: I've tested it by spawning two Wget
> processes on top of gdb.
> 
> The second patch fixes what's in my opinion a critical bug. It only
> triggered when more than one Wget processes used the same HSTS database
> simultaneously, and that's why it's gone unnoticed until now. I noticed it
> while I was working on the first patch, so it could as well be part of it,
> but IMO this bug is more related to the principles of our HSTS design
> itself than to the specific race condition issue.
> > On 08/18/2015 09:12 PM, Ander Juaristi wrote:
> >> On 08/10/2015 06:06 PM, Daniel Kahn Gillmor wrote:
> >> 
> >> This sounds like there might still be a race condition where one
> >> process's changes get clobbered.  can we make any atomicity guarantees
> >> for users who might be concerned about this?
> > 
> > You're right. My fault not to take this into account. This could be fixed
> > with flock/fcntl. I think they're both in gnulib.
> > 
> > These last two issues require code changes. I'll take the responsibility
> > to fix them, but outside of GSoC.  The first one requires a bit of
> > consensus before coding anything, but the second one seems a bit more
> > straightforward. For now, I attach the two previous patches. The first
> > one (the HSTS docs) amended with dkg's suggestions. If there're no more
> > complaints, I think they can be pushed, because Wget's behaviour has not
> > changed yet. When we implement any of the ideas proposed above, we'll
> > update the docs.
> Regards,
> - AJ