Hi Dagobert, hi Ander,
I could reproduce and fix the problem on a 32bit Debian wheezy VM.
@Dagobert Please check if the attached patch works on Solaris as well
@Ander Please review that I did not get anything wrong
Tim
On Tuesday 17 November 2015 21:21:00 Dagobert Michelsen wrote:
> Hi Tim,
>
> Am 17.11.2015 um 21:17 schrieb Tim Rühsen <[email protected]>:
> > Am Dienstag, 17. November 2015, 15:09:05 schrieb Dagobert Michelsen:
> >>> RUNNING TEST test_hsts_url_rewrite_superdomain...
> >>> A new entry should've been created
> >>> Tests run: 15
> >>> FAIL unit-tests (exit status: 1)
> >>
> >> So which test is failing? test_hsts_url_rewrite_superdomain ?
> >> There is no *hsts* in tests/
> >
> > Could you please change L689 of src/hsts.c from
> >
> > created = hsts_store_entry (s, SCHEME_HTTPS, "www.foo.com", 443,
> > time(NULL)
> >
> > + 1234, true);
> >
> > to
> >
> > created = hsts_store_entry (s, SCHEME_HTTPS, "www.foo.com", 443, 1234,
> >
> > true);
> >
> > and give it a try ?
>
> Sorry, still fails.
>
>
> Best regards
>
> — Dago
>
> --
> "You don't become great by trying to be great, you become great by wanting
> to do something, and then doing it so hard that you become great in the
> process." - xkcd #896
>From 41ef7db760f9726c0fd7d17baa2c1049e35905cd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?= <[email protected]>
Date: Wed, 18 Nov 2015 10:58:56 +0100
Subject: [PATCH] Fix HSTS memory issue + test code issue
* src/hsts.c (hsts_find_entry): Fix freeing memory
(hsts_remove_entry): Remove freeing host member
(hsts_match): Free host member here
(hsts_store_entry): Free host member here
(test_url_rewrite): Fix 'created' value
(test_hsts_read_database): Fix 'created' value
Reported-by: Dagobert Michelsen <[email protected]>
---
src/hsts.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/src/hsts.c b/src/hsts.c
index b0989c7..0918169 100644
--- a/src/hsts.c
+++ b/src/hsts.c
@@ -148,13 +148,14 @@ hsts_find_entry (hsts_store_t store,
end:
/* restore pointer or we'll get a SEGV */
k->host = org_ptr;
- xfree (k->host);
/* copy parameters to previous frame */
if (match_type)
*match_type = match;
if (kh)
memcpy (kh, k, sizeof (struct hsts_kh));
+ else
+ xfree (k->host);
xfree (k);
return khi;
@@ -236,8 +237,7 @@ hsts_new_entry (hsts_store_t store,
static void
hsts_remove_entry (hsts_store_t store, struct hsts_kh *kh)
{
- if (hash_table_remove (store->table, kh))
- xfree (kh->host);
+ hash_table_remove (store->table, kh);
}
static bool
@@ -377,7 +377,8 @@ hsts_match (hsts_store_t store, struct url *u)
}
}
- xfree(kh);
+ xfree (kh->host);
+ xfree (kh);
return url_changed;
}
@@ -453,7 +454,8 @@ hsts_store_entry (hsts_store_t store,
/* we ignore new entries with max_age == 0 */
}
- xfree(kh);
+ xfree (kh->host);
+ xfree (kh);
return result;
}
@@ -613,7 +615,7 @@ test_url_rewrite (hsts_store_t s, const char *url, int port, bool rewrite)
if (rewrite)
{
if (port == 80)
- mu_assert("URL: port should've been rewritten to 443", u.port == 443);
+ mu_assert("URL: port should've been rewritten to 443", u.port == 443);
else
mu_assert("URL: port should've been left intact", u.port == port);
mu_assert("URL: scheme should've been rewritten to HTTPS", u.scheme == SCHEME_HTTPS);
@@ -686,7 +688,7 @@ test_hsts_url_rewrite_superdomain (void)
s = open_hsts_test_store ();
mu_assert("Could not open the HSTS store", s != NULL);
- created = hsts_store_entry (s, SCHEME_HTTPS, "www.foo.com", 443, time(NULL) + 1234, true);
+ created = hsts_store_entry (s, SCHEME_HTTPS, "www.foo.com", 443, 1234, true);
mu_assert("A new entry should've been created", created == true);
TEST_URL_RW (s, "www.foo.com", 80);
@@ -707,7 +709,7 @@ test_hsts_url_rewrite_congruent (void)
s = open_hsts_test_store ();
mu_assert("Could not open the HSTS store", s != NULL);
- created = hsts_store_entry (s, SCHEME_HTTPS, "foo.com", 443, time(NULL) + 1234, false);
+ created = hsts_store_entry (s, SCHEME_HTTPS, "foo.com", 443, 1234, false);
mu_assert("A new entry should've been created", created == true);
TEST_URL_RW (s, "foo.com", 80);
@@ -726,6 +728,7 @@ test_hsts_read_database (void)
char *home = home_dir();
char *file = NULL;
FILE *fp = NULL;
+ time_t created = time(NULL) - 10;
if (home)
{
@@ -734,9 +737,9 @@ test_hsts_read_database (void)
if (fp)
{
fputs ("# dummy comment\n", fp);
- fputs ("foo.example.com\t0\t1\t1434224817\t123123123\n", fp);
- fputs ("bar.example.com\t0\t0\t1434224817\t456456456\n", fp);
- fputs ("test.example.com\t8080\t0\t1434224817\t789789789\n", fp);
+ fprintf (fp, "foo.example.com\t0\t1\t%ld\t123\n",(long) created);
+ fprintf (fp, "bar.example.com\t0\t0\t%ld\t456\n", (long) created);
+ fprintf (fp, "test.example.com\t8080\t0\t%ld\t789\n", (long) created);
fclose (fp);
table = hsts_store_open (file);
--
1.7.10.4
