From: Mark Jamsek <mark@jamsek.com>
Subject: Re: got-build-regress.sh regress failure
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Sat, 22 Apr 2023 03:41:02 +1000

No, I think you’re absolutely right on all counts. It’s not a good idea to change unveil for tests. Please disregard my suggestion, and ok for your diff.

Sent from my iPhone

> On 22 Apr 2023, at 3:05 am, Omar Polo <op@omarpolo.com> wrote:
> 
> 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");
> 
>