From: Omar Polo <op@omarpolo.com>
Subject: Re: got-build-regress.sh regress failure
To: Mark Jamsek <mark@jamsek.com>
Cc: gameoftrees@openbsd.org
Date: Fri, 21 Apr 2023 19:05:31 +0200

On 2023/04/22 02:37:03 +1000, Mark Jamsek <mark@jamsek.com> wrote:
> On 23-04-21 05:40PM, Omar Polo wrote:
> > On 2023/04/21 17:11:46 +0200, Omar Polo <op@omarpolo.com> wrote:
> > > ==== run-regress-fetch_test ====
> > > /home/chiaki/w/got/regress/fetch/obj/fetch_test -q
> > > 
> > > ===> regress/tog
> > > ==== log ====
> > > ./log.sh -q -r "/home/chiaki/tmp/"
> > > tog: fopen: /home/chiaki/tmp/tog-test-log_hsplit_diff-eLI9p4Ou/view: No such file or directory
> > 
> > It fails because the file pointed in the TOG_SCR_DUMP environment
> > variable is not permitted by unveil(), but since the default
> > GOT_TEST_ROOT dir is /tmp which is unveiled this wasn't noticed
> > before.
> > 
> > Here's a possible way to fix it: open the screendump earlier and keep
> > it open.  I've added a ftruncate call in screendump() just in case it
> > gets called multiple times.
> > 
> > With this all tests are passing again on amd64 when ran as
> > 
> > 	$ mkdir -p ~/tmp/got-tmp
> > 	$ make -C regress/tog GOT_TEST_ROOT=~/tmp/got-tmp
> 
> Could we unveil GOT_TEST_ROOT if it's set and it's a test run?

I think it's better to have a pledge/unveil policy during regress as
close as possible as the one we have during normal operation to help
catching bugs.

When I first implemented 'got patch' I was violating unveil(2), but
never realized (until stsp' regress ran) because I was working
entirely in /tmp ^^"

So, I'd advise against unveilling $GOT_TEST_ROOT.

one suggestion below

> OTOH, it's unveiling another directory albeit only for tests. Either
> way, I'm ok with your diff too.
> 
> diff /home/mark/src/got
> commit - 69b9e75f5436338a9b5cddd5b8462929def12e8c
> path + /home/mark/src/got
> blob - 69803386ef3f054281b09871e9eb299e4c663e2b
> file + tog/tog.c
> --- tog/tog.c
> +++ tog/tog.c
> @@ -4159,6 +4159,14 @@ apply_unveil(const char *repo_path, const char *worktr
>  {
>  	const struct got_error *error;
>  
> +	if (getenv("TOG_TEST_SCRIPT") != NULL) {
> +		const char *got_test_root;
> +
> +		got_test_root = getenv("GOT_TEST_ROOT");
> +		if (got_test_root && unveil(got_test_root, "rwc") != 0)

we could just unveil TOG_SCR_DUMP here if you prefer.

FWIW I still prefer the opening early (plus eventual ftruncate).  stsp
suggested on irc to use got_opentemp_truncate() (which beside the name
has nothing to do with temp files, it's a "safe" wrapper around
ftruncate(2) for FILE*s) and I have it in my tree.

If you want to unveil $TOG_SCR_DUMP it's ok for me, otherwise I can
commit my diff with stsp' tweak.  Your call :)

> +			return got_error_from_errno2("unveil", got_test_root);
> +	}
> +
>  #ifdef PROFILE
>  	if (unveil("gmon.out", "rwc") != 0)
>  		return got_error_from_errno2("unveil", "gmon.out");