Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

__builtin_dynamic_object_size() fails to return correct size depending on depth of flexible array #110385

Closed
kees opened this issue Sep 28, 2024 · 6 comments · Fixed by #110497
Closed
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior clang:codegen

Comments

@kees
Copy link
Contributor

kees commented Sep 28, 2024

As seen in the Linux kernel:

we're having erroneous size reporting from __builtin_dynamic_object_size(), where the depth of dereference for the flexible array causes a 0 size report:

https://godbolt.org/z/qohGd5xh1


#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

int main(int argc, char *argv[])
{
        struct bucket *p;
        struct variable *v;

        p = malloc(sizeof(*p));
        v = malloc(sizeof(*p->growable) + sizeof(*p->growable->array) * 32);
        v->length = 32;


        printf("%zu\n", __builtin_dynamic_object_size(v->array, 1));

        p->growable = v;
        printf("%zu\n", __builtin_dynamic_object_size(p->growable->array, 1));

        return 0;
}


GCC shows 64 64, but Clang shows 64 0.

cc @isanbard @bwendling

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Sep 28, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 28, 2024

@llvm/issue-subscribers-clang-frontend

Author: Kees Cook (kees)

As seen in the Linux kernel:

we're having erroneous size reporting from __builtin_dynamic_object_size(), where the depth of dereference for the flexible array causes a 0 size report:

https://godbolt.org/z/qohGd5xh1


#include &lt;stdio.h&gt;
#include &lt;stdlib.h&gt;
#include &lt;unistd.h&gt;

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

int main(int argc, char *argv[])
{
        struct bucket *p;
        struct variable *v;

        p = malloc(sizeof(*p));
        v = malloc(sizeof(*p-&gt;growable) + sizeof(*p-&gt;growable-&gt;array) * 32);
        v-&gt;length = 32;


        printf("%zu\n", __builtin_dynamic_object_size(v-&gt;array, 1));

        p-&gt;growable = v;
        printf("%zu\n", __builtin_dynamic_object_size(p-&gt;growable-&gt;array, 1));

        return 0;
}


GCC shows 64 64, but Clang shows 64 0.

cc @isanbard @bwendling

@Cydox
Copy link
Contributor

Cydox commented Sep 28, 2024

I found a fix.

Haven't run the tests on it yet, but it compiles the kernel. Will open a PR tomorrow:

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 9166db4c7412..143dd3fcfcf8 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1164,15 +1164,15 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
     Res = EmitDeclRefLValue(DRE).getPointer(*this);
     Res = Builder.CreateAlignedLoad(ConvertType(DRE->getType()), Res,
                                     getPointerAlign(), "dre.load");
-  } else if (const MemberExpr *ME = dyn_cast<MemberExpr>(StructBase)) {
-    LValue LV = EmitMemberExpr(ME);
-    Address Addr = LV.getAddress();
-    Res = Addr.emitRawPointer(*this);
   } else if (StructBase->getType()->isPointerType()) {
     LValueBaseInfo BaseInfo;
     TBAAAccessInfo TBAAInfo;
     Address Addr = EmitPointerWithAlignment(StructBase, &BaseInfo, &TBAAInfo);
     Res = Addr.emitRawPointer(*this);
+  } else if (const MemberExpr *ME = dyn_cast<MemberExpr>(StructBase)) {
+    LValue LV = EmitMemberExpr(ME);
+    Address Addr = LV.getAddress();
+    Res = Addr.emitRawPointer(*this);
   } else {
     return nullptr;
   }

Cydox added a commit to Cydox/llvm-project that referenced this issue Sep 29, 2024
Fixes llvm#110385

Fix counted_by attribute for cases where the flexible array member is accessed
through struct pointer inside another struct:

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a
pointer, it is treated as a pointer. Otherwise clang will generate to
code to access the address of p->growable intead of loading the value of
p->growable->length.
Cydox added a commit to Cydox/llvm-project that referenced this issue Sep 29, 2024
Fixes llvm#110385

Fix counted_by attribute for cases where the flexible array member is accessed
through struct pointer inside another struct:

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a
pointer, it is treated as a pointer. Otherwise clang will generate to
code to access the address of p->growable intead of loading the value of
p->growable->length.
@bwendling
Copy link
Collaborator

@Cydox, I have a fix that I'm planning on submitting first.

@bwendling bwendling self-assigned this Sep 30, 2024
@bwendling bwendling added the bug Indicates an unexpected problem or unintended behavior label Sep 30, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 30, 2024

@llvm/issue-subscribers-bug

Author: Kees Cook (kees)

As seen in the Linux kernel:

we're having erroneous size reporting from __builtin_dynamic_object_size(), where the depth of dereference for the flexible array causes a 0 size report:

https://godbolt.org/z/qohGd5xh1


#include &lt;stdio.h&gt;
#include &lt;stdlib.h&gt;
#include &lt;unistd.h&gt;

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

int main(int argc, char *argv[])
{
        struct bucket *p;
        struct variable *v;

        p = malloc(sizeof(*p));
        v = malloc(sizeof(*p-&gt;growable) + sizeof(*p-&gt;growable-&gt;array) * 32);
        v-&gt;length = 32;


        printf("%zu\n", __builtin_dynamic_object_size(v-&gt;array, 1));

        p-&gt;growable = v;
        printf("%zu\n", __builtin_dynamic_object_size(p-&gt;growable-&gt;array, 1));

        return 0;
}


GCC shows 64 64, but Clang shows 64 0.

cc @isanbard @bwendling

@bwendling
Copy link
Collaborator

See #110487 for a proposed fix. (One liner even!)

@bwendling bwendling added this to the LLVM 19.X Release milestone Sep 30, 2024
Cydox added a commit to Cydox/llvm-project that referenced this issue Sep 30, 2024
Fixes llvm#110385

Fix counted_by attribute for cases where the flexible array member is accessed
through struct pointer inside another struct:

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a
pointer, it is treated as a pointer. Otherwise clang will generate to
code to access the address of p->growable intead of loading the value of
p->growable->length.
@EugeneZelenko EugeneZelenko added clang:codegen and removed clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 3, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 3, 2024

@llvm/issue-subscribers-clang-codegen

Author: Kees Cook (kees)

As seen in the Linux kernel:

we're having erroneous size reporting from __builtin_dynamic_object_size(), where the depth of dereference for the flexible array causes a 0 size report:

https://godbolt.org/z/qohGd5xh1


#include &lt;stdio.h&gt;
#include &lt;stdlib.h&gt;
#include &lt;unistd.h&gt;

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

int main(int argc, char *argv[])
{
        struct bucket *p;
        struct variable *v;

        p = malloc(sizeof(*p));
        v = malloc(sizeof(*p-&gt;growable) + sizeof(*p-&gt;growable-&gt;array) * 32);
        v-&gt;length = 32;


        printf("%zu\n", __builtin_dynamic_object_size(v-&gt;array, 1));

        p-&gt;growable = v;
        printf("%zu\n", __builtin_dynamic_object_size(p-&gt;growable-&gt;array, 1));

        return 0;
}


GCC shows 64 64, but Clang shows 64 0.

cc @isanbard @bwendling

xgupta pushed a commit to xgupta/llvm-project that referenced this issue Oct 4, 2024
Fix counted_by attribute for cases where the flexible array member is
accessed through struct pointer inside another struct:

```
struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};
```

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a
pointer, it is treated as a pointer. Otherwise clang will generate to
code to access the address of p->growable intead of loading the value of
p->growable->length.

Fixes llvm#110385
bwendling pushed a commit to bwendling/llvm-project that referenced this issue Oct 7, 2024
Fix counted_by attribute for cases where the flexible array member is
accessed through struct pointer inside another struct:

```
struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};
```

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a
pointer, it is treated as a pointer. Otherwise clang will generate to
code to access the address of p->growable intead of loading the value of
p->growable->length.

Fixes llvm#110385
tru pushed a commit to bwendling/llvm-project that referenced this issue Oct 11, 2024
Fix counted_by attribute for cases where the flexible array member is
accessed through struct pointer inside another struct:

```
struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};
```

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a
pointer, it is treated as a pointer. Otherwise clang will generate to
code to access the address of p->growable intead of loading the value of
p->growable->length.

Fixes llvm#110385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment