> On Jun 18, 2025, at 15:50, Siddhesh Poyarekar <siddh...@gotplt.org> wrote:
>
> On 2025-06-18 15:14, Qing Zhao wrote:
>>> struct fam_container
>>> {
>>> int a;
>>> int count;
>>> char fam[] __counted_by__ (count);
>>> };
>>>
>>> size_t
>>> baz (struct fam_container *ptr)
>>> {
>>> ...
>>> ptr->count = 0;
>>> __builtin_free (ptr);
>>> ...
>>> return 0;
>>> }
>>>
>>> void
>>> bar (size_t insz)
>>> {
>>> struct fam_container *c =
>>> __builtin_malloc (insz + sizeof (struct fam_container));
>>> c->count = insz;
>>> sz = baz (c);
>>> foo (c, sz);
>>> }
>>>
>>> void
>>> __attribute__ ((noinline))
>>> foo (struct fam_container *ptr, size_t sz)
>>> {
>>> ...
>>> objsz = __builtin_dynamic_object_size (ptr, 0);
>>> __memcpy_chk (ptr, src, sz, objsz);
>>> ...
>>> }
>>>
>>> Because SZ is 0, in practice there would have been no dereference of PTR in
>>> __memcpy_chk because the underlying memcpy would have not dereferenced PTR.
>>> Here, _FORTIFY_SOURCE ends up actually introducing an invalid dereference
>>> through the OBJSZ argument.
>>>
>>> I hope that clarifies what I'm trying to say.
>> Yes, Now I see what did you mean from the above example.
>> However, I think that the use-after-free error in the source code should be
>> caught by a separate tool -fsanitize=address. (And I just checked the above
>> small testing case with -fsanitize=address, Yes, the use-after-free error is
>> detected during run-time). It’s not the job of
>> __builtin_dynamic_object_size to catch such errors.
>
> I agree, validating invalid pointers is not the job of __bdos. My concern is
> about having __bdos *generate* code that results in invalid pointer access.
>
>> It should be reasonable to assume that in
>> __builtin_dynamic_object_size (ptr, 0);
>> ptr is NULL or point to a valid memory.
>> (I think that we might need clarify in the documentation of __bos and
>> __bdos, if an invalid pointer is passed to these builtins, the behavior is
>> undefined)
>> Forcing compiler optimization or other sanitizers always check whether every
>> pointer reference might be use-after-free is not a reasonable requirement.
>> For example:
>> [opc@qinzhao-ol8u3-x86 ~]$ cat tt.c
>> struct fam_container
>> {
>> char fam[10];
>> int count;
>> };
>> void __attribute__ ((noinline))
>> baz (struct fam_container *ptr)
>> {
>> ptr->count = 0;
>> __builtin_free (ptr);
>> return;
>> }
>> void
>> __attribute__ ((noinline))
>> foo (struct fam_container *ptr)
>> {
>> __builtin_printf ("size of fam is %u \n”,
>> __builtin_dynamic_object_size (ptr->fam, 1));
>> }
>> void
>> bar ()
>> {
>> struct fam_container *c =
>> __builtin_malloc (sizeof (struct fam_container));
>> baz (c);
>> foo (c);
>> }
>> int main()
>> {
>> bar ();
>> return 0;
>> }
>> [opc@qinzhao-ol8u3-x86 ~]$ gcc -O2 tt.c;./a.out
>> size of fam is 10
>> In the above, we can see clearly that “ptr” is invalid when passed to
>> __builtin_dynamic_object_size,
>> However, the current __builtin_dynamic_object_size didn’t check such invalid
>> ptr and on the other hand, it
>> Incorrectly reports the size of the invalid pointer is 10. Is this wrong?
>
> It's not wrong, because __bdos does not validate a pointer. They key
> difference from the example I cited though, is that in this case the size
> expression does not *create* a dereference of a pointer. With this patch,
> __bdos will start doing that, which is the risky proposition.
Then even the current implementation for using counted_by information for FAM
should check the invalid pointer?
Right now, for a slightly modified testing case for the above tt.c (change the
fixed size array “fam[10]” to a flexible array member with counted_by):
[opc@qinzhao-ol8u3-x86 ~]$ cat ttt.c
struct fam_container
{
int count;
char fam[] __attribute__ ((counted_by (count)));
};
void
__attribute__ ((noinline))
baz (struct fam_container *ptr)
{
ptr->count = 0;
__builtin_free (ptr);
}
void
__attribute__ ((noinline))
foo (struct fam_container *ptr)
{
__builtin_printf ("size of fam is %u \n",
__builtin_dynamic_object_size (ptr->fam, 1));
}
void
bar (unsigned insz)
{
struct fam_container *c =
__builtin_malloc (insz + sizeof (struct fam_container));
c->count = insz;
baz (c);
foo (c);
}
int main()
{
bar (10);
return 0;
}
[opc@qinzhao-ol8u3-x86 ~]$ gcc -O2 ttt.c;./a.out
size of fam is 6847
In the above example, when ptr->fam is passed to __builtin_dynamic_object_size,
we assume that “ptr” should be a valid pointer and should not be NULL,
therefore, ptr->count is generated to represent the size of the array.
However, now, even when ptr->fam is available in the source code, we cannot
assume that the “ptr” is valid, is such assumption too conservative?
I still think that "checking the pointer is valid” should separate from
__bdos/bos.
>
>> I think that for such behavior, we should clarify in the documentation of
>> __bdos and __bos that:
>> if an invalid pointer is passed to these builtins, __bdos and __bos does
>> check for this. the behavior is undefined.
>> This should resolve the issue.
The above had a typo, the clarification in DOC should be:
"if an invalid pointer is passed to these builtins, __bdos and __bos does NOT
check for this. the behavior of the __bdos/__bos for an invalid pointer
passed to it is undefined.”
>
> If we say that, it could indeed give us the ability to dereference without
> validating even for NULL,
We should check for NULL for the current patch since NULL is a valid value for
a pointer.
When ptr->fam is passed to __bdos in the source code as following: (the
situation we already handled in __bdos)
__builtin_dynamic_object_size (ptr->fam, 1));
“ptr" is guaranteed not a NULL. So, we don’t need to check for NULL when
generating prt->count to represent the size;
However, when ptr is passed to __bdos as the following: (as the current patch
try to handle)
__builtin_dynamic_object_size (ptr, 1));
“ptr" could be a NULL since NULL is a valid pointer value. If ptr is NULL,
generating “ptr->count” to represent the size
will introduce NEW undefined behavior into the program, which is wrong behavior
we should avoid. Therefore, when
__bdos only see the pointer “ptr”, not “ptr->fam”, we should check “ptr” is NOT
NULL first before generating “ptr->count”.
> although we'll still need it to avoid a case where passes later assume
> validity of the pointer based on the dereference __bdos generated. However
> I'm not sure if it's a net positive because of the second example I shared
> (the SZ = 0 one); there's a real chance that we introduce an active
> vulnerability in buggy code that was at least not vulnerable.
As long as we don’t introduce NEW undefined behavior code, I think we are doing
correctly.
Qing
>
> Sid
