This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Detect when function pointer is freed
ClosedPublic

Authored by AndersRonnholm on Apr 4 2017, 12:42 AM.

Details

Summary

The MallocChecker does not currently detect freeing of function pointers.

Example code.

void (*fnptr)(int);
void freeIndirectFunctionPtr() {
  void *p = (void*)fnptr;
  free(p); // expected-warning {{Argument to free() points to a function pointer}}
}

Diff Detail

Repository
rL LLVM

Event Timeline

AndersRonnholm created this revision.Apr 4 2017, 12:42 AM
NoQ edited edge metadata.Apr 4 2017, 1:06 AM

Hello, thanks for the patch!

Because we already warn on freeing concrete function pointers, eg.

void foo() {
  free(&foo);
}

this is a useful addition.

Is freeing function pointers always undefined? I wonder what happens if we take some JIT-enabled javascript engine, maybe with some on-stack replacement of theirs, it may malloc() a memory and use it as a function, and then eventually it'd need to free it by design. However, because we're analyzing a small part of the program, we may fail to see in the analyzer that the symbolic pointer originally comes from malloc(). Would such rare but important users be able to avoid/suppress the warning?

test/Analysis/malloc.c
1780 ↗(On Diff #94012)

Nope, it doesn't point to a function pointer. It's still the same function pointer.
Also, freeing a pointer to a function pointer is not a bug.

In D31650#717691, @NoQ wrote:

Is freeing function pointers always undefined?

I guess not.. however I don't personally see why it would be useful to allocate function pointers with malloc.

I wonder what happens if we take some JIT-enabled javascript engine, maybe with some on-stack replacement of theirs, it may malloc() a memory and use it as a function, and then eventually it'd need to free it by design. However, because we're analyzing a small part of the program, we may fail to see in the analyzer that the symbolic pointer originally comes from malloc(). Would such rare but important users be able to avoid/suppress the warning?

Maybe when writing JIT there is some usecase, I don't know. The code could be rewritten like:

void *malloc(unsigned long);
void free(void*);

typedef void (*fnptr)(int);

void allocatedFunctionPointer() {
  void *p = malloc(sizeof(fnptr));
  fnptr p2 = (fnptr)p;
  free(p);
}

no warning is written about this code.

void *p = malloc(sizeof(fnptr));

sorry ... I guess that should be something like "void *p = malloc(100);"

Updated after comments

AndersRonnholm marked an inline comment as done.Apr 20 2017, 4:16 AM
NoQ accepted this revision.Apr 27 2017, 9:05 AM

Looks good, thanks!

Did you evaluate this on a large codebase - were warnings plentiful and were there any false positives known?

I'd like to summon Anna here for a little bit because that's a new check that is enabled by default, so it's always a bit of a historical moment:

  • Does the warning message sound reasonable? (it does to me).
  • Should we keep it as part of the unix.Malloc checker package, or should we be able to enable it separately?
This revision is now accepted and ready to land.Apr 27 2017, 9:05 AM
zaks.anna accepted this revision.Apr 28 2017, 10:11 AM
This revision was automatically updated to reflect the committed changes.