Back

@kees but what if the calling code wants to use the pointer value they just freed for something else? Maybe they use it as a key in some other structure and they now need to go remove that entry. By setting their passed lvalue to null won’t you break their code? Obviously an edge case, but just pointing out this trick isn’t universally safe. Obviously very useful most of the time however.

@pauldoo @kees yeah that was my thought as well. Shouldn't the compiler or static checkers be able to do more here? Check that it's unused after kfree() or not dereferenced after kfree() etc.

Also after the Linus reply on kmalloc_obj() seems he's very much against hidden side effects so maybe should have been cc'd here too :/

@vbabka @pauldoo That's a fair point -- it's another hidden assignment...

Perhaps make kfree() return NULL and mark it __must_check, and tree wide replacement:

ptr = kfree(ptr);

? Leave __kfree as void?

It's much more painful though, since there are so so many non-lvalue kfree uses.

@kees @vbabka @pauldoo you could do a treewide replacement of kfree() with something called KFREE() or such, and then at least the uppercasing will hint to everyone that this is a macro doing magic macro things?

@fanf @pauldoo Is there anything in GCC or Clang that does this? I didn't find a function attribute that would have that effect. It'd be a nice diagnostic. The "malloc" attribute has a "deallocator" argument, but it doesn't seem to imply variable uninitialized-ness. :(

@kees @pauldoo i don’t think any compilers currently do so, but there is some very risky stuff going on around realloc and allocation size tracking, so i expect future compiler releases to do more and more exciting things with undefined behaviour and (de)allocation functions … it worries me, so i wanted to warn against using pointers after free() - and it’s another reason that it’s a good idea to nullify them

@fanf @kees @pauldoo for extra irony, I try to be sure and nullify pointers for more or less these reasons, and what I've learned is that cov-scan hates it.

Because it's a dead store if you get it right, and it flags it as a maintenance issue. fml.

@thesamesam @kees @fanf @pauldoo @pinskia there's a tangentially related case where programs would assume that if realloc returned the same pointer value, they could continue using the old pointer variable without refreshing it (thus violating the invalidated pointer limitation in the standard), which stumbled on _FORTIFY_SOURCE because the compiler couldn't see the new size.

I would say it's safest to always assume that a freed pointer is unusable the moment it is freed.

@thesamesam @kees @fanf @pauldoo @pinskia to Kees' question, the compiler does have some diagnostics (use-after-free and some analyzer heuristics IIRC) that use the deallocator pairing to emit warnings, but it doesn't flag the freed pointers as "uninitialised".

@vegard Oh that's very clever!

#define __is_lvalue(x) __builtin_constant_p((x) == (x))

This doesn't pass the generalized corner cases, but it does seem to work for the much smaller subset of existing kfree() arguments:

https://godbolt.org/z/oMEaK56Ts

(It still chokes on "(void *)var", which I'm hoping to fix with the built-in.)

@kees This is fine, right? https://godbolt.org/z/GEGxrhE83

@vegard Actual LOL. I mean ... it DOES work. That took me a second to parse! And here, this solves the "const" problem too. And interestingly detects variables that were _made_ constant:

https://godbolt.org/z/qv1sj67Mb

I think we can get the "main" misdetection fixed with __builtin_object_size() too, but tricks with __builtin_choose_expr() are needed. I gotta run...