Created attachment 162603 [details] [review]
fix leak in gdm-server.c

Created attachment 162604 [details] [review]
fix leaks in gdm-session-direct.c

Created attachment 162605 [details] [review]
fix leaks in gdm-session-worker-job.c

Created attachment 162606 [details] [review]
fix leaks in gdm-simple-slave.c

Created attachment 162607 [details] [review]
fix leaks in gdm-slave.c

Created attachment 162608 [details] [review]
fix leaks in gdm-slave-proxy.c

Created attachment 162609 [details] [review]
fix leaks in gdm-welcome-session.c

Created attachment 162722 [details] [review]
fix leaks in gdm-session-direct.c

This patch also fixes other leaks which are not included in the original one.

Created attachment 162724 [details] [review]
fix leaks in gdm-session-worker.c

Created attachment 162725 [details] [review]
fix leaks in gdm-product-display.c

Created attachment 162726 [details] [review]
fix leaks in gdm-product-slave.c

Review of attachment 162603 [details] [review]:

Please commit with changes suggested in the comments.

::: daemon/gdm-server.c
@@ +504,3 @@
                 str = g_strsplit (*l, "=", 2);
+                g_hash_table_insert (hash, g_strdup (str[0]), g_strdup (str[1]));
+                g_strfreev (str);

Probably a little more efficient to do:
g_hash_table_insert (hash, str[0], str[1]);
g_free (str);

@@ +977,3 @@
+        if (server->priv->chosen_hostname)
+                g_free (server->priv->chosen_hostname);
+

Looks right.  But the GDM style is to use an explicit NULL comparison and always use braces, like:
if (server->priv->command != NULL) {
    g_free (server->priv->command);
}

However, g_free already does a NULL check so these tests aren't needed.

Review of attachment 162605 [details] [review]:

Please commit with the suggested change.

::: daemon/gdm-session-worker-job.c
@@ +442,3 @@
+        if (session_worker_job->priv->command)
+                g_free (session_worker_job->priv->command);
+        if (session_worker_job->priv->server_address)

As mentioned above, the ifs aren't needed here.  Hmm, we probably shouldn't need a copy of priv->command since it is constant.  Might be best to make the command a #define value.

Review of attachment 162606 [details] [review]:

Please commit with that change.

::: daemon/gdm-simple-slave.c
@@ +360,1 @@
 

Interesting that we check for a NULL username here but not in the above two cases.  If the NULL is allowed we should probably "if" the above  calls to run_script.  If not then we should probably assert.  The g_free might be a little cleaner inside the if block here.

Review of attachment 162607 [details] [review]:

Looks good.

Review of attachment 162608 [details] [review]:

Looks good.

Review of attachment 162609 [details] [review]:

Please commit with that change.

::: daemon/gdm-welcome-session.c
@@ +832,3 @@
         gboolean   res;
+        char      *std_out = NULL;
+        char      *std_err = NULL;

Please initialize variables on separate lines after the variable initialization block.  Like this:

char *str1;
char *str2;

str1 = NULL;
str2 = NULL;

And in this case do it immediately before they are used.  Right above error = NULL;

Review of attachment 162722 [details] [review]:

Commit with changes.

::: daemon/gdm-session-direct.c
@@ +517,3 @@
                                          G_KEY_FILE_NONE,
                                          &error);
+        g_free (full_path);

It appears we aren't using this value.  So, better to remove it and pass in NULL.

@@ +721,3 @@
         }
 
         return get_fallback_session_name ();

Another option would be make get_fallback_session_name() not have to recompute and allocate a new string each time.  But I guess this is ok.

@@ +1997,3 @@
         }
 
         return get_default_session_name (session);

Same comment as above.

@@ +2238,3 @@
                                          G_KEY_FILE_NONE,
                                          &error);
+        g_free (full_path);

If we aren't using it please remove it.

Review of attachment 162724 [details] [review]:

Commit after fixing comments.

::: daemon/gdm-session-worker.c
@@ +1727,3 @@
         if (ret) {
                 g_debug ("GdmSessionWorker: state ACCREDITED");
                 ret = TRUE;

If you free them in out: you need to set them to NULL earlier.  There is a path to reach out: before they are initialized.

@@ +2997,3 @@
+                worker->priv->hostname = NULL;
+        }
+

None of these if statements and NULL assignments are needed.  Finalize is run last.  My guess is that we once had a dispose() where they would be needed because dispose can run multiple times.

Please remove the ifs and = NULL for all of these.

Review of attachment 162725 [details] [review]:

Commit after comment.

::: daemon/gdm-product-display.c
@@ +249,3 @@
+                product_display->priv->relay_address = NULL;
+        }
+

just the g_free will be fine here.  No if or = NULL.

Review of attachment 162726 [details] [review]:

Looks good.

::: daemon/gdm-product-slave.c
@@ +1265,2 @@
         G_OBJECT_CLASS (gdm_product_slave_parent_class)->finalize (object);
 }

Was this already fixed in master?

Since these are small enough an all related please commit these as once patch.  Thanks.

Any chance someone can update these patches by tomorrow?  I'd like to get them into a new release.

I'm working on it

(In reply to comment #19)
> Review of attachment 162724 [details] [review]:
> 
> Commit after fixing comments.
> 
> ::: daemon/gdm-session-worker.c
> @@ +1727,3 @@
>          if (ret) {
>                  g_debug ("GdmSessionWorker: state ACCREDITED");
>                  ret = TRUE;
> 
> If you free them in out: you need to set them to NULL earlier.  There is a path
> to reach out: before they are initialized.
> 

I don't understand what do you mean.

Created attachment 163806 [details] [review]
0001-Fixes-several-memory-leaks.-By-Wang-Xin.patch

Here it is. Patch all-in-one, but please review it too.

Note that it lacks the #define thing.

Review of attachment 163806 [details] [review]:

Looks pretty good.  I'm going to commit this with the changes I've mentioned.  Thanks.

::: daemon/gdm-server.c
@@ +512,3 @@
                 str = g_strsplit (*l, "=", 2);
                 g_hash_table_insert (hash, str[0], str[1]);
+                g_free (server->priv->chosen_hostname);

Should be g_free (str);

::: daemon/gdm-session-worker.c
@@ +1727,3 @@
         if (ret) {
                 g_debug ("GdmSessionWorker: state ACCREDITED");
                 ret = TRUE;

The first call to "goto out" is before where home and shell are initialized to NULL.  We need to move the initialization earlier.

@@ +2981,3 @@
+        g_free (worker->priv->server_address);
+        g_strfreev (worker->priv->arguments);
+        g_hash_table_destroy (worker->priv->environment);

While the g_free and g_strfreev calls check for and allow NULL values, g_hash_table_destroy does not.  So, we need to check for NULL before calling it.  Now, we know that will not happen in this class but we want to be robust against changes in other parts of the code.

::: daemon/gdm-welcome-session.c
@@ +1465,3 @@
         gdm_welcome_session_stop (welcome_session);
 
+        ck_connector_unref (welcome_session->priv->ckc);

Since there are cases where ckc can be NULL and ck_connector_unref doesn't allow NULLs we need to check it.

Fixed in 2-30 and master.  Thanks!