Download raw body.
change got_worktree_init, open_worktree to use fds
Attached are the changes exactly as you've described. The amount of
code-changing required does decrease drastically if we move cap_enter()
after opening the repo and worktree. I suppose the 'secondary benefits'
of using fds are not relevant in got_worktree_init and _open's case, so
this is all agreeable to me.
On Wed, Dec 9, 2020 at 10:14 AM Stefan Sperling <stsp@stsp.name> wrote:
>
> On Wed, Dec 09, 2020 at 06:28:10PM +0100, Stefan Sperling wrote:
> > On Wed, Dec 09, 2020 at 09:00:00AM -0800, Yang Zhong wrote:
> > > Here's the updated diff. With regard to committing, am I OK to
> > > commit the initial patch (the one that adds the fd field to got_worktree)
> > > at this point?
> >
> > I didn't realize the new patch you posted wasn't self-contained.
> >
> > You mean the initial patch quoted below?
> >
> > My point of view on this is that you should not actually need most of
> > these changes if you call cap_enter() after got_worktree_open() and
> > got_repo_open() are done.
> > Adding a root_fd to struct got_worktree is fine, of course. I would suggest
> > to roll a new patch that is self-contained and doesn't assume that your
> > sandbox is active during got_worktree_init/open() and got_repo_open().
>
> To clarify, I imagine this could work somewhat as follows:
>
> got_worktree_init() stays as-is (it does not need sandbox protection
> since all the data involved is created locally.)
>
> open_worktree() opens the wt_fd and caches it in struct got_worktree,
> but otherwise doesn't use it. All the read_meta_file() calls etc. stay
> as they were (no need for sandbox protection).
>
> This should allow capsicum to access worktree->root_fd after
> got_open_worktree() is done, and configure appropriate rights on the fd.
>
> Once the work tree has been opened, and the repository fd and tmpfd have
> also been opened, the code can enter a capsicum sandbox.
>
> It looks to me like this could work.
> Perhaps I am missing something?
>
> I am suggesting this simply in an effort to keep the amount of required
> changes small. I'd be fine with the original diffs if my suggestions
> somehow conflict with what capsicum needs.
diff --git a/lib/fileindex.c b/lib/fileindex.c
index b46dbb29..6086d328 100644
--- a/lib/fileindex.c
+++ b/lib/fileindex.c
@@ -88,15 +88,15 @@ got_fileindex_perms_to_st(struct got_fileindex_entry *ie)
const struct got_error *
got_fileindex_entry_update(struct got_fileindex_entry *ie,
- const char *ondisk_path, uint8_t *blob_sha1, uint8_t *commit_sha1,
- int update_timestamps)
+ int wt_fd, const char *ondisk_path, uint8_t *blob_sha1,
+ uint8_t *commit_sha1, int update_timestamps)
{
struct stat sb;
- if (lstat(ondisk_path, &sb) != 0) {
+ if (fstatat(wt_fd, ondisk_path, &sb, AT_SYMLINK_NOFOLLOW) != 0) {
if (!((ie->flags & GOT_FILEIDX_F_NO_FILE_ON_DISK) &&
errno == ENOENT))
- return got_error_from_errno2("lstat", ondisk_path);
+ return got_error_from_errno2("fstatat", ondisk_path);
sb.st_mode = GOT_DEFAULT_FILE_MODE;
} else {
if (sb.st_mode & S_IFDIR)
diff --git a/lib/got_lib_fileindex.h b/lib/got_lib_fileindex.h
index f1005832..537374d7 100644
--- a/lib/got_lib_fileindex.h
+++ b/lib/got_lib_fileindex.h
@@ -108,7 +108,7 @@ uint16_t got_fileindex_perms_from_st(struct stat *);
mode_t got_fileindex_perms_to_st(struct got_fileindex_entry *);
const struct got_error *got_fileindex_entry_update(struct got_fileindex_entry *,
- const char *, uint8_t *, uint8_t *, int);
+ int, const char *, uint8_t *, uint8_t *, int);
const struct got_error *got_fileindex_entry_alloc(struct got_fileindex_entry **,
const char *);
void got_fileindex_entry_free(struct got_fileindex_entry *);
diff --git a/lib/got_lib_worktree.h b/lib/got_lib_worktree.h
index 067bac22..651bcc8f 100644
--- a/lib/got_lib_worktree.h
+++ b/lib/got_lib_worktree.h
@@ -17,6 +17,7 @@
struct got_worktree {
char *root_path;
char *repo_path;
+ int root_fd;
char *path_prefix;
struct got_object_id *base_commit_id;
char *head_ref_name;
diff --git a/lib/worktree.c b/lib/worktree.c
index 2e265b00..f4e65ab5 100644
--- a/lib/worktree.c
+++ b/lib/worktree.c
@@ -431,6 +431,12 @@ open_worktree(struct got_worktree **worktree, const char *path)
err = got_gotconfig_read(&(*worktree)->gotconfig,
(*worktree)->gotconfig_path);
+
+ (*worktree)->root_fd = open((*worktree)->root_path, O_DIRECTORY);
+ if ((*worktree)->root_fd == -1) {
+ err = got_error_from_errno2("open", (*worktree)->root_path);
+ goto done;
+ }
done:
if (repo)
got_repo_close(repo);
@@ -507,6 +513,7 @@ got_worktree_close(struct got_worktree *worktree)
free(worktree->gotconfig_path);
got_gotconfig_free(worktree->gotconfig);
free(worktree);
+ close(worktree->root_fd);
return err;
}
@@ -1179,7 +1186,7 @@ done:
static const struct got_error *
create_fileindex_entry(struct got_fileindex_entry **new_iep,
struct got_fileindex *fileindex, struct got_object_id *base_commit_id,
- const char *ondisk_path, const char *path, struct got_object_id *blob_id)
+ int wt_fd, const char *path, struct got_object_id *blob_id)
{
const struct got_error *err = NULL;
struct got_fileindex_entry *new_ie;
@@ -1190,7 +1197,7 @@ create_fileindex_entry(struct got_fileindex_entry **new_iep,
if (err)
return err;
- err = got_fileindex_entry_update(new_ie, ondisk_path,
+ err = got_fileindex_entry_update(new_ie, wt_fd, path,
blob_id->sha1, base_commit_id->sha1, 1);
if (err)
goto done;
@@ -1865,11 +1872,11 @@ done:
* we had to run a full content comparison to find out.
*/
static const struct got_error *
-sync_timestamps(char *ondisk_path, unsigned char status,
+sync_timestamps(int wt_fd, const char *path, unsigned char status,
struct got_fileindex_entry *ie, struct stat *sb)
{
if (status == GOT_STATUS_NO_CHANGE && stat_info_differs(ie, sb))
- return got_fileindex_entry_update(ie, ondisk_path,
+ return got_fileindex_entry_update(ie, wt_fd, path,
ie->blob_sha1, ie->commit_sha1, 1);
return NULL;
@@ -1922,7 +1929,8 @@ update_blob(struct got_worktree *worktree,
if (got_fileindex_entry_has_commit(ie) &&
memcmp(ie->commit_sha1, worktree->base_commit_id->sha1,
SHA1_DIGEST_LENGTH) == 0) {
- err = sync_timestamps(ondisk_path, status, ie, &sb);
+ err = sync_timestamps(worktree->root_fd,
+ path, status, ie, &sb);
if (err)
goto done;
err = (*progress_cb)(progress_arg, GOT_STATUS_EXISTS,
@@ -1932,7 +1940,8 @@ update_blob(struct got_worktree *worktree,
if (got_fileindex_entry_has_blob(ie) &&
memcmp(ie->blob_sha1, te->id.sha1,
SHA1_DIGEST_LENGTH) == 0) {
- err = sync_timestamps(ondisk_path, status, ie, &sb);
+ err = sync_timestamps(worktree->root_fd,
+ path, status, ie, &sb);
goto done;
}
}
@@ -1991,17 +2000,17 @@ update_blob(struct got_worktree *worktree,
* Otherwise, a future status walk would treat them as
* unmodified files again.
*/
- err = got_fileindex_entry_update(ie, ondisk_path,
+ err = got_fileindex_entry_update(ie, worktree->root_fd, path,
blob->id.sha1, worktree->base_commit_id->sha1,
update_timestamps);
} else if (status == GOT_STATUS_MODE_CHANGE) {
- err = got_fileindex_entry_update(ie, ondisk_path,
+ err = got_fileindex_entry_update(ie, worktree->root_fd, path,
blob->id.sha1, worktree->base_commit_id->sha1, 0);
} else if (status == GOT_STATUS_DELETE) {
err = (*progress_cb)(progress_arg, GOT_STATUS_MERGE, path);
if (err)
goto done;
- err = got_fileindex_entry_update(ie, ondisk_path,
+ err = got_fileindex_entry_update(ie, worktree->root_fd, path,
blob->id.sha1, worktree->base_commit_id->sha1, 0);
if (err)
goto done;
@@ -2024,11 +2033,12 @@ update_blob(struct got_worktree *worktree,
goto done;
if (ie) {
- err = got_fileindex_entry_update(ie, ondisk_path,
- blob->id.sha1, worktree->base_commit_id->sha1, 1);
+ err = got_fileindex_entry_update(ie,
+ worktree->root_fd, path, blob->id.sha1,
+ worktree->base_commit_id->sha1, 1);
} else {
err = create_fileindex_entry(&ie, fileindex,
- worktree->base_commit_id, ondisk_path, path,
+ worktree->base_commit_id, worktree->root_fd, path,
&blob->id);
}
if (err)
@@ -2128,8 +2138,8 @@ delete_blob(struct got_worktree *worktree, struct got_fileindex *fileindex,
* Preserve the working file and change the deleted blob's
* entry into a schedule-add entry.
*/
- err = got_fileindex_entry_update(ie, ondisk_path, NULL, NULL,
- 0);
+ err = got_fileindex_entry_update(ie, worktree->root_fd,
+ ie->path, NULL, NULL, 0);
} else {
err = (*progress_cb)(progress_arg, GOT_STATUS_DELETE, ie->path);
if (err)
@@ -2937,7 +2947,7 @@ merge_file_cb(void *arg, struct got_blob_object *blob1,
goto done;
if (status == GOT_STATUS_DELETE) {
err = got_fileindex_entry_update(ie,
- ondisk_path, blob2->id.sha1,
+ a->worktree->root_fd, path2, blob2->id.sha1,
a->worktree->base_commit_id->sha1, 0);
if (err)
goto done;
@@ -2959,8 +2969,8 @@ merge_file_cb(void *arg, struct got_blob_object *blob1,
err = got_fileindex_entry_alloc(&ie, path2);
if (err)
goto done;
- err = got_fileindex_entry_update(ie, ondisk_path,
- NULL, NULL, 1);
+ err = got_fileindex_entry_update(ie,
+ a->worktree->root_fd, path2, NULL, NULL, 1);
if (err) {
got_fileindex_entry_free(ie);
goto done;
@@ -3779,7 +3789,8 @@ schedule_addition(void *arg, unsigned char status, unsigned char staged_status,
err = got_fileindex_entry_alloc(&ie, relpath);
if (err)
goto done;
- err = got_fileindex_entry_update(ie, ondisk_path, NULL, NULL, 1);
+ err = got_fileindex_entry_update(ie, a->worktree->root_fd,
+ relpath, NULL, NULL, 1);
if (err) {
got_fileindex_entry_free(ie);
goto done;
@@ -4586,7 +4597,8 @@ revert_file(void *arg, unsigned char status, unsigned char staged_status,
if (status == GOT_STATUS_DELETE ||
status == GOT_STATUS_MODE_CHANGE) {
err = got_fileindex_entry_update(ie,
- ondisk_path, blob->id.sha1,
+ a->worktree->root_fd, relpath,
+ blob->id.sha1,
a->worktree->base_commit_id->sha1, 1);
if (err)
goto done;
@@ -5289,18 +5301,26 @@ done:
}
static const struct got_error *
-update_fileindex_after_commit(struct got_pathlist_head *commitable_paths,
- struct got_object_id *new_base_commit_id, struct got_fileindex *fileindex,
- int have_staged_files)
+update_fileindex_after_commit(struct got_worktree *worktree,
+ struct got_pathlist_head *commitable_paths,
+ struct got_object_id *new_base_commit_id,
+ struct got_fileindex *fileindex, int have_staged_files)
{
const struct got_error *err = NULL;
struct got_pathlist_entry *pe;
+ char *relpath = NULL;
TAILQ_FOREACH(pe, commitable_paths, entry) {
struct got_fileindex_entry *ie;
struct got_commitable *ct = pe->data;
ie = got_fileindex_entry_get(fileindex, pe->path, pe->path_len);
+
+ err = got_path_skip_common_ancestor(&relpath,
+ worktree->root_path, ct->ondisk_path);
+ if (err)
+ goto done;
+
if (ie) {
if (ct->status == GOT_STATUS_DELETE ||
ct->staged_status == GOT_STATUS_DELETE) {
@@ -5310,32 +5330,41 @@ update_fileindex_after_commit(struct got_pathlist_head *commitable_paths,
got_fileindex_entry_stage_set(ie,
GOT_FILEIDX_STAGE_NONE);
got_fileindex_entry_staged_filetype_set(ie, 0);
+
err = got_fileindex_entry_update(ie,
- ct->ondisk_path, ct->staged_blob_id->sha1,
+ worktree->root_fd, relpath,
+ ct->staged_blob_id->sha1,
new_base_commit_id->sha1,
!have_staged_files);
} else
err = got_fileindex_entry_update(ie,
- ct->ondisk_path, ct->blob_id->sha1,
+ worktree->root_fd, relpath,
+ ct->blob_id->sha1,
new_base_commit_id->sha1,
!have_staged_files);
} else {
err = got_fileindex_entry_alloc(&ie, pe->path);
if (err)
- break;
- err = got_fileindex_entry_update(ie, ct->ondisk_path,
- ct->blob_id->sha1, new_base_commit_id->sha1, 1);
+ goto done;
+ err = got_fileindex_entry_update(ie,
+ worktree->root_fd, relpath, ct->blob_id->sha1,
+ new_base_commit_id->sha1, 1);
if (err) {
got_fileindex_entry_free(ie);
- break;
+ goto done;
}
err = got_fileindex_entry_add(fileindex, ie);
if (err) {
got_fileindex_entry_free(ie);
- break;
+ goto done;
}
}
+ free(relpath);
+ relpath = NULL;
}
+done:
+ if (relpath)
+ free(relpath);
return err;
}
@@ -5666,8 +5695,8 @@ got_worktree_commit(struct got_object_id **new_commit_id,
if (err)
goto done;
- err = update_fileindex_after_commit(&commitable_paths, *new_commit_id,
- fileindex, have_staged_files);
+ err = update_fileindex_after_commit(worktree, &commitable_paths,
+ *new_commit_id, fileindex, have_staged_files);
sync_err = sync_fileindex(fileindex, fileindex_path);
if (sync_err && err == NULL)
err = sync_err;
@@ -6252,8 +6281,8 @@ rebase_commit(struct got_object_id **new_commit_id,
if (err)
goto done;
- err = update_fileindex_after_commit(&commitable_paths, *new_commit_id,
- fileindex, 0);
+ err = update_fileindex_after_commit(worktree, &commitable_paths,
+ *new_commit_id, fileindex, 0);
sync_err = sync_fileindex(fileindex, fileindex_path);
if (sync_err && err == NULL)
err = sync_err;
change got_worktree_init, open_worktree to use fds