Hi Tim,
Thanks for your comments. I re-send the first patch. No changes on the second.
On 10/08/2015 10:37 AM, Tim Ruehsen wrote:
+ 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().
You were right, the lock is released by fclose(). Obvious, isn't it? I mean,
that's what one would expect.
I thought about this but I wasn't really sure. The docs are not 100% clear IMO:
"Furthermore, the lock is released either by an explicit LOCK_UN operation
on any of these duplicate descriptors,
or when all such descriptors have been closed."
Regards,
- AJ
>From dab29c0b517d4fedaf7df46bd80fc506dd3699ad Mon Sep 17 00:00:00 2001
From: Ander Juaristi <[email protected]>
Date: Mon, 5 Oct 2015 23:03:45 +0200
Subject: [PATCH] Fix potential race condition
* src/hsts.c (hsts_read_database): get an open file handle
instead of a file name.
(hsts_store_dump): get an open file handle
instead of a file name.
(hsts_store_open): open the file and pass the open file handle.
(hsts_store_save): lock the file before the read-merge-dump
process.
Reported-by: Daniel Kahn Gillmor <[email protected]>
---
src/hsts.c | 126 +++++++++++++++++++++++++++++++++----------------------------
1 file changed, 68 insertions(+), 58 deletions(-)
diff --git a/src/hsts.c b/src/hsts.c
index 5c4ca35..ab2f41c 100644
--- a/src/hsts.c
+++ b/src/hsts.c
@@ -39,13 +39,16 @@ as that of the covered work. */
#include "c-ctype.h"
#ifdef TESTING
#include "test.h"
-#include <unistd.h> /* for unlink(), used only in tests */
#endif
+#include <unistd.h>
+#include <sys/types.h>
#include <stdlib.h>
#include <time.h>
#include <sys/stat.h>
#include <string.h>
+#include <stdio.h>
+#include <sys/file.h>
struct hsts_store {
struct hash_table *table;
@@ -263,9 +266,8 @@ hsts_store_merge (hsts_store_t store,
}
static bool
-hsts_read_database (hsts_store_t store, const char *file, bool merge_with_existing_entries)
+hsts_read_database (hsts_store_t store, FILE *fp, bool merge_with_existing_entries)
{
- FILE *fp = NULL;
char *line = NULL, *p;
size_t len = 0;
int items_read;
@@ -279,67 +281,54 @@ hsts_read_database (hsts_store_t store, const char *file, bool merge_with_existi
func = (merge_with_existing_entries ? hsts_store_merge : hsts_new_entry);
- fp = fopen (file, "r");
- if (fp)
+ while (getline (&line, &len, fp) > 0)
{
- while (getline (&line, &len, fp) > 0)
- {
- for (p = line; c_isspace (*p); p++)
- ;
-
- if (*p == '#')
- continue;
+ for (p = line; c_isspace (*p); p++)
+ ;
- items_read = sscanf (p, "%255s %d %d %lu %lu",
- host,
- &port,
- &include_subdomains,
- (unsigned long *) &created,
- (unsigned long *) &max_age);
+ if (*p == '#')
+ continue;
- if (items_read == 5)
- func (store, host, port, created, max_age, !!include_subdomains);
- }
-
- xfree (line);
- fclose (fp);
+ items_read = sscanf (p, "%255s %d %d %lu %lu",
+ host,
+ &port,
+ &include_subdomains,
+ (unsigned long *) &created,
+ (unsigned long *) &max_age);
- result = true;
+ if (items_read == 5)
+ func (store, host, port, created, max_age, !!include_subdomains);
}
+ xfree (line);
+ result = true;
+
return result;
}
static void
-hsts_store_dump (hsts_store_t store, const char *filename)
+hsts_store_dump (hsts_store_t store, FILE *fp)
{
- FILE *fp = NULL;
hash_table_iterator it;
- fp = fopen (filename, "w");
- if (fp)
+ /* Print preliminary comments. We don't care if any of these fail. */
+ fputs ("# HSTS 1.0 Known Hosts database for GNU Wget.\n", fp);
+ fputs ("# Edit at your own risk.\n", fp);
+ fputs ("# <hostname>[:<port>]\t<incl. subdomains>\t<created>\t<max-age>\n", fp);
+
+ /* Now cycle through the HSTS store in memory and dump the entries */
+ for (hash_table_iterate (store->table, &it); hash_table_iter_next (&it);)
{
- /* Print preliminary comments. We don't care if any of these fail. */
- fputs ("# HSTS 1.0 Known Hosts database for GNU Wget.\n", fp);
- fputs ("# Edit at your own risk.\n", fp);
- fputs ("# <hostname>[:<port>]\t<incl. subdomains>\t<created>\t<max-age>\n", fp);
+ struct hsts_kh *kh = (struct hsts_kh *) it.key;
+ struct hsts_kh_info *khi = (struct hsts_kh_info *) it.value;
- /* Now cycle through the HSTS store in memory and dump the entries */
- for (hash_table_iterate (store->table, &it); hash_table_iter_next (&it);)
+ if (fprintf (fp, "%s\t%d\t%d\t%lu\t%lu\n",
+ kh->host, kh->explicit_port, khi->include_subdomains,
+ khi->created, khi->max_age) < 0)
{
- struct hsts_kh *kh = (struct hsts_kh *) it.key;
- struct hsts_kh_info *khi = (struct hsts_kh_info *) it.value;
-
- if (fprintf (fp, "%s\t%d\t%d\t%lu\t%lu\n",
- kh->host, kh->explicit_port, khi->include_subdomains,
- khi->created, khi->max_age) < 0)
- {
- logprintf (LOG_ALWAYS, "Could not write the HSTS database correctly.\n");
- break;
- }
+ logprintf (LOG_ALWAYS, "Could not write the HSTS database correctly.\n");
+ break;
}
-
- fclose (fp);
}
}
@@ -472,6 +461,7 @@ hsts_store_open (const char *filename)
{
hsts_store_t store = NULL;
struct stat st;
+ FILE *fp = NULL;
store = xnew0 (struct hsts_store);
store->table = hash_table_new (0, hsts_hash_func, hsts_cmp_func);
@@ -482,13 +472,15 @@ hsts_store_open (const char *filename)
if (stat (filename, &st) == 0)
store->last_mtime = st.st_mtime;
- if (!hsts_read_database (store, filename, false))
+ fp = fopen (filename, "r");
+ if (!fp || !hsts_read_database (store, fp, false))
{
/* abort! */
hsts_store_close (store);
xfree (store);
- store = NULL;
}
+ if (fp)
+ fclose (fp);
}
return store;
@@ -498,18 +490,36 @@ void
hsts_store_save (hsts_store_t store, const char *filename)
{
struct stat st;
+ FILE *fp = NULL;
+ int fd = 0;
if (filename && hash_table_count (store->table) > 0)
{
- /* If the file has changed, merge the changes with our in-memory data
- before dumping them to the file.
- Otherwise we could potentially overwrite the data stored by other Wget processes.
- */
- if (store->last_mtime && stat (filename, &st) == 0 && st.st_mtime > store->last_mtime)
- hsts_read_database (store, filename, true);
-
- /* now dump to the file */
- hsts_store_dump (store, filename);
+ fp = fopen (filename, "a+");
+ if (fp)
+ {
+ /* Lock the file to avoid potential race conditions */
+ fd = fileno (fp);
+ flock (fd, LOCK_EX);
+
+ /* If the file has changed, merge the changes with our in-memory data
+ before dumping them to the file.
+ Otherwise we could potentially overwrite the data stored by other Wget processes.
+ */
+ if (store->last_mtime && stat (filename, &st) == 0 && st.st_mtime > store->last_mtime)
+ hsts_read_database (store, fp, true);
+
+ /* We've merged the latest changes so we can now truncate the file
+ and dump everything. */
+ fseek (fp, 0, SEEK_SET);
+ ftruncate (fd, 0);
+
+ /* now dump to the file */
+ hsts_store_dump (store, fp);
+
+ /* fclose is expected to unlock the file for us */
+ fclose (fp);
+ }
}
}
--
1.9.1
