From 8f6f117372e4f1092196bfdddbaeb293039f727b Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Fri, 23 Jun 2017 18:53:33 -0400 Subject: [PATCH] Fix memory management with 'GLib.HashTable' (fix #211) The specific issue (#211) was caused by a leaky hash table and this is a general situation if the 'full' constructor is not being used. Use explicit 'full' constructor wherever possible and in particular: - reset and free 'GLib.Value' in context - fix copied hash table when setting the 'charset' parameter --- src/valum/valum-basic.vala | 6 ++++-- src/valum/valum-content-negotiation.vala | 6 ++++-- src/valum/valum-context.vala | 10 ++++++++-- src/valum/valum-route.vala | 2 +- src/valum/valum-router.vala | 9 ++++++--- src/valum/valum-static.vala | 2 +- src/vsgi/vsgi-response.vala | 18 ++++++++++++++++-- 7 files changed, 40 insertions(+), 13 deletions(-) diff --git a/src/valum/valum-basic.vala b/src/valum/valum-basic.vala index 8be0ea7c9..faeb34129 100644 --- a/src/valum/valum-basic.vala +++ b/src/valum/valum-basic.vala @@ -158,8 +158,10 @@ namespace Valum { // basic handling default: - var @params = new HashTable ((HashFunc) Soup.str_case_hash, - (EqualFunc) Soup.str_case_equal); + var @params = new HashTable.full ((HashFunc) Soup.str_case_hash, + (EqualFunc) Soup.str_case_equal, + free, + free); @params["charset"] = "utf-8"; res.headers.set_content_type ("text/plain", @params); try { diff --git a/src/valum/valum-content-negotiation.vala b/src/valum/valum-content-negotiation.vala index 0661d21ef..4cd943e1d 100644 --- a/src/valum/valum-content-negotiation.vala +++ b/src/valum/valum-content-negotiation.vala @@ -181,8 +181,10 @@ namespace Valum.ContentNegotiation { HashTable @params; var content_type = res.headers.get_content_type (out @params) ?? "application/octet-stream"; if (@params == null) { - @params = new HashTable ((GLib.HashFunc) Soup.str_case_hash, - (GLib.EqualFunc) Soup.str_case_equal); + @params = new HashTable.full ((GLib.HashFunc) Soup.str_case_hash, + (GLib.EqualFunc) Soup.str_case_equal, + free, + free); } @params["charset"] = charset; res.headers.set_content_type (content_type, @params); diff --git a/src/valum/valum-context.vala b/src/valum/valum-context.vala index 6f48c2bc0..39698aff1 100644 --- a/src/valum/valum-context.vala +++ b/src/valum/valum-context.vala @@ -30,10 +30,16 @@ public delegate void Valum.ContextForeachFunc (string key, Value @value, uint de [Version (since = "0.3")] public class Valum.Context : Object { + private static void _reset_and_free_value (void* @val) + { + ((Value?) @val).reset (); + free (@val); + } + /** * Internal mapping of states. */ - private HashTable states = new HashTable (str_hash, str_equal); + private HashTable states = new HashTable.full (str_hash, str_equal, free, _reset_and_free_value); /** * Parent's context from which missing keys are resolved. @@ -93,7 +99,7 @@ public class Valum.Context : Object { */ [Version (since = "0.3")] public new void @set (string key, owned Value @value) { - states.@set (key, (owned) @value); + states.insert (key, (owned) @value); } /** diff --git a/src/valum/valum-route.vala b/src/valum/valum-route.vala index 0217c546a..e2976610d 100644 --- a/src/valum/valum-route.vala +++ b/src/valum/valum-route.vala @@ -87,7 +87,7 @@ namespace Valum { */ [Version (since = "0.3")] public string to_url_from_valist (va_list list) { - var hash = new HashTable (str_hash, str_equal); + var hash = new HashTable.full (str_hash, str_equal, free, free); // potential compiler bug here: SEGFAULT if 'var' is used instead of 'unowned string' for (unowned string key = list.arg (), val = list.arg (); key != null && val != null; diff --git a/src/valum/valum-router.vala b/src/valum/valum-router.vala index 27f780b6f..364b4563c 100644 --- a/src/valum/valum-router.vala +++ b/src/valum/valum-router.vala @@ -35,7 +35,7 @@ namespace Valum { [Version (since = "0.3", experimental = true)] public Sequence routes = new Sequence (); - private HashTable types = new HashTable (str_hash, str_equal); + private HashTable types = new HashTable.full (str_hash, str_equal, free, null); private Queue scopes = new Queue (); [Version (since = "0.3")] @@ -62,7 +62,7 @@ namespace Valum { */ [Version (since = "0.3", experimental = true)] public void register_type (string name, Regex pattern) { - types[name] = pattern; + types.insert (name, pattern); } [Version (since = "0.3")] @@ -260,7 +260,10 @@ namespace Valum { route (new MatcherRoute (method | Method.PROVIDED, (owned) matcher, (owned) cb)); } - private HashTable _named_routes = new HashTable (str_hash, str_equal); + private HashTable _named_routes = new HashTable.full (str_hash, + str_equal, + free, + null); /* owned by 'routes' */ /** * Append a {@link Route} object on the routing sequence. diff --git a/src/valum/valum-static.vala b/src/valum/valum-static.vala index fab75308f..7ce6f8cad 100644 --- a/src/valum/valum-static.vala +++ b/src/valum/valum-static.vala @@ -198,7 +198,7 @@ namespace Valum.Static { ServeFlags serve_flags = ServeFlags.NONE, owned HandlerCallback forward = Valum.forward) { // cache for already computed 'ETag' values - var etag_cache = new HashTable (str_hash, str_equal); + var etag_cache = new HashTable .full (str_hash, str_equal, free, free); return (req, res, next, ctx) => { var path = "%s%s".printf (prefix, ctx["path"].get_string ()); diff --git a/src/vsgi/vsgi-response.vala b/src/vsgi/vsgi-response.vala index 6378a7af0..2dc742017 100644 --- a/src/vsgi/vsgi-response.vala +++ b/src/vsgi/vsgi-response.vala @@ -313,6 +313,15 @@ namespace VSGI { _body = new TeeOutputStream (_body ?? request.connection.output_stream, tee_stream); } + private inline HashTable _copy_params_hash (HashTable @params) { + var params_copy = new HashTable.full ((HashFunc) Soup.str_case_hash, + (EqualFunc) Soup.str_case_equal, + free, + free); + @params.foreach ((k, v) => params_copy.insert (k, v)); + return params_copy; + } + private inline void _mark_content_as_utf8 () { if (head_written) { return; @@ -321,9 +330,14 @@ namespace VSGI { var content_type = headers.get_content_type (out @params); if (content_type == null) { headers.set_content_type ("application/octet-stream", Soup.header_parse_param_list ("charset=UTF-8")); + } else if (@params == null) { + headers.set_content_type (content_type, Soup.header_parse_param_list ("charset=UTF-8")); } else if (@params["charset"] == null) { - @params["charset"] = "UTF-8"; - headers.set_content_type (content_type, @params); + var @params_copy = _copy_params_hash (@params); + params_copy["charset"] = "UTF-8"; + headers.set_content_type (content_type, params_copy); + } else { + // charset is already set, so we assume a converter is applied appropriately } }