static analyzer false positive due to logic error

Hi,

I have a case where the static analyzer produced a false positive report of a null pointer dereference. I am reporting this because it seems to indicate an error in the logic of the checker. It has assumed two different values for the same variable at different points along the path is used to generate the report.

I have copied the relevant section of the HTML report below. At step 1 it assumes Cept_configuration is not null. At step 15 it assumes the same variable is now null (i.e. in contradiction to its previous assumption). This variable is not modified by the code between these two points. The reported error at step 18 is a direct result of the incorrect assumption at step 15.

!

1659

! ; // If no configuration defined then driver status is unknowable.

1660

// Therefore, don’t bother to do any reads/writes of the signal bits.

1661

if(Cept_configuration == NU_NULL || !Cept_configu ration->globals.cept)

1

Assuming ‘Cept_configuration’ is not equal to null

e$B"*e(B

2

e$B"+e(B

Taking false branch

e$B"*e(B

1662

return;

1663

1664

// Read all the signalling bits.

1665

debounced = Cept_Read_Signalling_Bits(fiber, &sbits_in, &driver_status);

1666

1667

// Grab the lock that protects globals before accessing it.

1668

NUX_Lock(&Cept_lock, NU_SUSPEND);

1669

1670

#if CEPT_DEBOUNCING

1671

// If we are debouncing the input, then don’t update the CEPT driver

1672

// status (i.e., skip the rest of this function).

1673

if(old_driver_status >= NU_FALSE || !debounced)

3

e$B"+e(B

Assuming ‘old_driver_status’ is >= 0

e$B"*e(B

1674

{

1675

#endif

1676

// Update the link LED (even if! f this is not part of a CEPT loop).

1677

if(driver_status)

4

e$B"+e(B

Assuming ‘driver_status’ is not equal to 0

e$B"*e(B

5

e$B"+e(B

Taking true branch

e$B"*e(B

1678

CEPT_link_okay_led = DSP__LED_ON, CEPT_driver_status! = NU_TRUE, CEPT_driver = sbits_in.dcc_id;

1679

else

1680

CEPT_link_okay_led = DSP__LED_OFF, CEPT_driver_status = NU_FALSE, CEPT_driver = 0;

1681

1682

// Update the user interface (because status has changed).

1683

&nbs! p; if(old_driver_status != CEPT_driver_status || old_driver != CEPT_driver)

6

e$B"+e(B

Assuming ‘old_driver_status’ is equal to ‘CEPT_driver_status’

e$B"*e(B

7

e$B"+e(B

Taking false branch

e$B"*e(B

1684

(void) NU_Set_Events(&UI_events, UI_CEPT_CHANGED, NU_OR);

1685

1686

! / / Log the change of CEPT driver status.

1687

if(old_driver_status != CEPT_driver_status)

8

e$B"+e(B

Taking false branch

e$B"*e(B

1688

{

1689

if(CEPT_driver_status)

1690

LOG_CEPT_LINK_HEALED(C ept_myid, CEPT_driver);

1691

else

1692

LOG_CEPT_LINK_FAILED(Cept_myid);

1693

}

1694

1695

// We have to set our CEPT identifier so it gets output during send.

1696

1697

1698

// Note that this is not used but should be set for backwards compatibility.

1699

sbits_out->loop_status = (INT8) -1;

1700

1701

// If no configuration ! loaded therefore no way to update outgoing bits.

1702

if(Cept_myid != 0 &&

9

e$B"+e(B

Assuming ‘Cept_myid’ is not equal to 0

e$B! "*e(B

10

e$B"+e(B

Assuming ‘CEPT_driver’ is not equal to 0

e$B"*e(B

11

e$B"+e(B

Taking true branch

e$B"*e(B

1703

{

1704

// No change in driver status (from above). Determine loop status.

1705

// Have to check if the CEPT is looped back onto itself and this

1706

// will be considered a loop failur e (but not a link failure).

1707

if(CEPT_driver_status && CEPT_driver != Cept_myid)

12

e$B"+e(B

Taking true branch

e$B"*e(B!

1708

{

1709

// Check if this DCC is (still) the loop master.

1710

! if(sbits_in.loop_id == Cept_myid)

13

e$B"+e(B

Taking true branch

e$B"*e(B

1711

{

1712

// Save the new loop identifier (us, same as old).

1713

1714

1715

// We have looped back to ourselves so indicate this.

1716

sbits_out->wrapped_around = (UINT8) -1;

1717

}

1718

// Check if we are going to be the new loop master.

1719

else if(sbits_in.loop_id > Cept_myid)

1720

{

1721

// Start new CEPT loop status bits.

1722

sbits_out->loop_id = Cept_myid;

1723

sbits_out->wrapped_around = (UINT8) 0;

1724

}

1725

// Unconfigured driver so assume we will be master.

1726

else if(sbits_in.loop_id == 0)

1727

{

1728

// Noone is cept master so we will try to start the loop.

1729

&nb! sp; sbits_out->loop_id = Cept_myid;

1730

sbits_out->wrapped_around = (UINT8) 0;

1731

}

1732

! // Another DCC is the loop master (lower numbered).

1733

else! < o:p>

1734

{

1735

// Someone else is the CEPT master.

1736

&n bsp; sbits_out->loop_id = sbits_in.loop_id;

1737

1738

// Transfer CEPT loop status from the CEPT master.

1739

&n! bsp; sbits_out->wrapped_around = sbits_in.wrapped_around;

1740

}

174! 1

}

1742

// CEPT driver is not working.

1743

else

1744

{

1745

&n bsp; // Update the CEPT status bits when CEPT driver not working.

1746

sbits_out->loop_id = Cept_myid;

1747

sbits_out->wrapped_around = (UINT8) 0;

1748

}

1749

}

1750

// If not configured or no driver.

1751

else

1752

{

1753

// Update the CEPT status bits when CEPT driver not working.

1754

sbits_out->loop_id = Cept_myid;

1755

sbits_out->wrapped_around = (UINT8) 0;

1756

}

1757

1758

// Read the CEPT loop status and master (inherited from previous DCC).

1759

CEPT_status = sbits_out->wrapped_around ? NU_TRUE : NU_FALSE;

14

e$B"+e(B

‘?’ condition is true

e$B"*e(B

1760!

1761

// Set the CEPT status iff CEPT loop is complete.

1762

if(Cept_configuration != NU_NULL && CEPT_status)

15

e$B"+e(B

Assuming ‘Cept_configuration’ is equal to null

e$B"*e(B

1763

CEPT_master = sbits_out->loop_id;

1764

1765

// If CEPT status changes then do some stuff. If no change do nothing.

1766

if(old_loop_status != CEPT_status || old_master != CEPT_master)

16

e$B"+e(B

Assuming ‘old_loop_status’ is equal to ‘CEPT_status’

e$B"*e(B

17

e$B"+e(B

Taking false branch

e$B"*e(B

1767

{

1768

// Update the user interf! ace (because status has changed).

1769

(void) NU_Set_Events

1770

1771

// If the CEPT is dead, disconnect its users.

1772

if(!CEPT_status)

1773

Cept_Teardown();

1774

// S! etup any users (i.e., music) now that cept is alive.

1775

else

1776

Cept_Setup();

1777

1778

// Only log iff there is a configuration and this is part of a loop.

1779

if(Cept_configuration->globals.cept)

1780

1781

// Log the CEPT heal/fail.

1782

if(CEPT_status)

1783

LOG_CEPT_LOOP_HEALED(Cept_myid, CEPT_master);

1784

else

1785

&! nbsp; LOG_CEPT_LOOP_FAILED(Cept_myid);

1786

}

1787

}

1788

1789

// Set the loop LED based upon the loop status iff part of CEPT loop.

1790

if(CEPT_status && Cept_configuration->globals.cept)

18

e$B"+e(B

Dereference of null pointer

Hi, Dennis. Your HTML got pretty mangled in my mail client, but I would guess your problem is that Cept_configuration is a global variable. Because of this, the analyzer can't actually be sure that none of the functions you call between step 1 and step 15 aren't going to modify it. It looks like you can't just make the global variable const, so you'll probably have to introduce a local variable to let the analyzer know what's going on.

We've had reports like this before, but until we have some notion of function summaries / whole-program analysis, we're not going to be able to do much here. And that's a big project, so...well, don't hold your breath.

Jordan