Thread: [Vimprobable-users] [PATCH] Possible fix for crash on :echo

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Hi!

Following patch fixes the crash on :echo command. I don't know if I missed a
free, but it seems to work. Maybe someone else can inspect the issue too.

Daniel

Hi!

The patch introducs memory leaks because only the script function that is used
for the javascript and echo command frees the given arg->s, other function do
not free the argument. We should decide where to free the memory.

I know that the descussion about freeing was done in the paste
(http://www.vimprobable.org/mailman/archives/vimprobable-users/2011-January/000590.html),
but I can't find a solution for the problem there. I would prefer the way
Thoma Adam showed in mail
(http://www.vimprobable.org/mailman/archives/vimprobable-users/2011-January/000602.html).

Daniel



Hi!

Daniel Carl <dan...@gm...> wrote:
> I know that the descussion about freeing was done in the paste
> (http://www.vimprobable.org/mailman/archives/vimprobable-users/2011-January/000590.html),
> but I can't find a solution for the problem there. I would prefer the
> way Thoma Adam showed in mail
> (http://www.vimprobable.org/mailman/archives/vimprobable-users/2011-January/000602.html).

Please, no asymmetric freeing anymore! This lead to a lot of
seemingly unrelated issues in the past.

Hannes









Hi Hannes!
On Fri, Sep 16, 2011 at 03:25:22PM +0200, Hannes Schüller wrote:
> Please, no asymmetric freeing anymore! This lead to a lot of
> seemingly unrelated issues in the past.

What's ment by assymetric feeing, freeing memory where it isn't mallo()d? The
script function frees memory that it hasn't malloc()d. Then the script
function should not free arg->s, but it does. What was the reason to free
arg->s in the script function?

Daniel



Daniel Carl <dan...@gm...> wrote:
> On Fri, Sep 16, 2011 at 03:25:22PM +0200, Hannes Sch__ller wrote:
> > Please, no asymmetric freeing anymore! This lead to a lot of
> > seemingly unrelated issues in the past.
> 
> What's ment by assymetric feeing, freeing memory where it isn't
> mallo()d? The script function frees memory that it hasn't malloc()d.
> Then the script function should not free arg->s, but it does. What
> was the reason to free arg->s in the script function?

The reason, and feel free to correct me, was roughly "we cannot be
bothered to track down all calling instances and free the memory there,
so let's just do it in this central function". Which lead to all kinds
of nasty race conditions in several less used functions.

In your original patch, what I'd suggest to to allocate the memory,
call the function and then free the memory again afterwards. Nice and
readable. If we want to be sure, a "safefree" macro as suggested
certainly will not hurt.

Hannes





Hi!
On Fri, Sep 16, 2011 at 03:59:03PM +0200, Hannes Schüller wrote:
> The reason, and feel free to correct me, was roughly "we cannot be
> bothered to track down all calling instances and free the memory there,
> so let's just do it in this central function". Which lead to all kinds
> of nasty race conditions in several less used functions.
> 
> In your original patch, what I'd suggest to to allocate the memory,
> call the function and then free the memory again afterwards. Nice and
> readable. If we want to be sure, a "safefree" macro as suggested
> certainly will not hurt.

If I understood right, follwing patch should do that. It's a replacement for
previous patch. We need to remove the const Arg from sript function to write
the pointer ti NULL after freeing it.

Daniel

Hi,

On 15:00 Fri 16 Sep     , Daniel Carl wrote:
> Hi!
> 
> The patch introducs memory leaks because only the script function that is used
> for the javascript and echo command frees the given arg->s, other function do
> not free the argument. We should decide where to free the memory.

I'm (like Hannes) in favor of a free-by-caller convention for the
Arg-functions. Thomas tried to implement free-by-callee in those
patches.

It would be nice if we could finally get consistent behaviour regarding
mallocing and freeing. This means, that script() shouldn't free
anything and we should instead track down all the places where it's used
and call free() there. This should also remove the need for a SAVEFREE
macro.

Regards,
HP

Hi!
On Fri, Sep 16, 2011 at 05:18:25PM +0200, Hans-Peter Deifel wrote:
> Hi,
> 
> It would be nice if we could finally get consistent behaviour regarding
> mallocing and freeing. This means, that script() shouldn't free
> anything and we should instead track down all the places where it's used
> and call free() there. This should also remove the need for a SAVEFREE
> macro.

I like consistency, and the most parts use free-by-caller, so why not to apply
this paradigm to the script() function too.

Following patch removes freeing from script function and fixex also the crash
on :echo foo call.

I think the code should be refactered. In the last days i saw multiple places,
where memory is allocated via g_strdup() and not free()d. By the way we could
replace parts like following by g_strdup_printf().

    new = g_malloc(sizeof("http://") + len);
    strcpy(new, "http://");
    memcpy(&new[sizeof("http://") - 1], filename, len + 1);

Example cames from togge_proxy function where the new pointer isn't free()d.

Daniel

Sorry, I missed a g_free().

Following patch is a replacement for previous patches.

Daniel

Applied

Hi!

I looked through the code and found places where memory wasn't free()d. I'm a
C-beginner so maybe some of the free()s aren't needed.

Daniel

---
This patch is on top of Daniel's other memleak patches. There is another leak
in complete(), which I didn't fix because I didn't want to tinker with
complete()'s internals. I added a comment instead. complete() needs a general
cleanup, but I won't have time to do that anytime soon ;)

 main.c |   51 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/main.c b/main.c
index e4e79ac..9c7c22e 100644
--- a/main.c
+++ b/main.c
@@ -159,7 +159,7 @@ static SoupCookieJar *file_cookie_jar = NULL;
 static time_t cookie_timeout = 4800;
 static char *cookie_store;
 static void setup_cookies(void);
-static const char *get_cookies(SoupURI *soup_uri);
+static char *get_cookies(SoupURI *soup_uri);
 static void load_all_cookies(void);
 static void new_generic_request(SoupSession *soup_ses, SoupMessage *soup_msg, gpointer unused);
 static void update_cookie_jar(SoupCookieJar *jar, SoupCookie *old, SoupCookie *new);
@@ -426,11 +426,14 @@ set_widget_font_and_color(GtkWidget *widget, const char *font_str, const char *b
 void
 webview_hoverlink_cb(WebKitWebView *webview, char *title, char *link, gpointer data) {
     const char *uri = webkit_web_view_get_uri(webview);
+    char *markup;
 
     memset(rememberedURI, 0, 1024);
     if (link) {
-        gtk_label_set_markup(GTK_LABEL(status_url), g_markup_printf_escaped("<span font=\"%s\">Link: %s</span>", statusfont, link));
+        markup = g_markup_printf_escaped("<span font=\"%s\">Link: %s</span>", statusfont, link);
+        gtk_label_set_markup(GTK_LABEL(status_url), markup);
         strncpy(rememberedURI, link, 1024);
+        g_free(markup);
     } else
         update_url(uri);
 }
@@ -678,16 +681,18 @@ GtkWidget * fill_eventbox(const char * completion_line) {
     GtkBox    * row;
     GtkWidget *row_eventbox, *el;
     GdkColor  color;
-    char      * markup;
+    char      *markup, *markup_tmp;
 
     row = GTK_BOX(gtk_hbox_new(FALSE, 0));
     row_eventbox = gtk_event_box_new();
     gdk_color_parse(completionbgcolor[0], &color);
     gtk_widget_modify_bg(row_eventbox, GTK_STATE_NORMAL, &color);
     el = gtk_label_new(NULL);
+    markup_tmp = g_markup_escape_text(completion_line, strlen(completion_line));
     markup = g_strconcat("<span font=\"", completionfont[0], "\" color=\"", completioncolor[0], "\">",
-        g_markup_escape_text(completion_line, strlen(completion_line)), "</span>", NULL);
+        markup_tmp, "</span>", NULL);
     gtk_label_set_markup(GTK_LABEL(el), markup);
+    g_free(markup_tmp);
     g_free(markup);
     gtk_misc_set_alignment(GTK_MISC(el), 0, 0);
     gtk_box_pack_start(row, el, TRUE, TRUE, 2);
@@ -811,6 +816,7 @@ complete(const Arg *arg) {
                     if (n < MAX_LIST_SIZE && strstr(browsersettings[i].name, searchfor) != NULL) {
                         /* match */
                         fill_suggline(suggline, command, browsersettings[i].name);
+                        /* FIXME(HP): This memory is never freed */
                         suggurls[n] = (char *)malloc(sizeof(char) * 512 + 1);
                         strncpy(suggurls[n], suggline, 512);
                         suggestions[n] = suggurls[n];
@@ -837,6 +843,7 @@ complete(const Arg *arg) {
             elementpointer = elementlist;
             while (elementpointer != NULL) {
                 fill_suggline(suggline, command, elementpointer->element);
+                /* FIXME(HP): This memory is never freed */
                 suggurls[n] = (char *)malloc(sizeof(char) * 512 + 1);
                 strncpy(suggurls[n], suggline, 512);
                 suggestions[n] = suggurls[n];
@@ -1379,8 +1386,10 @@ script(const Arg *arg) {
     jsapi_evaluate_script(arg->s, &value, &message);
     if (message) {
         set_error(message);
+        g_free(message);
         return FALSE;
     }
+    g_free(message);
     if (arg->i != Silent && value) {
         a.i = arg->i;
         a.s = g_strdup(value);
@@ -1964,6 +1973,7 @@ void
 update_url(const char *uri) {
     gboolean ssl = g_str_has_prefix(uri, "https://");
     GdkColor color;
+    gchar *markup;
 #ifdef ENABLE_HISTORY_INDICATOR
     char before[] = " [";
     char after[] = "]";
@@ -1973,14 +1983,16 @@ update_url(const char *uri) {
     if (!back && !fwd)
         before[0] = after[0] = '\0';
 #endif
-    gtk_label_set_markup(GTK_LABEL(status_url), g_markup_printf_escaped(
+    markup = g_markup_printf_escaped(
 #ifdef ENABLE_HISTORY_INDICATOR
         "<span font=\"%s\">%s%s%s%s%s</span>", statusfont, uri,
         before, back ? "+" : "", fwd ? "-" : "", after
 #else
         "<span font=\"%s\">%s</span>", statusfont, uri
 #endif
-    ));
+    );
+    gtk_label_set_markup(GTK_LABEL(status_url), markup);
+    g_free(markup);
     gdk_color_parse(ssl ? sslbgcolor : statusbgcolor, &color);
     gtk_widget_modify_bg(eventbox, GTK_STATE_NORMAL, &color);
     gdk_color_parse(ssl ? sslcolor : statuscolor, &color);
@@ -2054,6 +2066,7 @@ update_state() {
 
     markup = g_markup_printf_escaped("<span font=\"%s\">%s</span>", statusfont, status->str);
     gtk_label_set_markup(GTK_LABEL(status_state), markup);
+    g_free(markup);
 
     g_string_free(status, TRUE);
 }
@@ -2154,6 +2167,7 @@ setup_settings() {
     file_url = g_strdup_printf("file://%s", filename);
     g_object_set(G_OBJECT(settings), "user-stylesheet-uri", file_url, NULL);
     g_free(file_url);
+    g_free(filename);
     g_object_set(G_OBJECT(settings), "user-agent", useragent, NULL);
     g_object_get(G_OBJECT(settings), "zoom-step", &zoomstep, NULL);
     webkit_web_view_set_settings(webview, settings);
@@ -2168,10 +2182,12 @@ setup_settings() {
                 strcpy(new, "http://");
                 memcpy(&new[sizeof("http://") - 1], filename, len + 1);
                 proxy_uri = soup_uri_new(new);
+                g_free(new);
             } else {
                 proxy_uri = soup_uri_new(filename);
             }
             g_object_set(session, "proxy-uri", proxy_uri, NULL);
+            soup_uri_free(proxy_uri);
         }
     }
 }
@@ -2256,22 +2272,24 @@ void
 new_generic_request(SoupSession *session, SoupMessage *soup_msg, gpointer unused) {
 	SoupMessageHeaders *soup_msg_h;
 	SoupURI *uri;
-	const char *cookie_str;
+	char *cookie_str;
 
 	soup_msg_h = soup_msg->request_headers;
 	soup_message_headers_remove(soup_msg_h, "Cookie");
 	uri = soup_message_get_uri(soup_msg);
-	if( (cookie_str = get_cookies(uri)) )
+	if( (cookie_str = get_cookies(uri)) ) {
 		soup_message_headers_append(soup_msg_h, "Cookie", cookie_str);
+		g_free(cookie_str);
+	}
 
 	g_signal_connect_after(G_OBJECT(soup_msg), "got-headers", G_CALLBACK(handle_cookie_request), NULL);
 
 	return;
 }
 
-const char *
+char *
 get_cookies(SoupURI *soup_uri) {
-	const char *cookie_str;
+	char *cookie_str;
 
 	cookie_str = soup_cookie_jar_get_cookies(file_cookie_jar, soup_uri, TRUE);
 
@@ -2281,12 +2299,11 @@ get_cookies(SoupURI *soup_uri) {
 void
 handle_cookie_request(SoupMessage *soup_msg, gpointer unused)
 {
-	GSList *resp_cookie = NULL;
+	GSList *resp_cookie = NULL, *cookie_list;
 	SoupCookie *cookie;
 
-	for(resp_cookie = soup_cookies_from_response(soup_msg);
-		resp_cookie;
-		resp_cookie = g_slist_next(resp_cookie))
+	cookie_list = soup_cookies_from_response(soup_msg);
+	for(resp_cookie = cookie_list; resp_cookie; resp_cookie = g_slist_next(resp_cookie))
 	{
 		SoupDate *soup_date;
 		cookie = soup_cookie_copy((SoupCookie *)resp_cookie->data);
@@ -2294,10 +2311,13 @@ handle_cookie_request(SoupMessage *soup_msg, gpointer unused)
 		if (cookie_timeout && cookie->expires == NULL) {
 			soup_date = soup_date_new_from_time_t(time(NULL) + cookie_timeout * 10);
 			soup_cookie_set_expires(cookie, soup_date);
+			soup_date_free(soup_date);
 		}
 		soup_cookie_jar_add_cookie(file_cookie_jar, cookie);
 	}
 
+	soup_cookies_free(cookie_list);
+
 	return;
 }
 
@@ -2320,10 +2340,12 @@ update_cookie_jar(SoupCookieJar *jar, SoupCookie *old, SoupCookie *new)
 void
 load_all_cookies(void)
 {
+	GSList *cookie_list;
 	file_cookie_jar = soup_cookie_jar_text_new(cookie_store, COOKIES_STORAGE_READONLY);
 
 	/* Put them back in the session store. */
 	GSList *cookies_from_file = soup_cookie_jar_all_cookies(file_cookie_jar);
+	cookie_list = cookies_from_file;
 
 	for (; cookies_from_file;
 	       cookies_from_file = cookies_from_file->next)
@@ -2332,6 +2354,7 @@ load_all_cookies(void)
 	}
 
 	soup_cookies_free(cookies_from_file);
+	g_slist_free(cookie_list);
 
 	return;
 }
-- 
1.7.3.4




Applied

Applied

Hi,

On 16:49 Sat 17 Sep     , Daniel Carl wrote:
> Hi!
> 
> I looked through the code and found places where memory wasn't free()d. I'm a
> C-beginner so maybe some of the free()s aren't needed.

Unless I've overseen something, your patches are good. Only a few
comments:

>  setup_settings() {
>      WebKitWebSettings *settings = (WebKitWebSettings*)webkit_web_settings_new();
>      SoupURI *proxy_uri;
> -    char *filename, *new;
> +    char *filename, *file_url, *new;
>      int  len;
>  
>      session = webkit_get_default_session();
> @@ -2140,8 +2151,9 @@ setup_settings() {
>      g_object_set(G_OBJECT(settings), "enable-java-applet", enableJava, NULL);
>      g_object_set(G_OBJECT(settings), "enable-page-cache", enablePagecache, NULL);
>      filename = g_strdup_printf(USER_STYLESHEET);

filename should be freed.

> -    filename = g_strdup_printf("file://%s", filename);
> -    g_object_set(G_OBJECT(settings), "user-stylesheet-uri", filename, NULL);
> +    file_url = g_strdup_printf("file://%s", filename);
> +    g_object_set(G_OBJECT(settings), "user-stylesheet-uri", file_url, NULL);
> +    g_free(file_url);
>      g_object_set(G_OBJECT(settings), "user-agent", useragent, NULL);
>      g_object_get(G_OBJECT(settings), "zoom-step", &zoomstep, NULL);
>      webkit_web_view_set_settings(webview, settings);

A few lines from here, 'new' is allocated and not freed

> @@ -2397,6 +2409,7 @@ main(int argc, char *argv[]) {
>  
>  	    feedback_str = g_strdup_printf("Config file '%s' doesn't exist", cfile);
>  	    give_feedback(feedback_str);
> +        g_free(feedback_str);

Indentation is somehow broken here. Not your fault, but still strange.


I ran vimprobable with your patches in valgrind and it seems that most
memory leaks have been plugged. I couldn't test everything, because
webkit's GC and valgrind don't seem to like each other, so the whole
thing crashed as soon as the GC kicked in.

However, I found two other leaks. I will create a patch for them tomorrow.

Regards,
HP

On Wed, Oct 05, 2011 at 01:31:42AM +0200, Hans-Peter Deifel wrote:
> Hi,
> Unless I've overseen something, your patches are good. Only a few
> comments:
> 
> >  setup_settings() {
> >      WebKitWebSettings *settings = (WebKitWebSettings*)webkit_web_settings_new();
> >      SoupURI *proxy_uri;
> > -    char *filename, *new;
> > +    char *filename, *file_url, *new;
> >      int  len;
> >  
> >      session = webkit_get_default_session();
> > @@ -2140,8 +2151,9 @@ setup_settings() {
> >      g_object_set(G_OBJECT(settings), "enable-java-applet", enableJava, NULL);
> >      g_object_set(G_OBJECT(settings), "enable-page-cache", enablePagecache, NULL);
> >      filename = g_strdup_printf(USER_STYLESHEET);
> 
> filename should be freed.

Yes it should, but if I free it vimprobable crashes and I have no idea why.
Does it work for you if you free filename?

> > -    filename = g_strdup_printf("file://%s", filename);
> > -    g_object_set(G_OBJECT(settings), "user-stylesheet-uri", filename, NULL);
> > +    file_url = g_strdup_printf("file://%s", filename);
> > +    g_object_set(G_OBJECT(settings), "user-stylesheet-uri", file_url, NULL);
> > +    g_free(file_url);
> >      g_object_set(G_OBJECT(settings), "user-agent", useragent, NULL);
> >      g_object_get(G_OBJECT(settings), "zoom-step", &zoomstep, NULL);
> >      webkit_web_view_set_settings(webview, settings);
> 
> A few lines from here, 'new' is allocated and not freed

This is right, can you do that with your patch for the other two leaks?

> > @@ -2397,6 +2409,7 @@ main(int argc, char *argv[]) {
> >  
> >  	    feedback_str = g_strdup_printf("Config file '%s' doesn't exist", cfile);
> >  	    give_feedback(feedback_str);
> > +        g_free(feedback_str);
> 
> Indentation is somehow broken here. Not your fault, but still strange.

We should fix code indentation with an extra patch, by the way the whitespace
errors could be fixed (trailing whitespace).

Daniel



Hi,

Sorry for the delay, I'm currently too busy to do any work for
vimprobable.

On 14:40 Sat 08 Oct     , Daniel Carl wrote:
> Can you do that with your patch for the other two leaks?

Yes, I will write the patch once I have a little more time, Probably on
Saturday, maybe earlier.




Thread: [Vimprobable-users] [PATCH] Possible fix for crash on :echo

Vimprobable is a lean web browser optimised for full keyboard control

vimprobable-users