On 2025-06-18 16:20, Qing Zhao wrote:
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.
__bdos(ptr->fam) is different from __bdos(ptr). In case of
__bdos(ptr->fam), the dereference of PTR has already happened before the
__bdos call, so the generated ptr->count does not introduce a new
dereference, i.e. not a potentially new introduction of undefined
behaviour. In case of __bdos(ptr) though, the code didn't have a
dereference until the __bdos gets expanded to 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.
That's the problem, I think we do end up introducing new undefined
behaviour in some cases, like the SZ=0 example I shared earlier.
Sid
