Deduplicate and fix up our use of mmap()

Buried in this large patch is a logical fix:

```
-  if (!map)
-    return glnx_throw_errno_prefix (error, "mmap");
+  if (map == (void*)-1)
+    return glnx_null_throw_errno_prefix (error, "mmap");
```

Which would have helped me debug another patch I was working
on.  But it turns out that actually correctly checking for
errors from `mmap()` triggers lots of other bugs - basically
because we sometimes handle zero-length variants (in detached
metadata).  When we start actually returning errors due to
this, things break.  (It wasn't a problem in practice before
because most things looked at the zero size, not the data).

Anyways there's a bigger picture issue here - a while ago
we made a fix to only use `mmap()` for reading metadata from disk
only if it was large enough (i.e. `>16k`).  But that didn't
help various other paths in the pull code and others that were
directly doing the `mmap()`.

Fix this by having a proper low level fs helper that does "read all data from
fd+offset into GBytes", which handles the size check. Then the `GVariant` bits
are just a clean layer on top of this. (At the small cost of an additional
allocation)

Side note: I had to remind myself, but the reason we can't just use
`GMappedFile` here is it doesn't support passing an offset into `mmap()`.

Closes: #1251
Approved by: jlebon
This commit is contained in:
Colin Walters authored and Atomic Bot committed 2017-10-04 20:42:39 +00:00
1 parent c511ca0fae
commit 5c7d2dd8be
10 files changed
+131 -172

No files matched your search

+20 -17
View File
@@ -2212,26 +2212,29 @@ ostree_repo_read_commit_detached_metadata (OstreeRepo *self,
char buf[_OSTREE_LOOSE_PATH_MAX];
_ostree_loose_path (buf, checksum, OSTREE_OBJECT_TYPE_COMMIT_META, self->mode);
g_autoptr(GVariant) ret_metadata = NULL;
if (self->commit_stagedir.initialized &&
!ot_util_variant_map_at (self->commit_stagedir.fd, buf,
G_VARIANT_TYPE ("a{sv}"),
OT_VARIANT_MAP_ALLOW_NOENT | OT_VARIANT_MAP_TRUSTED, &ret_metadata, error))
return glnx_prefix_error (error, "Unable to read existing detached metadata");
if (self->commit_stagedir.initialized)
{
glnx_fd_close int fd = -1;
if (!ot_openat_ignore_enoent (self->commit_stagedir.fd, buf, &fd, error))
return FALSE;
if (fd != -1)
return ot_variant_read_fd (fd, 0, G_VARIANT_TYPE ("a{sv}"), TRUE,
out_metadata, error);
}
if (ret_metadata == NULL &&
!ot_util_variant_map_at (self->objects_dir_fd, buf,
G_VARIANT_TYPE ("a{sv}"),
OT_VARIANT_MAP_ALLOW_NOENT | OT_VARIANT_MAP_TRUSTED, &ret_metadata, error))
return glnx_prefix_error (error, "Unable to read existing detached metadata");
glnx_fd_close int fd = -1;
if (!ot_openat_ignore_enoent (self->objects_dir_fd, buf, &fd, error))
return FALSE;
if (fd != -1)
return ot_variant_read_fd (fd, 0, G_VARIANT_TYPE ("a{sv}"), TRUE,
out_metadata, error);
if (ret_metadata == NULL && self->parent_repo)
if (self->parent_repo)
return ostree_repo_read_commit_detached_metadata (self->parent_repo,
checksum,
out_metadata,
cancellable,
error);
ot_transfer_out_value (out_metadata, &ret_metadata);
checksum, out_metadata,
cancellable, error);
/* Nothing found */
*out_metadata = NULL;
return TRUE;
}
+4 -4
View File
@@ -1228,8 +1228,8 @@ meta_fetch_on_complete (GObject *object,
if (fetch_data->is_detached_meta)
{
if (!ot_util_variant_map_fd (fd, 0, G_VARIANT_TYPE ("a{sv}"),
FALSE, &metadata, error))
if (!ot_variant_read_fd (fd, 0, G_VARIANT_TYPE ("a{sv}"),
FALSE, &metadata, error))
goto out;
if (!ostree_repo_write_commit_detached_metadata (pull_data->repo, checksum, metadata,
@@ -1245,8 +1245,8 @@ meta_fetch_on_complete (GObject *object,
}
else
{
if (!ot_util_variant_map_fd (fd, 0, ostree_metadata_variant_type (objtype),
FALSE, &metadata, error))
if (!ot_variant_read_fd (fd, 0, ostree_metadata_variant_type (objtype),
FALSE, &metadata, error))
goto out;
/* Write the commitpartial file now while we're still fetching data */
+10 -7
View File
@@ -246,8 +246,8 @@ ostree_repo_static_delta_execute_offline (OstreeRepo *self,
return glnx_throw_errno_prefix (error, "openat(%s)", basename);
g_autoptr(GVariant) meta = NULL;
if (!ot_util_variant_map_fd (meta_fd, 0, G_VARIANT_TYPE (OSTREE_STATIC_DELTA_SUPERBLOCK_FORMAT),
FALSE, &meta, error))
if (!ot_variant_read_fd (meta_fd, 0, G_VARIANT_TYPE (OSTREE_STATIC_DELTA_SUPERBLOCK_FORMAT),
FALSE, &meta, error))
return FALSE;
/* Parsing OSTREE_STATIC_DELTA_SUPERBLOCK_FORMAT */
@@ -448,8 +448,8 @@ _ostree_static_delta_part_open (GInputStream *part_in,
int part_fd = g_file_descriptor_based_get_fd ((GFileDescriptorBased*)part_in);
/* No compression, no checksums - a fast path */
if (!ot_util_variant_map_fd (part_fd, 1, G_VARIANT_TYPE (OSTREE_STATIC_DELTA_PART_PAYLOAD_FORMAT_V0),
trusted, &ret_part, error))
if (!ot_variant_read_fd (part_fd, 1, G_VARIANT_TYPE (OSTREE_STATIC_DELTA_PART_PAYLOAD_FORMAT_V0),
trusted, &ret_part, error))
return FALSE;
}
else
@@ -767,9 +767,12 @@ _ostree_repo_static_delta_dump (OstreeRepo *self,
superblock_path = _ostree_get_relative_static_delta_superblock_path (from, to);
if (!ot_util_variant_map_at (self->repo_dir_fd, superblock_path,
(GVariantType*)OSTREE_STATIC_DELTA_SUPERBLOCK_FORMAT,
OT_VARIANT_MAP_TRUSTED, &delta_superblock, error))
glnx_fd_close int superblock_fd = -1;
if (!glnx_openat_rdonly (self->repo_dir_fd, superblock_path, TRUE, &superblock_fd, error))
return FALSE;
if (!ot_variant_read_fd (superblock_fd, 0,
(GVariantType*)OSTREE_STATIC_DELTA_SUPERBLOCK_FORMAT,
TRUE, &delta_superblock, error))
return FALSE;
g_print ("Delta: %s\n", delta_id);
+17 -31
View File
@@ -2811,7 +2811,6 @@ load_metadata_internal (OstreeRepo *self,
GError **error)
{
char loose_path_buf[_OSTREE_LOOSE_PATH_MAX];
struct stat stbuf;
glnx_fd_close int fd = -1;
g_autoptr(GInputStream) ret_stream = NULL;
g_autoptr(GVariant) ret_variant = NULL;
@@ -2853,36 +2852,14 @@ load_metadata_internal (OstreeRepo *self,
if (fd != -1)
{
struct stat stbuf;
if (!glnx_fstat (fd, &stbuf, error))
return FALSE;
if (out_variant)
{
/* http://stackoverflow.com/questions/258091/when-should-i-use-mmap-for-file-access */
if (stbuf.st_size > 16*1024)
{
GMappedFile *mfile;
mfile = g_mapped_file_new_from_fd (fd, FALSE, error);
if (!mfile)
return FALSE;
ret_variant = g_variant_new_from_data (ostree_metadata_variant_type (objtype),
g_mapped_file_get_contents (mfile),
g_mapped_file_get_length (mfile),
TRUE,
(GDestroyNotify) g_mapped_file_unref,
mfile);
g_variant_ref_sink (ret_variant);
}
else
{
g_autoptr(GBytes) data = glnx_fd_readall_bytes (fd, cancellable, error);
if (!data)
return FALSE;
ret_variant = g_variant_new_from_bytes (ostree_metadata_variant_type (objtype),
data, TRUE);
g_variant_ref_sink (ret_variant);
}
if (!ot_variant_read_fd (fd, 0, ostree_metadata_variant_type (objtype), TRUE,
&ret_variant, error))
return FALSE;
/* Now, let's put it in the cache */
if (is_dirmeta_cachable)
@@ -4202,15 +4179,24 @@ ostree_repo_add_gpg_signature_summary (OstreeRepo *self,
GCancellable *cancellable,
GError **error)
{
g_autoptr(GBytes) summary_data = ot_file_mapat_bytes (self->repo_dir_fd, "summary", error);
glnx_fd_close int fd = -1;
if (!glnx_openat_rdonly (self->repo_dir_fd, "summary", TRUE, &fd, error))
return FALSE;
g_autoptr(GBytes) summary_data = ot_fd_readall_or_mmap (fd, 0, error);
if (!summary_data)
return FALSE;
/* Note that fd is reused below */
(void) close (glnx_steal_fd (&fd));
g_autoptr(GVariant) existing_signatures = NULL;
if (!ot_util_variant_map_at (self->repo_dir_fd, "summary.sig",
G_VARIANT_TYPE (OSTREE_SUMMARY_SIG_GVARIANT_STRING),
OT_VARIANT_MAP_ALLOW_NOENT, &existing_signatures, error))
if (!ot_openat_ignore_enoent (self->repo_dir_fd, "summary.sig", &fd, error))
return FALSE;
if (fd != -1)
{
if (!ot_variant_read_fd (fd, 0, G_VARIANT_TYPE (OSTREE_SUMMARY_SIG_GVARIANT_STRING),
FALSE, &existing_signatures, error))
return FALSE;
}
g_autoptr(GVariant) new_metadata = NULL;
for (guint i = 0; key_id[i]; i++)