Warning when comparing address of function or variable with constant?

Hello all,

This morning I fixed a small bug at FreeBSD that involved the following
code:

  void
  func(struct foo *idx)
  {

    if (index == NULL)
      return;
    ...
  }

The bug in this code is that we should have compared against idx -- not
index. This works by accident, as index() is a function provided by our
C library (BSD's strchr()).

I think it is hardly ever possible that a function or variable ever
resides at address 0, except in kernelspace or when using a hacked
run-time linker. Does Clang have a warning for this? If not, would it be
nice to gain such a feature?

GCC (4.2) seems to support something like this, but doesn't do it
properly. The following code triggers a warning:

  if (index != 0)
    puts("Hi");

While this code does not:

  if (index != NULL)
    puts("Hi");

Essentially the compiler would be free to emit a warning for comparing
an address of a function or variable with any constant expression. As
things like address space randomisation become more prevalent, a fixed
address means nothing.

Hi,

Commenting on myself: a warning like that could have detected this
security bug, for example:

  http://blogs.oracle.com/alanc/entry/security_hole_in_xorg_6

PR 10169.

Joerg

Hello all,

This morning I fixed a small bug at FreeBSD that involved the following
code:

  void
  func(struct foo *idx)
  {

    if (index == NULL)
      return;
    ...
  }

The bug in this code is that we should have compared against idx -- not
index. This works by accident, as index() is a function provided by our
C library (BSD's strchr()).

I think it is hardly ever possible that a function or variable ever
resides at address 0, except in kernelspace or when using a hacked
run-time linker. Does Clang have a warning for this? If not, would it be
nice to gain such a feature?

I'm not aware of clang having this warning and I think it is very useful.

One complication is that the warning shouldn't fire for weak symbols,
those can be NULL and checking that is usually intentional.

- Ben

Hello all,

This morning I fixed a small bug at FreeBSD that involved the following
code:

  void
  func(struct foo *idx)
  {

    if (index == NULL)
      return;
    ...
  }

The bug in this code is that we should have compared against idx -- not
index. This works by accident, as index() is a function provided by our
C library (BSD's strchr()).

I think it is hardly ever possible that a function or variable ever
resides at address 0, except in kernelspace or when using a hacked
run-time linker. Does Clang have a warning for this? If not, would it be
nice to gain such a feature?

Note that a function can be NULL if it is declared weak on Darwin, which is something pretty common as it is used for backward compatibility.

GCC (4.2) seems to support something like this, but doesn't do it
properly. The following code triggers a warning:

  if (index != 0)
    puts("Hi");

While this code does not:

  if (index != NULL)
    puts("Hi");

Essentially the compiler would be free to emit a warning for comparing
an address of a function or variable with any constant expression. As
things like address space randomisation become more prevalent, a fixed
address means nothing.

--
Ed Schouten <ed@80386.nl>
WWW: http://80386.nl/
_______________________________________________
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

-- Jean-Daniel

Yes, -Wbool-conversions:

$ cat test.cc
int index();
void f(int* idx) {
  if (index)
    return;
}
$ ./build/bin/clang -fsyntax-only -Wbool-conversions test.cc
test.cc:3:7: warning: address of function 'index' will always evaluate
to 'true' [-Wbool-conversions]
  if (index)
      ^~~~~
test.cc:3:7: note: prefix with the address-of operator to silence this warning
  if (index)
      ^
      &
1 warning generated.

Cheers,
Matt

I think it's perfectly reasonable to extend this warning to also complain about comparisons against null pointers (with the typical caveat for weak declarations).

  - Doug

Oops, you're right, the current warning doesn't actually fire when the
address is used in a comparison. I agree, it should be extended to
cover comparison against NULL.

-Matt