> On Jun 18, 2025, at 12:17, Siddhesh Poyarekar <siddh...@gotplt.org> wrote:
>
> On 2025-06-18 10:32, Qing Zhao wrote:
>>> A NULL check alone does not solve this problem because the pointer could be
>>> arbitrary and invalid. I thought some more about adding the size
>>> expression only if there's an existing access like I suggested in that
>>> email, but I realize that doesn't make sense either, since code that uses
>>> __bdos typically would look to the __bdos result to determine validity of
>>> the access.
>>>
>>> Can you see a way to validate the pointer in addition to the NULL check?
>> Right now, we only generate the size expression for the following cases:
>> 1. When the pointer is read as the first parameter of the
>> __builtin_dynamic_object_size (p, 0);
>> AND
>> 2. p != NULL.
>> Is the above 1 & 2 not enough to avoid the introduced undefined behavior?
>> I thought that the above 1 should validate the pointer already, let me know
>> if this is not the case.
>> If So, could you please provide me a small testing case that illustrates the
>> issue?
>
> I'm afraid not, a NULL pointer is not the only case where a pointer could be
> invalid. For example, the pointer could have been recently freed or even be
> an arbitrary non-NULL value due to a memory corruption. Lastly, the memory
> location could be protected (e.g. PROT_NONE) which makes reading it an
> invalid operation.
>
> For example if you have:
>
> struct fam_container
> {
> int a;
> int count;
> char fam[] __counted_by__ (count);
> };
>
> void
> baz (struct fam_container *ptr)
> {
> ...
> ptr->count = 0;
> __builtin_free (ptr);
> ...
> }
>
> void
> bar (size_t insz)
> {
> struct fam_container *c =
> __builtin_malloc (insz + sizeof (struct fam_container));
> c->count = insz;
> baz (c);
> foo (c);
> }
>
> void
> __attribute__ ((noinline))
> foo (struct fam_container *ptr)
> {
> ...
> objsz = __builtin_dynamic_object_size (ptr, 0);
> __memcpy_chk (ptr, src, sz, objsz);
> ...
> }
>
> The function foo would transform to something like:
>
> void
> foo (struct fam_container *ptr)
> {
> ...
> objsz = ptr != NULL ? ptr->count + sizeof (*ptr) : -1;
> __memcpy_chk (ptr, src, sz, objsz);
> }
>
> Here, ptr is not NULL and tree-object-size ends up generating a dereference
> of a freed pointer, thus generating new undefined behaviour. Earlier too the
> program was buggy, i.e. __memcpy_chk was being passed a freed PTR, but
> tree-object-size ends up adding yet another invalid access BEFORE that one.
>
> The example above is a simplification of how _FORTIFY_SOURCE works, i.e.
> __bdos is passed as an additional size argument to a checking function before
> the actual libc function is called. Essentially, the __bdos is supposed to
> protect access to PTR and instead, ends up adding another invalid access to
> PTR. Admittedly, _FORTIFY_SOURCE is unable to protect pointer corruption,
> but at least it should not generate a new invalid dereference.
>
> To demonstrate how it could make things objectively worse, consider this
> example:
>
> 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.
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?
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.
WDYT?
Thanks.
Qing
>
> Sid
