Hi Tim, On 24/05/16 14:49, Tim Ruehsen wrote: > > I meant e.g. in hsts_match(), you eventually remove an expired entry, but you > do not set store->changed. Thus on exit, the database wouldn't be updated. > I think it is good to keep the database as small as possible - following > invocations of wget have less to read. >
True. I could not reproduce the behavior you describe, but I see the 'changed' field is not being updated in hsts_match(). I knew I was forgetting something! Please find a new patch attached, and tell me if there's still something left. > > Sounds good, thanks for explaining. > My thought was to update the DB every time we get a STS header, since max_age > is relative to the current time. But your approach is much more elegant in > respect to the number of writes (disk&cpu usage). Actually, I blindly followed the RFC. But yes, I guess that was one of the reasons the RFC authors did it that way. > > Tim > Best regards, - AJ
>From 931db7e9fd0a55225e3e2d75fb3fdf328cb565bc Mon Sep 17 00:00:00 2001 From: Ander Juaristi <[email protected]> Date: Tue, 24 May 2016 11:14:38 +0200 Subject: [PATCH] Correct HSTS debug message * src/main.c (save_hsts): save the in-memory HSTS database to a file only if something changed. * src/hsts.c (struct hsts_store): new field 'changed'. (hsts_match): update field 'changed' accordingly. (hsts_store_entry): update field 'changed' accordingly. (hsts_store_has_changed): new function. * src/hsts.h (hsts_store_has_changed): new function. --- src/hsts.c | 23 +++++++++++++++++++++-- src/hsts.h | 1 + src/main.c | 8 +++++--- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/hsts.c b/src/hsts.c index d5e0bee..be824ea 100644 --- a/src/hsts.c +++ b/src/hsts.c @@ -53,6 +53,7 @@ as that of the covered work. */ struct hsts_store { struct hash_table *table; time_t last_mtime; + bool changed; }; struct hsts_kh { @@ -370,10 +371,14 @@ hsts_match (hsts_store_t store, struct url *u) if (u->port == 80) u->port = 443; url_changed = true; + store->changed = true; } } else - hsts_remove_entry (store, kh); + { + hsts_remove_entry (store, kh); + store->changed = true; + } } xfree (kh->host); } @@ -423,7 +428,10 @@ hsts_store_entry (hsts_store_t store, if (entry && match == CONGRUENT_MATCH) { if (max_age == 0) - hsts_remove_entry (store, kh); + { + hsts_remove_entry (store, kh); + store->changed = true; + } else if (max_age > 0) { if (entry->max_age != max_age || @@ -436,6 +444,8 @@ hsts_store_entry (hsts_store_t store, entry->created = t; entry->max_age = max_age; entry->include_subdomains = include_subdomains; + + store->changed = true; } } /* we ignore negative max_ages */ @@ -450,6 +460,8 @@ hsts_store_entry (hsts_store_t store, happen we got a non-existent entry with max_age == 0. */ result = hsts_add_entry (store, host, port, max_age, include_subdomains); + if (result) + store->changed = true; } /* we ignore new entries with max_age == 0 */ xfree (kh->host); @@ -470,6 +482,7 @@ hsts_store_open (const char *filename) store = xnew0 (struct hsts_store); store->table = hash_table_new (0, hsts_hash_func, hsts_cmp_func); store->last_mtime = 0; + store->changed = false; if (file_exists_p (filename)) { @@ -531,6 +544,12 @@ hsts_store_save (hsts_store_t store, const char *filename) } } +bool +hsts_store_has_changed (hsts_store_t store) +{ + return (store ? store->changed : false); +} + void hsts_store_close (hsts_store_t store) { diff --git a/src/hsts.h b/src/hsts.h index 9a7043d..6e5bbc8 100644 --- a/src/hsts.h +++ b/src/hsts.h @@ -43,6 +43,7 @@ hsts_store_t hsts_store_open (const char *); void hsts_store_save (hsts_store_t, const char *); void hsts_store_close (hsts_store_t); +bool hsts_store_has_changed (hsts_store_t); bool hsts_store_entry (hsts_store_t, enum url_scheme, const char *, int, diff --git a/src/main.c b/src/main.c index ed050a5..e7d5c66 100644 --- a/src/main.c +++ b/src/main.c @@ -204,10 +204,12 @@ save_hsts (void) { char *filename = get_hsts_database (); - if (filename) - DEBUGP (("Saving HSTS entries to %s\n", filename)); + if (filename && hsts_store_has_changed (hsts_store)) + { + DEBUGP (("Saving HSTS entries to %s\n", filename)); + hsts_store_save (hsts_store, filename); + } - hsts_store_save (hsts_store, filename); hsts_store_close (hsts_store); xfree (filename); -- 2.1.4
