I Am Not Myself

Bills.Pay(Developer.Skills).ShouldBeTrue()

Sinister Bug is Sinister

Can you spot the bug in this flags enumeration?

    [Flags]
    public enum ItemCoreChangeType
    {
        ViewStyleChanged = 1,
        VaryByStructureChanged = 2,
        LayersChanged = 4,
        LayerContentsChanged = 8,
        PropertiesChanged = 16,
        ErrorsChanged = 32,
        ChangeHistoryCleared = 64,
        EditableCommentChanged = 128,
        NeedsValidation = 256,
        AffectsPropertyChangeHistory = 512,
        AffectsLayerChangeHistory = 1024,
        PushFormatters = 2048,
        ResetValueGroups = 4096,
        SkipChangeTrackingUpdate = 8092,
        StatusChanged = 16384,
        Flexible2DIndicesChanged = 32768,
    }

In case you can’t spot it with the naked eye (I couldn’t, coworker Alex caught it.), watch this video. Pay attention to the binary display as I enter in the values of my enumeration above. Notice how the bit is steadily walking down the display? What happens when I hit SkipChangeTrackingUpdate?

So it seems that when I use the SkipChangeTrackingUpdate value I am actually sending many of the other values in this flags enumeration. Nasty bug to track down eh?

What makes this bug so sinister is just how long it took to find. Using the blame command in git, I see the following result. The names have been changed to protect the guilty.

72fe38b4 (dirk.diggler 2010-02-10 23:49:28 -0800 33) SkipChangeTrackingUpdate = 8092,

So this particular flaw has been in the code base unnoticed for close to two years. Two years worth of code has been written with the assumption that SkipChangeTrackingUpdate actually sends the value of 9 different flag values. Fixing the flag value could potentially effect any part of that code.

What do?

Lucky for me, a search for that flag in the entire code base returned 3 usages, one of which was the definition of the flag, one was the usage of the flag and one consuming the flag.

Crisis averted but it could have been much worse.

3 responses to “Sinister Bug is Sinister

  1. David Thomas Garcia January 24, 2012 at 9:47 am

    That is a tough one to spot. I do my flag enums like this, that way I don’t have to input the values myself and I know none of them will overlap:

    [Flags]
    public enum ItemCoreChangeType {
    ViewStyleChanged = 1 << 0,
    VaryByStructureChanged = 1 << 1,
    LayersChanged = 1 << 2

    }

  2. Mark Simpson (@verdammelt) January 24, 2012 at 5:13 pm

    That is a good trick.

    And I have to, shamefully, admit that I still didn’t see the problem even when you mentioned which enum item it was – only when i watched the video – then looked back that the value on that item did i finally see it.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: