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
valum-framework · Jun 23, 2017 · 8f6f117 · 8f6f117
1 parent 70bbdf4
commit 8f6f117
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 13 deletions.
diff --git 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<string, string> ((HashFunc<string>) Soup.str_case_hash,
-						                                             (EqualFunc<string>) Soup.str_case_equal);
+						var @params = new HashTable<string, string>.full ((HashFunc<string>) Soup.str_case_hash,
+						                                                  (EqualFunc<string>) 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
@@ -181,8 +181,10 @@ namespace Valum.ContentNegotiation {
 			HashTable<string, string> @params;
 			var content_type   = res.headers.get_content_type (out @params) ?? "application/octet-stream";
 			if (@params == null) {
-				@params = new HashTable<string, string> ((GLib.HashFunc<string>) Soup.str_case_hash,
-				                                         (GLib.EqualFunc<string>) Soup.str_case_equal);
+				@params = new HashTable<string, string>.full ((GLib.HashFunc<string>) Soup.str_case_hash,
+				                                              (GLib.EqualFunc<string>) 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
@@ -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<string, Value?> states = new HashTable<string, Value?> (str_hash, str_equal);
+	private HashTable<string, Value?> states = new HashTable<string, Value?>.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
@@ -87,7 +87,7 @@ namespace Valum {
 		 */
 		[Version (since = "0.3")]
 		public string to_url_from_valist (va_list list) {
-			var hash = new HashTable<string, string> (str_hash, str_equal);
+			var hash = new HashTable<string, string>.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<string> (), val = list.arg<string> ();
 				key != null && val != null;

diff --git 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<Route> routes = new Sequence<Route> ();
 
-		private HashTable<string, Regex> types  = new HashTable<string, Regex> (str_hash, str_equal);
+		private HashTable<string, Regex> types  = new HashTable<string, Regex>.full (str_hash, str_equal, free, null);
 		private Queue<string>            scopes = new Queue<string> ();
 
 		[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<string, Route> _named_routes = new HashTable<string, Route> (str_hash, str_equal);
+		private HashTable<string, Route> _named_routes = new HashTable<string, Route>.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
@@ -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 <string, string> (str_hash, str_equal);
+		var etag_cache = new HashTable <string, string>.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
@@ -313,6 +313,15 @@ namespace VSGI {
 			_body = new TeeOutputStream (_body ?? request.connection.output_stream, tee_stream);
 		}
 
+		private inline HashTable<string, string> _copy_params_hash (HashTable<string, string> @params) {
+			var params_copy = new HashTable<string, string>.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
 			}
 		}