On Sat, Mar 22, 2025 at 04:38:21AM +0100, Jann Horn wrote: > On Fri, Mar 21, 2025 at 9:41 PM Kees Cook <kees@xxxxxxxxxx> wrote: > > If __builtin_is_lvalue() is available, use it with __is_lvalue(). There > > is patch to Clang to provide this builtin now[1]. > > > > Link: https://github.com/kees/llvm-project/commits/builtin_is_lvalue/ [1] > > Signed-off-by: Kees Cook <kees@xxxxxxxxxx> > > Before you land that LLVM patch, it might be a good idea to figure out > how this interacts with the fun C quirk where you can have temporary > rvalues which can contain array members to which you can technically > create lvalues but must not write. As far as I understand, calling > "kfree(getS().ptrs[0])" as in the following example would cause UB > with your patch: Yay UB! I can confirm that currently isModifiableLvalue() does not catch this. > > ``` > $ cat kees-kfree-test.c > #include <stdlib.h> > > #define __is_lvalue(expr) __builtin_is_lvalue(expr) > > void __kfree(void *ptr); > void BEFORE_SET_TO_NULL(); > void AFTER_SET_TO_NULL(); > static inline void kfree_and_null(void **ptr) > { > __kfree(*ptr); > BEFORE_SET_TO_NULL(); > *ptr = NULL; > AFTER_SET_TO_NULL(); > } > #define __force_lvalue_expr(x) \ > __builtin_choose_expr(__is_lvalue(x), x, (void *){ NULL }) > #define __free_and_null(__how, x) \ > ({ \ > typeof(x) *__ptr = &(x); \ > __how ## _and_null((void **)__ptr); \ > }) > #define __free_and_maybe_null(__how, x) \ > __builtin_choose_expr(__is_lvalue(x), \ > __free_and_null(__how, __force_lvalue_expr(x)), \ > __kfree(x)) > #define kfree(x) __free_and_maybe_null(kfree, x) > > struct S { > void *ptrs[1]; > }; > struct S getS(void); > > int is_lvalue_test(void) { > return __is_lvalue(getS().ptrs[0]); > } > void testfn2(void) { > kfree(getS().ptrs[0]); > } > $ [...]/bin/clang-14 -c -o kees-kfree-test.o kees-kfree-test.c -O3 -Wall > $ objdump -d -Mintel -r kees-kfree-test.o > > kees-kfree-test.o: file format elf64-x86-64 > > > Disassembly of section .text: > > 0000000000000000 <is_lvalue_test>: > 0: b8 01 00 00 00 mov eax,0x1 > 5: c3 ret > 6: 66 2e 0f 1f 84 00 00 cs nop WORD PTR [rax+rax*1+0x0] > d: 00 00 00 > > 0000000000000010 <testfn2>: > 10: 50 push rax > 11: e8 00 00 00 00 call 16 <testfn2+0x6> > 12: R_X86_64_PLT32 getS-0x4 > 16: 48 89 c7 mov rdi,rax > 19: e8 00 00 00 00 call 1e <testfn2+0xe> > 1a: R_X86_64_PLT32 __kfree-0x4 > 1e: 31 c0 xor eax,eax > 20: e8 00 00 00 00 call 25 <testfn2+0x15> > 21: R_X86_64_PLT32 BEFORE_SET_TO_NULL-0x4 > 25: 31 c0 xor eax,eax > 27: 59 pop rcx > 28: e9 00 00 00 00 jmp 2d <testfn2+0x1d> > 29: R_X86_64_PLT32 AFTER_SET_TO_NULL-0x4 I don't see UB manifested here, though? It looks more like a dead store was eliminated (i.e. it was an automatic variable that wasn't going to be referenced outside of the expression statement). If I add a global and assign the global from *ptr, it all seems to work fine: --- kees-kfree-test.c.orig 2025-03-22 00:00:53.550633347 -0700 +++ kees-kfree-test.c 2025-03-21 23:58:57.124268268 -0700 @@ -2,6 +2,8 @@ #define __is_lvalue(expr) __builtin_is_modifiable_lvalue(expr) +void *g; + void __kfree(void *ptr); void BEFORE_SET_TO_NULL(); void AFTER_SET_TO_NULL(); @@ -11,6 +13,7 @@ BEFORE_SET_TO_NULL(); *ptr = NULL; AFTER_SET_TO_NULL(); + g = *ptr; } #define __force_lvalue_expr(x) \ __builtin_choose_expr(__is_lvalue(x), x, (void *){ NULL }) ... 27: e8 00 00 00 00 call 2c <testfn2+0x1c> 28: R_X86_64_PLT32 AFTER_SET_TO_NULL-0x4 2c: 48 c7 05 00 00 00 00 mov QWORD PTR [rip+0x0],0x0 # 37 <testfn2+0x27> 33: 00 00 00 00 > jannh@horn:~/test/kees-kfree$ > ``` > > As far as I understand, this causes UB in C99 ("If an attempt is made > to modify the result of a function call or to access it after the next > sequence point, the behavior is undefined.") and in C11 ("A non-lvalue > expression with structure or union type, where the structure or union > contains a member with array type (including, recursively, members of > all contained structures and unions) refers to an object with > automatic storage duration and temporary lifetime. 36) Its lifetime > begins when the expression is evaluated and its initial value is the > value of the expression. Its lifetime ends when the evaluation of the > containing full expression or full declarator ends. Any attempt to > modify an object with temporary lifetime results in undefined > behavior."). > > Basically, something like getS().ptrs[0] gives you something that is > technically an lvalue but must not actually be written to, and > ->isModifiableLvalue() does not catch that. But I agree, any mention of UB does give me pause! :) -- Kees Cook
