5 False positive in "Dereference of null pointer" due to lack of handling of unions

[Quuxplusone]

Bugzilla Link	PR9332
Status	CONFIRMED
Importance	P normal
Reported by	Amit Kulkarni (amitkulz@gmail.com)
Reported on	2011-02-25 23:21:54 -0800
Last modified on	2011-04-05 17:12:14 -0700
Version	trunk
Hardware	Sun OpenBSD
CC	kremenek@apple.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments	report-8Sjxxe.html (638864 bytes, text/html) report-p5Ikh3.html (639421 bytes, text/html) clang_tcp_input.i.pre (279139 bytes, application/octet-stream)
Blocks
Blocked by
See also

Created attachment 6242
tcp_input.c null deref 1

clang version 2.9 (trunk 126522)

AMD64, OpenBSD current

5 False positives in

http://www.openbsd.org/cgi-bin/cvsweb/~checkout~/src/sys/netinet/tcp_input.c?rev=1.240;content-type=text%2Fplain

in function syn_cache_respond()

Basically, once sc->sc_src.sa.sa_family is selected to be either case AF_INET
or case AF_INET6, the other case "ip header" variables which are initially
uninitialized are found to be null.

initially
struct ip *ip = NULL;
struct ip6_hdr *ip6 = NULL;

Clang arbitrarily switches case (i.e first it takes case IP4 and then
***still*** inside function it takes case IPv6,) inside this function
syn_cache_respond(), and finds the other variables as deference to null
pointers i.e if initially it was case AF_INET, ip6 is found to be NULL.

either case AF_INET or case AF_INET6 is impossible to switch once you have
entered the atomic function syn_cache_respond().

Please look at attachment to easily figure out the problem, looking at the
switch(sc->sc_src.sa.sa_family). There are 4 more attachments all in this same
function. Can't add multiple attachments?

[Quuxplusone]

Attached report-8Sjxxe.html (638864 bytes, text/html): tcp_input.c null deref 1

[Quuxplusone]

Attached report-p5Ikh3.html (639421 bytes, text/html): another false positive

[Quuxplusone]

_This bug has been marked as a duplicate of bug 6150_

[Quuxplusone]

Why was this marked a duplicate? Please don't dup static analyzer radars without justification or diagnosis. While false positives may seem similar, they can often be from different causes. If there is reason to suspect this is the same issue, that should be documented in the PR.

[Quuxplusone]

cloned to rdar://problem/9057893

[Quuxplusone]

It is the same IMHO, i.e when clang goes inside a function, and then a constant
is given differing values by clang switching around.

But I get your point, it might be processed in different files or a different
section of the file.

Sorry for that. Btw, this is the cause of a number of false positives when
analyzing userland of OpenBSD, and it looks to be old cause. Please look if
possible before 2.9 lock.

Thanks for you time

[Quuxplusone]

I need an actual preprocessed file to diagnose this issue, not the html bug reports.

[Quuxplusone]

Attached clang_tcp_input.i.pre (279139 bytes, application/octet-stream): preprocessed output of tcp_input.c

[Quuxplusone]

Full command line used to generate preprocessed output.

Note, it does not include -Wstack-larger-than-2047 because of clang error.

As you can see switch (sc->sc_src.sa.sa_family) has two cases: case 2 and case 24 for TCP version 4 and TCP version 6 respectively.

clang -Werror -Wall -Wstrict-prototypes -Wmissing-prototypes -Wno-main -Wno-uninitialized -Wno-format -mcmodel=kernel -mno-red-zone -mno-sse2 -mno-sse -mno-3dnow -mno-mmx -msoft-float -fno-omit-frame-pointer -fno-builtin-printf -fno-builtin-snprintf -fno-builtin-vsnprintf -fno-builtin-log -fno-builtin-log2 -fno-builtin-malloc -O2 -pipe -nostdinc -I. -I../../../.. -I../../../../arch -DDDB -DDIAGNOSTIC -DKTRACE -DACCOUNTING -DKMEMSTATS -DPTRACE -DPOOL_DEBUG -DCRYPTO -DSYSVMSG -DSYSVSEM -DSYSVSHM -DUVM_SWAP_ENCRYPT -DCOMPAT_43 -DCOMPAT_O47 -DLKM -DFFS -DFFS2 -DFFS_SOFTUPDATES -DUFS_DIRHASH -DQUOTA -DEXT2FS -DMFS -DNNPFS -DNFSCLIENT -DNFSSERVER -DCD9660 -DUDF -DMSDOSFS -DFIFO -DSOCKET_SPLICE -DTCP_SACK -DTCP_ECN -DTCP_SIGNATURE -DINET -DALTQ -DINET6 -DIPSEC -DPPP_BSDCOMP -DPPP_DEFLATE -DMROUTING -DMPLS -DBOOT_CONFIG -DUSER_PCICONF -DAPERTURE -DMTRR -DNTFS -DPCIVERBOSE -DUSBVERBOSE -DWSDISPLAY_COMPAT_USL -DWSDISPLAY_COMPAT_RAWKBD -DWSDISPLAY_DEFAULTSCREENS="6" -DWSDISPLAY_COMPAT_PCVT -DX86EMU -DONEWIREVERBOSE -DMULTIPROCESSOR -DMAXUSERS=80 -D_KERNEL -E ../../../../netinet/tcp_input.c > clang_tcp_input.i.pre

[Quuxplusone]

I'm looking at the first case, in tcp_sack_option().  What I see:

            p = cur;
...
            while (cur) // take false branch; both p and cur are null
...
            p->next = ...  // p is null

The control-flow logic is rather confusing here, so there may be some control-
dependencies the analyzer is not handling, but it's not clear why this is a
false positive.

I will say the analyzer cannot handle reason about:

   (sack.start)-(cur->start)  > 0

The issue here is that both variables have symbolic values, and we aren't
tracking linear constraints (yet) between symbolic values.  That's covered by
PR 4550.

[Quuxplusone]

The second issue is in tcp_mss_adv:

int
tcp_mss_adv(struct ifnet *ifp, int af)
{
  int mss = 0;
  int iphlen;

  switch (af) {   // ANALYZER : take default branch
    case 2:
      if (ifp != 0L)
        mss = ifp->if_data.ifi_mtu;
      iphlen = sizeof(struct ip);
      break;

    case 24:
      if (ifp != 0L)
        mss = (((((struct in6_ifextra *)(ifp)->if_afdata[24])->nd_ifinfo)->linkmtu && (((struct in6_ifextra *)(ifp)->if_afdata[24])->nd_ifinfo)->linkmtu < (ifp)->if_data.ifi_mtu) ? (((struct in6_ifextra *)(ifp)->if_afdata[24])->nd_ifinfo)->linkmtu : (((((struct in6_ifextra *)(ifp)->if_afdata[24])->nd_ifinfo)->maxmtu && (((struct in6_ifextra *)(ifp)->if_afdata[24])->nd_ifinfo)->maxmtu < (ifp)->if_data.ifi_mtu) ? (((struct in6_ifextra *)(ifp)->if_afdata[24])->nd_ifinfo)->maxmtu : (ifp)->if_data.ifi_mtu));
      iphlen = sizeof(struct ip6_hdr);
      break;

  }
  mss = mss - iphlen - sizeof(struct tcphdr);  // ANALYZER: iphlen is still uninitialized
  return (max(mss, tcp_mssdflt));
}

Why is this a false positive?  The analyzer doesn't know that the only possible
cases are are '2' and '24' (if that is the case).  To document that (if that is
the case), add an assertion, e.g.:

  default:
     assert(0 && "cannot be reached);

The analyzer will prune the path at the assert() call.

[Quuxplusone]

In 'syn_cache_add()', the branches involved are:

  if (optp || (tp->t_flags & 0x0400)) {
...
  switch (src->sa_family) {
   ..
  default:
       ipopts = 0L;
...
  if ((sc = syn_cache_lookup(src, dst, &scp, ((struct inpcb *)(so)->so_pcb)->inp_rtableid))
      != 0L) {
...
    if (ipopts) {
...
    sc->sc_timestamp = tb.ts_recent;   // ERROR

While the analyzer isn't being clever about 'tp->t_flags & 0x400', I don't see
any logic here that the analyzer should understand to establish any control-
dependencies between these branches.  Looks like a legit warning to me.  If
there is a control-dependency, you'll need to add an assertion.

[Quuxplusone]

(In reply to comment #0)
> Created an attachment (

[Quuxplusone]

(In reply to comment #0)
>
> Please look at attachment to easily figure out the problem, looking at the
> switch(sc->sc_src.sa.sa_family). There are 4 more attachments all in this same
> function. Can't add multiple attachments?

Looks like the constraint on 'sa_family' isn't getting tracked because the
symbolic store is returning a value of "unknown" for the load in the switch
statement.

This like the analyzer's current lack of reasoning for 'unions'.

[Quuxplusone]

(In reply to comment #13)
> (In reply to comment #0)
> >
> > Please look at attachment to easily figure out the problem, looking at the
> > switch(sc->sc_src.sa.sa_family). There are 4 more attachments all in this
same
> > function. Can't add multiple attachments?
>
> Looks like the constraint on 'sa_family' isn't getting tracked because the
> symbolic store is returning a value of "unknown" for the load in the switch
> statement.
>
> This like the analyzer's current lack of reasoning for 'unions'.

More specifically:

struct syn_cache {
  <SNIP>

  union syn_cache_sa sc_src;  // <-- union

  <SNIP>

};

[Quuxplusone]

Hi Ted,

Thanks for your time in investigating this. And hoping for a fix!

--amit