Skip to content

Commit

Permalink
src/hash_index: fix fd handling of r_hash_index_open()
Browse files Browse the repository at this point in the history
So far, r_hash_index_open() expected that the ownership of the provided
file descriptor should be handed over to the created RaucHashIndex which
will close it then during autofree handling.

However, this logic is error-prone since the caller has to know exactly
when it still needs to close the fd and when it may not touch it
anymore.

This resulted in wrong fd handling in generate_adaptive_data() error
cases and a double-close on the fd.

This was wrong anyway but results in a critical error in RAUC since glib
version 2.75.0 which comes with a pickier g_close() handling:
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2964

| (rauc:818): GLib-CRITICAL **: 13:33:51.290: g_close(fd:3) failed with EBADF. The tracking of file descriptors got messed up

Fix this by letting `r_hash_index_open()` dup the passed file
descriptor. This way the caller and the hash index can handle their
respective file descriptor independently.

Simplify and fix the code by removing now unnecessary fd invalidations
(data_fd = -1) and leveraging g_auto(filedesc).

Signed-off-by: Enrico Joerns <[email protected]>
  • Loading branch information
ejoerns committed Aug 30, 2024
1 parent 4cba803 commit d890477
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 14 deletions.
4 changes: 1 addition & 3 deletions src/bundle.c
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ static gboolean generate_adaptive_data(RaucManifest *manifest, const gchar *dir,
g_autofree gchar *indexname = g_strconcat(image->filename, ".block-hash-index", NULL);
g_autofree gchar *indexpath = g_build_filename(dir, indexname, NULL);
g_autoptr(RaucHashIndex) index = NULL;
int fd = -1;
g_auto(filedesc) fd = -1;

if (image_is_archive(image)) {
g_warning("Generating block hash index requires a block device image but %s looks like an archive", image->filename);
Expand All @@ -419,7 +419,6 @@ static gboolean generate_adaptive_data(RaucManifest *manifest, const gchar *dir,
error,
ierror,
"Failed to generate hash index for %s: ", image->filename);
g_close(fd, NULL);
return FALSE;
}

Expand All @@ -428,7 +427,6 @@ static gboolean generate_adaptive_data(RaucManifest *manifest, const gchar *dir,
error,
ierror,
"Failed to write hash index for %s: ", image->filename);
g_close(fd, NULL);
return FALSE;
}

Expand Down
14 changes: 3 additions & 11 deletions src/hash_index.c
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ RaucHashIndex *r_hash_index_open(const gchar *label, int data_fd, const gchar *h
g_return_val_if_fail(error == NULL || *error == NULL, NULL);

idx->label = g_strdup(label);
idx->data_fd = data_fd;
idx->data_fd = dup(data_fd);

idx->count = get_chunk_count(data_fd, &ierror);
if (!idx->count) {
Expand Down Expand Up @@ -321,7 +321,7 @@ RaucHashIndex *r_hash_index_open_slot(const gchar *label, const RaucSlot *slot,
g_autoptr(RaucHashIndex) idx = NULL;
g_autofree gchar *dir = NULL;
g_autofree gchar *index_filename = NULL;
int data_fd = -1;
g_auto(filedesc) data_fd = -1;

g_return_val_if_fail(label, NULL);
g_return_val_if_fail(slot, NULL);
Expand Down Expand Up @@ -351,13 +351,9 @@ RaucHashIndex *r_hash_index_open_slot(const gchar *label, const RaucSlot *slot,
g_propagate_error(error, ierror);
goto out;
}
data_fd = -1; /* belongs to idx now */

g_debug("opened hash index for slot %s as %s", slot->name, label);
out:
if (data_fd >= 0) {
g_close(data_fd, NULL);
}
return g_steal_pointer(&idx);
}

Expand All @@ -366,7 +362,7 @@ RaucHashIndex *r_hash_index_open_image(const gchar *label, const RaucImage *imag
GError *ierror = NULL;
g_autoptr(RaucHashIndex) idx = NULL;
g_autofree gchar *index_filename = NULL;
int data_fd = -1;
g_auto(filedesc) data_fd = -1;

g_return_val_if_fail(label, NULL);
g_return_val_if_fail(image, NULL);
Expand All @@ -389,13 +385,9 @@ RaucHashIndex *r_hash_index_open_image(const gchar *label, const RaucImage *imag
g_propagate_error(error, ierror);
goto out;
}
data_fd = -1; /* belongs to idx now */

g_debug("opened hash index for image %s with index %s", image->filename, index_filename);
out:
if (data_fd >= 0) {
g_close(data_fd, NULL);
}
return g_steal_pointer(&idx);
}

Expand Down

0 comments on commit d890477

Please sign in to comment.