Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XdpAppInfo cleanups #1619

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions src/xdp-app-info-flatpak.c
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ flatpak_is_valid_name (const char *string)
return TRUE;
}

gboolean
static gboolean
xdp_app_info_flatpak_is_valid_sub_app_id (XdpAppInfo *app_info,
const char *sub_app_id)
{
Expand Down Expand Up @@ -208,11 +208,11 @@ xdp_app_info_flatpak_remap_path (XdpAppInfo *app_info,
NULL);

/* For apps we translate /app and /usr to the installed locations.
Also, we need to rewrite to drop the /newroot prefix added by
bubblewrap for other files to work. See
https://github.com/projectatomic/bubblewrap/pull/172
for a bit more information on the /newroot issue.
*/
* Also, we need to rewrite to drop the /newroot prefix added by
* bubblewrap for other files to work. See
* https://github.com/projectatomic/bubblewrap/pull/172
* for a bit more information on the /newroot issue.
*/

if (g_str_has_prefix (path, "/newroot/"))
path = path + strlen ("/newroot");
Expand Down Expand Up @@ -723,6 +723,7 @@ xdp_app_info_flatpak_new (int pid,
GError **error)
{
g_autoptr (XdpAppInfoFlatpak) app_info_flatpak = NULL;
XdpAppInfoFlags flags = 0;
g_autofd int info_fd = -1;
struct stat stat_buf;
g_autoptr(GError) local_error = NULL;
Expand Down Expand Up @@ -807,18 +808,22 @@ xdp_app_info_flatpak_new (int pid,
/* flatpak has a xdg-dbus-proxy running which means we can't get the pidfd
* of the connected process but we can get the pidfd of the bwrap instance
* instead. This is okay because it has the same namespaces as the calling
* process. */
* process.
*/
bwrap_pidfd = get_bwrap_pidfd (instance, error);
if (bwrap_pidfd == -1)
return NULL;

/* TODO: we can use pidfd to make sure we didn't race for sure */

flags |= XDP_APP_INFO_FLAG_SUPPORTS_OPATH;
if (has_network)
flags |= XDP_APP_INFO_FLAG_HAS_NETWORK;

app_info_flatpak = g_object_new (XDP_TYPE_APP_INFO_FLATPAK, NULL);
xdp_app_info_initialize (XDP_APP_INFO (app_info_flatpak),
FLATPAK_ENGINE_ID, id, instance,
bwrap_pidfd, gappinfo,
TRUE, has_network, TRUE);
bwrap_pidfd, gappinfo, flags);
app_info_flatpak->flatpak_info = g_steal_pointer (&metadata);

return XDP_APP_INFO (g_steal_pointer (&app_info_flatpak));
Expand Down
11 changes: 5 additions & 6 deletions src/xdp-app-info-host.c
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ xdp_app_info_host_get_usb_queries (XdpAppInfo *app_info)
return app_info_host->usb_queries;
}

gboolean
static gboolean
xdp_app_info_host_is_valid_sub_app_id (XdpAppInfo *app_info,
const char *sub_app_id)
{
return TRUE;
}

gboolean
static gboolean
xdp_app_info_host_validate_autostart (XdpAppInfo *app_info,
GKeyFile *keyfile,
const char * const *autostart_exec,
Expand All @@ -61,7 +61,7 @@ xdp_app_info_host_validate_autostart (XdpAppInfo *app_info,
return TRUE;
}

gboolean
static gboolean
xdp_app_info_host_validate_dynamic_launcher (XdpAppInfo *app_info,
GKeyFile *key_file,
GError **error)
Expand Down Expand Up @@ -197,9 +197,8 @@ xdp_app_info_host_new_full (const char *app_id,
/* engine, app id, instance */
NULL, app_id, NULL,
pidfd, gappinfo,
/* supports_opath */ TRUE,
/* has_network */ TRUE,
/* requires_pid_mapping */ FALSE);
XDP_APP_INFO_FLAG_HAS_NETWORK |
XDP_APP_INFO_FLAG_SUPPORTS_OPATH);

return app_info_host;
}
Expand Down
22 changes: 13 additions & 9 deletions src/xdp-app-info-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@

#include "xdp-app-info.h"

typedef enum
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typedef enum _XdpAppInfoFlags

tbh, I'm unsure why this even is a pattern in e.g. mutter but at least it makes it easy to grep for the declaration...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice because one can use ctags to go directly to the actual struct, instead of the typedef, by adding a _ in front of the name.

{
XDP_APP_INFO_FLAG_HAS_NETWORK = (1 << 0),
XDP_APP_INFO_FLAG_SUPPORTS_OPATH = (1 << 1),
} XdpAppInfoFlags;

struct _XdpAppInfoClass
{
GObjectClass parent_class;
Expand All @@ -46,12 +52,10 @@ struct _XdpAppInfoClass
GError **error);
};

void xdp_app_info_initialize (XdpAppInfo *app_info,
const char *engine,
const char *app_id,
const char *instance,
int pidfd,
GAppInfo *gappinfo,
gboolean supports_opath,
gboolean has_network,
gboolean requires_pid_mapping);
void xdp_app_info_initialize (XdpAppInfo *app_info,
const char *engine,
const char *app_id,
const char *instance,
int pidfd,
GAppInfo *gappinfo,
XdpAppInfoFlags flags);
7 changes: 5 additions & 2 deletions src/xdp-app-info-snap.c
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ xdp_app_info_snap_new (int pid,
g_autofree char *snap_id = NULL;
g_autofree char *desktop_id = NULL;
g_autoptr(GAppInfo) gappinfo = NULL;
XdpAppInfoFlags flags = 0;
gboolean has_network;

/* Check the process's cgroup membership to fail quickly for non-snaps */
Expand Down Expand Up @@ -222,11 +223,13 @@ xdp_app_info_snap_new (int pid,
SNAP_METADATA_KEY_NETWORK,
NULL);

if (has_network)
flags |= XDP_APP_INFO_FLAG_HAS_NETWORK;

app_info_snap = g_object_new (XDP_TYPE_APP_INFO_SNAP, NULL);
xdp_app_info_initialize (XDP_APP_INFO (app_info_snap),
"io.snapcraft", snap_id, NULL,
pidfd, gappinfo,
FALSE, has_network, TRUE);
pidfd, gappinfo, flags);

return XDP_APP_INFO (g_steal_pointer (&app_info_snap));
}
5 changes: 3 additions & 2 deletions src/xdp-app-info-test.c
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ xdp_app_info_test_get_usb_queries (XdpAppInfo *app_info)
return app_info_test->usb_queries;
}

gboolean
static gboolean
xdp_app_info_test_is_valid_sub_app_id (XdpAppInfo *app_info,
const char *sub_app_id)
{
Expand Down Expand Up @@ -137,7 +137,8 @@ xdp_app_info_test_new (const char *app_id,
xdp_app_info_initialize (XDP_APP_INFO (app_info_test),
"", app_id, NULL,
-1, NULL,
TRUE, TRUE, TRUE);
XDP_APP_INFO_FLAG_HAS_NETWORK |
XDP_APP_INFO_FLAG_SUPPORTS_OPATH);

app_info_test->usb_queries = parse_usb_queries_string (usb_queries_str);

Expand Down
60 changes: 28 additions & 32 deletions src/xdp-app-info.c
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ typedef struct _XdpAppInfoPrivate
char *instance;
int pidfd;
GAppInfo *gappinfo;
gboolean supports_opath;
gboolean has_network;
gboolean requires_pid_mapping;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I screwed something up when refactoring XdpAppInfo? Might be worth double checking what this was used for...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was never used:

XdpAppInfoFlags flags;

/* pid namespace mapping */
GMutex pidns_lock;
Expand Down Expand Up @@ -111,15 +109,13 @@ xdp_app_info_init (XdpAppInfo *app_info)
}

void
xdp_app_info_initialize (XdpAppInfo *app_info,
const char *engine,
const char *app_id,
const char *instance,
int pidfd,
GAppInfo *gappinfo,
gboolean supports_opath,
gboolean has_network,
gboolean requires_pid_mapping)
xdp_app_info_initialize (XdpAppInfo *app_info,
const char *engine,
const char *app_id,
const char *instance,
int pidfd,
GAppInfo *gappinfo,
XdpAppInfoFlags flags)
{
XdpAppInfoPrivate *priv = xdp_app_info_get_instance_private (app_info);

Expand All @@ -128,9 +124,7 @@ xdp_app_info_initialize (XdpAppInfo *app_info,
priv->instance = g_strdup (instance);
priv->pidfd = dup (pidfd);
g_set_object (&priv->gappinfo, gappinfo);
priv->supports_opath = supports_opath;
priv->has_network = has_network;
priv->requires_pid_mapping = requires_pid_mapping;
priv->flags = flags;
}

gboolean
Expand Down Expand Up @@ -195,7 +189,7 @@ xdp_app_info_has_network (XdpAppInfo *app_info)

priv = xdp_app_info_get_instance_private (app_info);

return priv->has_network;
return (priv->flags & XDP_APP_INFO_FLAG_HAS_NETWORK) != 0;
}

gboolean
Expand Down Expand Up @@ -234,7 +228,6 @@ static char *
remap_path (XdpAppInfo *app_info,
const char *path)
{

if (!XDP_APP_INFO_GET_CLASS (app_info)->remap_path)
return g_strdup (path);

Expand Down Expand Up @@ -262,8 +255,9 @@ verify_proc_self_fd (XdpAppInfo *app_info,
path_buffer[symlink_size] = 0;

/* All normal paths start with /, but some weird things
don't, such as socket:[27345] or anon_inode:[eventfd].
We don't support any of these */
* don't, such as socket:[27345] or anon_inode:[eventfd].
* We don't support any of these.
*/
if (path_buffer[0] != '/')
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_FILENAME,
Expand All @@ -272,23 +266,25 @@ verify_proc_self_fd (XdpAppInfo *app_info,
}

/* File descriptors to actually deleted files have " (deleted)"
appended to them. This also happens to some fake fd types
like shmem which are "/<name> (deleted)". All such
files are considered invalid. Unfortunately this also
matches files with filenames that actually end in " (deleted)",
but there is not much to do about this. */
* appended to them. This also happens to some fake fd types
* like shmem which are "/<name> (deleted)". All such
* files are considered invalid. Unfortunately this also
* matches files with filenames that actually end in " (deleted)",
* but there is not much to do about this.
*/
if (g_str_has_suffix (path_buffer, " (deleted)"))
{
const char *mountpoint = xdp_get_documents_mountpoint ();

if (mountpoint != NULL && g_str_has_prefix (path_buffer, mountpoint))
{
/* Unfortunately our workaround for dcache purging triggers
o_path file descriptors on the fuse filesystem being
marked as deleted, so we have to allow these here and
rewrite them. This is safe, becase we will stat the file
and compare to make sure we end up on the right file. */
path_buffer[symlink_size - strlen(" (deleted)")] = 0;
* o_path file descriptors on the fuse filesystem being
* marked as deleted, so we have to allow these here and
* rewrite them. This is safe, becase we will stat the file
* and compare to make sure we end up on the right file.
*/
path_buffer[symlink_size - strlen (" (deleted)")] = 0;
}
else
{
Expand Down Expand Up @@ -438,7 +434,7 @@ xdp_app_info_get_path_for_fd (XdpAppInfo *app_info,
return NULL;
}

if (!priv->supports_opath)
if ((priv->flags & XDP_APP_INFO_FLAG_SUPPORTS_OPATH) == 0)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
"App \"%s\" of type %s does not support O_PATH fd passing",
Expand Down Expand Up @@ -876,7 +872,7 @@ xdp_connection_create_host_app_info_sync (GDBusConnection *connection,
if (is_sandboxed)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
"Can't manually register a flatpak application");
"Can't manually register a Flatpak application");
return NULL;
}

Expand All @@ -886,7 +882,7 @@ xdp_connection_create_host_app_info_sync (GDBusConnection *connection,
if (is_sandboxed)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
"Can't manually register a snap application");
"Can't manually register a Snap application");
return NULL;
}

Expand Down