Message-ID: <55d3008a-a11a-406a-ac3b-2016466384b8@gmail.com>
Date: Sun, 26 May 2024 19:57:27 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Fix passing fcntl and ioctl variadic arguments to file system
 extension API
Content-Language: en-US
From: "J.W. Jagersma (jwjagersma AT gmail DOT com) [via djgpp AT delorie DOT com]" <djgpp AT delorie DOT com>
To: djgpp AT delorie DOT com, Pali <pali AT pali DOT im>
References: <20240509182019 DOT mrbfcyv4jhl3w34x AT pali> <867cg03azy DOT fsf AT gnu DOT org>
 <b97a8b35-4a67-4b75-8e18-45211e7afda7 AT gmail DOT com>
In-Reply-To: <b97a8b35-4a67-4b75-8e18-45211e7afda7@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Reply-To: djgpp AT delorie DOT com

On 2024-05-11 17:11, J.W. Jagersma wrote:
> On 2024-05-11 12:21, Eli Zaretskii (eliz AT gnu DOT org) [via djgpp AT delorie DOT com] wrote:
>>> Date: Thu, 9 May 2024 20:20:19 +0200
>>> From: Pali <pali AT pali DOT im>
>>> Cc: djgpp AT delorie DOT com
>>>
>>> Hello. I have there a change which fixes ioctl and fcntl functions when
>>> file descriptor uses file system extension API. These two functions are
>>> takes variadic arguments and file system extension API takes va_list
>>> structure with all corresponding arguments. djgpp code currently do not
>>> pass all arguments to API functions __FSEXT_ioctl and __FSEXT_fcntl.
>>>
>>> My fix, which is below, uses __builtin_apply_args() which returns
>>> pointer to va_list-compatible structure of all arguments (including
>>> non-variadic). It works for djgpp i386 calling convention. It does not
>>> work on e.g. AMD64, but this is not a djgpp case which is 32-bit x86.
>>> For using __builtin_apply_args, it was required to manually inline
>>> __FSEXT_func_wrapper function as it takes variadic arguments and not
>>> va_list structure as the "func" itself.
> 
> I (still) can't see Pali's message but this sounds similar to a patch I
> sent in last year:
> 
> https://www.delorie.com/djgpp/mail-archives/browse.cgi?p=djgpp/2023/06/28/12:14:52
> 
> At the moment, fsext-hooked ioctl() and fnctl() are useless and always
> invoke UB (entirely random effects), so I do think this is a
> high-priority bug.
> 
> Linked patch is technically UB too (sometimes reading past the end of va_args),
> but in that case, there is (should be) no code path where the va_args are used.
> 
> However I wasn't aware of __builtin_apply_args().  Whatever Pali's
> solution looks like, it is probably nicer than mine :)
> 

Getting back to this (and replying to myself)...

From the gcc manual, I noticed there is no mention that
__builtin_apply_args() must return something compatible (or even
similar) to va_list.  It's a neat trick, that may happen to work now on
i386 with stack-based parameters, but there are no guarantees about the
layout of this magic structure.  So, it could break again in the future.

It is really only meant to be used with __builtin_apply(), which also
requires somehow knowing the va_list length beforehand.  And we need to
pass additional arguments to the fsext handler too, so this is just not
practical to use.

Since as far as I can tell, all unix-style fcntl()/ioctl() operations
take at most 1 extra argument (always int/pointer sized), I think my
earlier 15-minute hack may still just be the best solution.  Reading one
element past the end of a va_list is probably not so disastrous - it's
not the sort of "undefined behavior" that the compiler can produce
invalid code for.