Re: [gdm-list] Jon - patch for gdm-gobject branch
- From: Brian Cameron <Brian Cameron Sun COM>
- To: gdm-list gnome org
- Subject: Re: [gdm-list] Jon - patch for gdm-gobject branch
- Date: Wed, 10 Oct 2007 19:44:39 -0500
Jon:
Since you are keeping careful track of who contributed various bits
of code to GDM, you should probably add Gary Winiger
(Gary Winiger sun com) to the authors list since he wrote the Sun
Audit stuff.
Looking pretty good. A few issues:
* Seems like it leaks the result from each g_object_get.
* The #if 0 around the utmp stuff should be removed from this patch
* The name in the D-Bus introspection xml should be x11_display_name
* We should probably rename console in the D-Bus introspection xml to
be display_device
I think the attached patch addresses the issues we discussed above via
private email. I am now freeing the values from g_object_get. I also
fixed the code so that x11_display_name and display_device are in the
introspection xml. I also changed the places where we were using
"console_name" in the source code so it now uses "display_device" to
be consistent.
Can this go upstream? As discussed, the audit and other logic needs
the display device information to be passed along to the session open.
So this fixes things so we pass this information along.
[ regarding supporting SDTLOGIN on Solaris ]
This is a private interface that causes the Xserver to drop to user
permissions so it doesn't run as root, on Solaris. This is a required
display manager feature for Sun to be able to ship the display manager.
Using __sun for this seems just wrong.
Would adding a configure argument like --with-sdtlogin to enable this
feature be acceptable? This way people on Soalris who want to build
the code with this useful security feature can do so without having to
figure out how to patch the code, etc.
If this would be acceptable, let me know and I'll clean up the
SDTLOGIN patch for you.
What if someone is building on
Solaris and doesn't have an Xorg that supports SDTLOGIN? I'll think
some more about this but if SDTLOGIN in Xorg is a Sun specific patch
then the SDTLOGIN support for GDM might be best applied as a patch to
your GDM packages too.
I did mention that Alan Coopersmith from Sun's Xserver team is planning
to get this interface upstream into the Xorg code so other platforms can
also have this security feature. However, I know Alan has a lot of work
on his plate, and it will not likely be done in the short-term. When he
does this, it probably will be a slightly different interface since
the "SDTLOGIN" naming probably doesn't make sense for a generic
interface since the "SD" stands for Sun Desktop".
If it is not acceptable for this code to live upstream, then Sun could
apply it as a patch. However, the disadvantage here is that people who
build the GDM code on their own will probably not know to patch the
code to use this interface.
Brian
Index: common/gdm-marshal.list
===================================================================
--- common/gdm-marshal.list (revision 5357)
+++ common/gdm-marshal.list (working copy)
@@ -1,3 +1,4 @@
+VOID:STRING,STRING,STRING,STRING,STRING
VOID:STRING,STRING,STRING,STRING
VOID:STRING,STRING,STRING
VOID:STRING,STRING
Index: daemon/gdm-product-slave.c
===================================================================
--- daemon/gdm-product-slave.c (revision 5357)
+++ daemon/gdm-product-slave.c (working copy)
@@ -944,13 +944,19 @@
static gboolean
reset_session (GdmProductSlave *slave)
{
+ gchar *display_name;
gboolean res;
GError *error;
+ g_object_get (slave,
+ "display-name", &display_name,
+ NULL);
+
gdm_session_close (slave->priv->session);
res = gdm_session_open (slave->priv->session,
"gdm",
"",
+ display_name,
"/dev/console",
&error);
if (! res) {
@@ -958,6 +964,8 @@
g_error_free (error);
}
+ g_free (display_name);
+
return res;
}
@@ -981,21 +989,29 @@
gpointer data)
{
GdmProductSlave *slave = GDM_PRODUCT_SLAVE (data);
+ gchar *display_name;
gboolean res;
GError *error;
g_debug ("Relay open: opening session");
+ g_object_get (slave,
+ "display-name", &display_name,
+ NULL);
+
error = NULL;
res = gdm_session_open (slave->priv->session,
"gdm",
"",
+ display_name,
"/dev/console",
&error);
if (! res) {
g_warning ("Unable to open session: %s", error->message);
g_error_free (error);
}
+
+ g_free (display_name);
}
static void
Index: daemon/test-session.c
===================================================================
--- daemon/test-session.c (revision 5357)
+++ daemon/test-session.c (working copy)
@@ -213,6 +213,7 @@
gdm_session_open (session,
"gdm",
"",
+ ":0",
ttyname (STDIN_FILENO),
NULL);
Index: daemon/gdm-session.c
===================================================================
--- daemon/gdm-session.c (revision 5357)
+++ daemon/gdm-session.c (working copy)
@@ -80,7 +80,8 @@
char *service_name;
char *username;
char *hostname;
- char *console_name;
+ char *x11_display_name;
+ char *display_device;
DBusMessage *message_pending_reply;
@@ -206,20 +207,20 @@
/* FIXME: I have no idea what to do for ut_id.
*/
strncpy (session_record.ut_id,
- session->priv->console_name +
- strlen (session->priv->console_name) -
+ session->priv->display_device +
+ strlen (session->priv->display_device) -
sizeof (session_record.ut_id),
sizeof (session_record.ut_id));
g_debug ("using id %.*s", sizeof (session_record.ut_id), session_record.ut_id);
- if (g_str_has_prefix (session->priv->console_name, "/dev/")) {
+ if (g_str_has_prefix (session->priv->display_device, "/dev/")) {
strncpy (session_record.ut_line,
- session->priv->console_name + strlen ("/dev/"),
+ session->priv->display_device + strlen ("/dev/"),
sizeof (session_record.ut_line));
- } else if (g_str_has_prefix (session->priv->console_name, ":")) {
+ } else if (g_str_has_prefix (session->priv->display_device, ":")) {
strncpy (session_record.ut_line,
- session->priv->console_name,
+ session->priv->display_device,
sizeof (session_record.ut_line));
}
@@ -232,15 +233,15 @@
*/
hostname = NULL;
if ((session->priv->hostname != NULL) &&
- g_str_has_prefix (session->priv->console_name, ":"))
+ g_str_has_prefix (session->priv->display_device, ":"))
hostname = g_strdup_printf ("%s%s", session->priv->hostname,
- session->priv->console_name);
+ session->priv->display_device);
else if ((session->priv->hostname != NULL) &&
- !strstr (session->priv->console_name, ":"))
+ !strstr (session->priv->display_device, ":"))
hostname = g_strdup (session->priv->hostname);
- else if (!g_str_has_prefix (session->priv->console_name, ":") &&
- strstr (session->priv->console_name, ":"))
- hostname = g_strdup (session->priv->console_name);
+ else if (!g_str_has_prefix (session->priv->display_device, ":") &&
+ strstr (session->priv->display_device, ":"))
+ hostname = g_strdup (session->priv->display_device);
if (hostname) {
g_debug ("using hostname %.*s",
@@ -970,13 +971,15 @@
" </method>\n"
" <signal name=\"BeginVerification\">\n"
" <arg name=\"service_name\" type=\"s\"/>\n"
+ " <arg name=\"x11_display_name\" type=\"s\"/>\n"
+ " <arg name=\"display_device\" type=\"s\"/>\n"
" <arg name=\"hostname\" type=\"s\"/>\n"
- " <arg name=\"console\" type=\"s\"/>\n"
" </signal>\n"
" <signal name=\"BeginVerificationForUser\">\n"
" <arg name=\"service_name\" type=\"s\"/>\n"
+ " <arg name=\"x11_display_name\" type=\"s\"/>\n"
+ " <arg name=\"display_device\" type=\"s\"/>\n"
" <arg name=\"hostname\" type=\"s\"/>\n"
- " <arg name=\"console\" type=\"s\"/>\n"
" <arg name=\"username\" type=\"s\"/>\n"
" </signal>\n"
" <signal name=\"StartProgram\">\n"
@@ -1354,14 +1357,16 @@
gdm_session_open (GdmSession *session,
const char *service_name,
const char *hostname,
- const char *console_name,
+ const char *x11_display_name,
+ const char *display_device,
GError **error)
{
gboolean res;
g_return_val_if_fail (session != NULL, FALSE);
g_return_val_if_fail (service_name != NULL, FALSE);
- g_return_val_if_fail (console_name != NULL, FALSE);
+ g_return_val_if_fail (x11_display_name != NULL, FALSE);
+ g_return_val_if_fail (display_device != NULL, FALSE);
g_return_val_if_fail (hostname != NULL, FALSE);
g_debug ("Openning session");
@@ -1370,7 +1375,8 @@
session->priv->service_name = g_strdup (service_name);
session->priv->hostname = g_strdup (hostname);
- session->priv->console_name = g_strdup (console_name);
+ session->priv->x11_display_name = g_strdup (x11_display_name);
+ session->priv->display_device = g_strdup (display_device);
return res;
}
@@ -1389,8 +1395,9 @@
dbus_message_iter_init_append (message, &iter);
dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->service_name);
+ dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->x11_display_name);
+ dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->display_device);
dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->hostname);
- dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->console_name);
if (! send_dbus_message (session->priv->worker_connection, message)) {
g_debug ("Could not send %s signal", "BeginVerification");
@@ -1413,8 +1420,9 @@
dbus_message_iter_init_append (message, &iter);
dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->service_name);
+ dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->x11_display_name);
+ dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->display_device);
dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->hostname);
- dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->console_name);
dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &session->priv->username);
if (! send_dbus_message (session->priv->worker_connection, message)) {
Index: daemon/gdm-session.h
===================================================================
--- daemon/gdm-session.h (revision 5357)
+++ daemon/gdm-session.h (working copy)
@@ -102,6 +102,7 @@
gboolean gdm_session_open (GdmSession *session,
const char *service_name,
const char *hostname,
+ const char *x11_display_name,
const char *console_name,
GError **error);
void gdm_session_close (GdmSession *session);
Index: daemon/gdm-session-worker.c
===================================================================
--- daemon/gdm-session-worker.c (revision 5357)
+++ daemon/gdm-session-worker.c (working copy)
@@ -775,7 +775,7 @@
const char *service,
const char *username,
const char *hostname,
- const char *console_name,
+ const char *display_device,
GError **error)
{
struct pam_conv pam_conversation;
@@ -836,7 +836,7 @@
}
}
- error_code = pam_set_item (worker->priv->pam_handle, PAM_TTY, console_name);
+ error_code = pam_set_item (worker->priv->pam_handle, PAM_TTY, display_device);
if (error_code != PAM_SUCCESS) {
g_set_error (error,
@@ -1111,7 +1111,8 @@
gdm_session_worker_verify_user (GdmSessionWorker *worker,
const char *service_name,
const char *hostname,
- const char *console_name,
+ const char *x11_display_name,
+ const char *display_device,
const char *username,
gboolean password_is_required,
GError **error)
@@ -1119,18 +1120,19 @@
GError *pam_error;
gboolean res;
- g_debug ("Verifying user: %s host: %s service: %s tty: %s",
+ g_debug ("Verifying user: %s host: %s service: %s display: %s tty: %s",
username != NULL ? username : "(null)",
hostname != NULL ? hostname : "(null)",
service_name != NULL ? service_name : "(null)",
- console_name != NULL ? console_name : "(null)");
+ x11_display_name != NULL ? x11_display_name : "(null)",
+ display_device != NULL ? display_device : "(null)");
pam_error = NULL;
res = gdm_session_worker_initialize_pam (worker,
service_name,
username,
hostname,
- console_name,
+ display_device,
&pam_error);
if (! res) {
g_propagate_error (error, pam_error);
@@ -1369,8 +1371,9 @@
static gboolean
gdm_session_worker_open (GdmSessionWorker *worker,
const char *service_name,
+ const char *x11_display_name,
+ const char *display_device,
const char *hostname,
- const char *console_name,
const char *username,
GError **error)
{
@@ -1384,7 +1387,8 @@
res = gdm_session_worker_verify_user (worker,
service_name,
hostname,
- console_name,
+ x11_display_name,
+ display_device,
username,
TRUE /* password is required */,
&verification_error);
@@ -1533,6 +1537,7 @@
typedef struct {
GdmSessionWorker *worker;
char *service;
+ char *x11_display_name;
char *console;
char *hostname;
char *username;
@@ -1549,6 +1554,7 @@
error = NULL;
res = gdm_session_worker_open (data->worker,
data->service,
+ data->x11_display_name,
data->console,
data->hostname,
data->username,
@@ -1576,6 +1582,7 @@
static void
queue_open (GdmSessionWorker *worker,
const char *service,
+ const char *x11_display_name,
const char *console,
const char *hostname,
const char *username)
@@ -1589,6 +1596,7 @@
data = g_new0 (OpenData, 1);
data->worker = worker;
data->service = g_strdup (service);
+ data->x11_display_name = g_strdup (x11_display_name);
data->console = g_strdup (console);
data->hostname = g_strdup (hostname);
data->username = g_strdup (username);
@@ -1601,6 +1609,7 @@
static void
on_begin_verification (DBusGProxy *proxy,
const char *service,
+ const char *x11_display_name,
const char *console,
const char *hostname,
gpointer data)
@@ -1608,12 +1617,13 @@
GdmSessionWorker *worker = GDM_SESSION_WORKER (data);
g_debug ("begin verification: %s %s", service, console);
- queue_open (worker, service, console, hostname, NULL);
+ queue_open (worker, service, x11_display_name, console, hostname, NULL);
}
static void
on_begin_verification_for_user (DBusGProxy *proxy,
const char *service,
+ const char *x11_display_name,
const char *console,
const char *hostname,
const char *username,
@@ -1622,7 +1632,7 @@
GdmSessionWorker *worker = GDM_SESSION_WORKER (data);
g_debug ("begin verification: %s %s", service, console);
- queue_open (worker, service, console, hostname, username);
+ queue_open (worker, service, x11_display_name, console, hostname, username);
}
static void
@@ -1689,12 +1699,20 @@
G_TYPE_STRING,
G_TYPE_STRING,
G_TYPE_INVALID);
+ dbus_g_object_register_marshaller (gdm_marshal_VOID__STRING_STRING_STRING_STRING_STRING,
+ G_TYPE_NONE,
+ G_TYPE_STRING,
+ G_TYPE_STRING,
+ G_TYPE_STRING,
+ G_TYPE_STRING,
+ G_TYPE_STRING,
+ G_TYPE_INVALID);
/* FIXME: not sure why introspection isn't working */
dbus_g_proxy_add_signal (worker->priv->server_proxy, "StartProgram", G_TYPE_STRING, G_TYPE_INVALID);
dbus_g_proxy_add_signal (worker->priv->server_proxy, "SetEnvironmentVariable", G_TYPE_STRING, G_TYPE_STRING, G_TYPE_INVALID);
- dbus_g_proxy_add_signal (worker->priv->server_proxy, "BeginVerification", G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_INVALID);
- dbus_g_proxy_add_signal (worker->priv->server_proxy, "BeginVerificationForUser", G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_INVALID);
+ dbus_g_proxy_add_signal (worker->priv->server_proxy, "BeginVerification", G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_INVALID);
+ dbus_g_proxy_add_signal (worker->priv->server_proxy, "BeginVerificationForUser", G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_STRING, G_TYPE_INVALID);
dbus_g_proxy_connect_signal (worker->priv->server_proxy,
"StartProgram",
Index: daemon/gdm-simple-slave.c
===================================================================
--- daemon/gdm-simple-slave.c (revision 5357)
+++ daemon/gdm-simple-slave.c (working copy)
@@ -904,8 +904,14 @@
on_greeter_cancel (GdmGreeterServer *greeter_server,
GdmSimpleSlave *slave)
{
+ char *display_name;
+
g_debug ("Greeter cancelled");
+ g_object_get (slave,
+ "display-name", &display_name,
+ NULL);
+
if (slave->priv->session != NULL) {
gdm_session_close (slave->priv->session);
g_object_unref (slave->priv->session);
@@ -916,8 +922,11 @@
gdm_session_open (slave->priv->session,
"gdm",
"" /* hostname */,
+ display_name,
"/dev/console",
NULL);
+
+ g_free (display_name);
}
static void
@@ -925,8 +934,10 @@
GdmSimpleSlave *slave)
{
gboolean display_is_local;
+ gchar *display_name;
g_object_get (slave,
+ "display-name", &display_name,
"display-is-local", &display_is_local,
NULL);
@@ -935,6 +946,7 @@
gdm_session_open (slave->priv->session,
"gdm",
"" /* hostname */,
+ display_name,
"/dev/console",
NULL);
@@ -942,6 +954,8 @@
if ( ! display_is_local) {
alarm (0);
}
+
+ g_free (display_name);
}
static void
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
