Jiri Moskovcak <[email protected]> writes:
> +static void handle_method_call(GDBusConnection *connection,
> + const gchar *caller,
> + const gchar *object_path,
> + const gchar *interface_name,
> + const gchar *method_name,
> + GVariant *parameters,
> + GDBusMethodInvocation *invocation,
> + gpointer user_data)
> +{
> + reset_timeout();
> + uid_t caller_uid;
> + GVariant *response;
> +
> + if (g_strcmp0(method_name, "GetProblems") == 0)
> + {
> +
^ pointless new line
> + caller_uid = get_caller_uid(connection, invocation, caller);
> +
> + if (caller_uid == (uid_t) -1)
> + return;
exctract from all parts of if () caller_uid and check before if's
> + }
> +
^ pointless new line before every *else if*
> + else if (g_strcmp0(method_name, "GetAllProblems") == 0)
> + {
> +
dito
> + }
> +
> + else if (g_strcmp0(method_name, "ChownProblemDir") == 0)
> + {
...
...
...
> + if (stat(problem_dir, &statbuf) == 0 && S_ISDIR(statbuf.st_mode))
> + {
> + if (caller_uid == 0 || uid_in_group(caller_uid, statbuf.st_gid))
> //caller seems to be in group with access to this dir, so no action needed
> + {
> + //return ok
remove comment
> + VERB1 log("caller has access to the requested directory %s",
> problem_dir);
> + g_dbus_method_invocation_return_value(invocation, NULL);
> + dd_close(dd);
> + return;
> + //free something?
dito comment
> + }
> +
> + }
> + else
> + {
> + g_dbus_method_invocation_return_dbus_error(invocation,
> +
> "org.freedesktop.problems.StatFailure",
> + strerror(errno));
> + dd_close(dd);
> + return;
> + }
> + pwd = getpwuid(caller_uid);
> + if (pwd)
> + {
> + errno = 0;
> + chown_res = chown(problem_dir, statbuf.st_uid, pwd->pw_gid);
you don't have to set errno to 0, because if chown on success return
0. On error -1 errno is set.
> + dd_init_next_file(dd);
> + char *short_name, *full_name;
> + while (chown_res == 0 && dd_get_next_file(dd, &short_name,
> &full_name))
> + {
> + VERB3 log("chowning %s", full_name);
> + chown_res = chown(full_name, statbuf.st_uid, pwd->pw_gid);
> + }
> +
> + if (chown_res != 0)
according to man-pages, you should have to check -1
> + g_dbus_method_invocation_return_dbus_error(invocation,
> +
> "org.freedesktop.problems.ChownError",
> + strerror(errno));
> +
> + g_dbus_method_invocation_return_value(invocation, NULL);
> + dd_close(dd);
> + return;
> + }
> + VERB3 log("shouldn't get here");
> + dd_close(dd);
> + }
> +
> + else if (g_strcmp0(method_name, "GetInfo") == 0)
> + {
> + VERB1 log("GetInfo");
> +
> + const gchar *problem_dir;
> + g_variant_get(parameters, "(&s)", &problem_dir);
> +
> + GVariantBuilder *builder;
> +
> + struct stat statbuf;
> + errno = 0;
do not reset errno unless it's needed
On success, zero is returned. On error, -1 is returned, and errno is
set appropriately.
> + if (stat(problem_dir, &statbuf) != 0)
> + {
> + g_dbus_method_invocation_return_dbus_error(invocation,
> +
> "org.freedesktop.problems.GetInfoError",
> + strerror(errno));
> + return;
> + }
> +
> +
> + caller_uid = get_caller_uid(connection, invocation, caller);
> +
> + if (caller_uid == (uid_t) -1)
> + return;
> +
> + if (!uid_in_group(caller_uid, statbuf.st_gid))
> + {
> + if (polkit_check_authorization_dname(caller,
> "org.freedesktop.problems.getall") != PolkitYes)
> + {
> + VERB1 log("not authorized");
> + g_dbus_method_invocation_return_dbus_error(invocation,
> +
> "org.freedesktop.problems.AuthFailure",
> + "Not Authorized");
^^^ not translatable
some others are not translatable also
> + return;
> + }
> + }
> +
> + struct dump_dir *dd = dd_opendir(problem_dir, DD_OPEN_READONLY |
> DD_FAIL_QUIETLY_EACCES);
> + if (!dd)
> + {
> + char *error_msg = g_strdup_printf(_("%s is not a valid problem
> directory"), problem_dir);
> + g_dbus_method_invocation_return_dbus_error(invocation,
> +
> "org.freedesktop.problems.GetInfo",
> + error_msg);
> + free(error_msg);
> + }
here is a bug, when dd is NULL then you print msg and continue to
dd_load_text(dd... to have to exit here.
> + builder = g_variant_builder_new(G_VARIANT_TYPE_ARRAY);
> + g_variant_builder_add (builder, "{ss}", g_strdup(FILENAME_TIME),
> dd_load_text(dd, FILENAME_TIME));
> + g_variant_builder_add (builder, "{ss}", g_strdup(FILENAME_REASON),
> dd_load_text(dd, FILENAME_REASON));
> + char *not_reportable_reason = dd_load_text_ext(dd,
> FILENAME_NOT_REPORTABLE, 0
> + |
> DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
> + |
> DD_FAIL_QUIETLY_ENOENT
> + |
> DD_FAIL_QUIETLY_EACCES);
> + if (not_reportable_reason)
> + g_variant_builder_add (builder, "{ss}",
> g_strdup(FILENAME_NOT_REPORTABLE), dd_load_text_ext(dd,
> FILENAME_NOT_REPORTABLE, 0
> +
> | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
> +
> | DD_FAIL_QUIETLY_ENOENT
> +
> | DD_FAIL_QUIETLY_EACCES));
> + /* the source of the problem:
> + * - first we try to load component, as we use it on Fedora
> + */
> + char *source = dd_load_text_ext(dd, FILENAME_COMPONENT, 0
> + | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
> + | DD_FAIL_QUIETLY_ENOENT
> + | DD_FAIL_QUIETLY_EACCES
> + );
> + /* if we don't have component, we fallback to executable */
> + if (!source)
> + {
> + source = dd_load_text_ext(dd, FILENAME_EXECUTABLE, 0
> + | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
> + | DD_FAIL_QUIETLY_ENOENT
> + | DD_FAIL_QUIETLY_EACCES
> + );
> + }
> +
> + g_variant_builder_add (builder, "{ss}", g_strdup("source"), source);
> + char *msg = dd_load_text_ext(dd, FILENAME_REPORTED_TO, 0
> + | DD_LOAD_TEXT_RETURN_NULL_ON_FAILURE
> + | DD_FAIL_QUIETLY_ENOENT
> + | DD_FAIL_QUIETLY_EACCES
> + );
> + if (msg)
> + g_variant_builder_add (builder, "{ss}",
> g_strdup(FILENAME_REPORTED_TO), msg);
> +
> + dd_close(dd);
> +
> + GVariant *response = g_variant_new("(a{ss})", builder);
> + g_variant_builder_unref(builder);
according to me, you are leaking everywhere here. g_variant_builde_unref
only count donw reference counter and you should take care of all returned
vaules from load_text and g_strdup (and of course you should have to use
xstrdup or check g_strdup to NULL)
> + g_dbus_method_invocation_return_value(invocation, response);
> + }
> +
> + else if (g_strcmp0(method_name, "Quit") == 0)
> + {
> + VERB1 log("Quit");
> +
> + g_dbus_method_invocation_return_value(invocation, NULL);
> +
> + g_main_loop_quit(loop);
> +
> + }
> +}
> +
> +/* for now */
> +static const GDBusInterfaceVTable interface_vtable =
> +{
> + handle_method_call,
> + NULL,
> + NULL,
> +};
this looks like a function. I don't have any strong opinion here, but
more readable solution is
static const foo = {
.x = bar;
};
what is problem here is big or little endians. C doen't guarantee order
of list fields. you have to use
static const GDBusInterfaceVTable interface_vtable =
{
.GDBusInterfaceMethodCallFunc = handle_method_call,
.GDBusInterfaceGetPropertyFunc = NULL,
.GDBusInterfaceSetPropertyFunc = NULL,
};
> +static void on_name_acquired (GDBusConnection *connection,
> + const gchar *name,
> + gpointer user_data)
> +{
> +}
e? couldn't be set to NULL at g_bus_own_name call?
> +static void on_name_lost (GDBusConnection *connection,
> + const gchar *name,
> + gpointer user_data)
> +{
> + g_print(_("The name '%s' has been lost, please check if other "
> + "service owning the name is not running.\n"), name);
> + exit(1);
daemon should not exit(1)
> +}
> +
> +int main (int argc, char *argv[])
> +{
> + /* I18n */
> + setlocale(LC_ALL, "");
> +#if ENABLE_NLS
> + bindtextdomain(PACKAGE, LOCALEDIR);
> + textdomain(PACKAGE);
> +#endif
> + guint owner_id;
> +
> + abrt_init(argv);
> +
> + const char *program_usage_string = _(
> + "& [options]"
> + );
> + enum {
> + OPT_v = 1 << 0,
> + OPT_t = 1 << 1,
> + };
> + /* Keep enum above and order of options below in sync! */
> + struct options program_options[] = {
> + OPT__VERBOSE(&g_verbose),
> + OPT_INTEGER('t', NULL, &s_timeout, _("Exit after NUM seconds of
> inactivity")),
> + OPT_END()
> + };
> + unsigned opts = parse_opts(argc, argv, program_options,
> program_usage_string);
> +
> + export_abrt_envvars(0);
> +
> + /* When dbus daemon starts us, it doesn't set PATH
> + * (I saw it set only DBUS_STARTER_ADDRESS and DBUS_STARTER_BUS_TYPE).
> + * In this case, set something sane:
> + */
> + const char *env_path = getenv("PATH");
> + if (!env_path || !env_path[0])
> + putenv((char*)"PATH=/usr/sbin:/usr/bin:/sbin:/bin");
> +
> + msg_prefix = "abrt-dbus"; /* for log(), error_msg() and such */
> +
> + if (!(opts & OPT_t))
> + s_timeout = 120; //if the timeout is not set we default to 120sec
s_timeout is not used; it's write only
> +static PolkitResult do_check(PolkitSubject *subject, const char *action_id)
> +{
...
...
...
> + if (polkit_authorization_result_get_is_challenge(auth_result))
> + {
> + /* Can't happen (happens only with
> + * POLKIT_CHECK_AUTHORIZATION_FLAGS_NONE flag) */
> + result = PolkitChallenge;
> + goto out;
> + }
> +
> + if (polkit_authorization_result_get_is_authorized(auth_result))
> + {
> + result = PolkitYes;
> + goto out;
pointless goto; you will reach the out after jump off the *if*
> + }
> +
> +out:
> + g_object_unref(auth_result);
> + return result;
> +}
comment please, because I haven't run it yet
--
Nikola
