Does the following Linux driver C code have an off by one error?

Question

I'm reading through a USB Wi-Fi card's C driver code and have come across a part I'm not sure I fully understand. I suspect it's my understanding of the C language and operator precedence that's wrong and that the driver code is fine, but I wanted to check.

In /drivers/net/wireless/rtl818x/rtl8187/dev.c is some code that reads a bunch of values into a 14 element channels array. The relevant code from dev.c is as follows:

    channel = priv->channels;
    for (i = 0; i < 3; i++) {
            eeprom_93cx6_read(&eeprom, RTL8187_EEPROM_TXPWR_CHAN_1 + i,
                              &txpwr);
            (*channel++).hw_value = txpwr & 0xFF;
            (*channel++).hw_value = txpwr >> 8;
    }
    for (i = 0; i < 2; i++) {
            eeprom_93cx6_read(&eeprom, RTL8187_EEPROM_TXPWR_CHAN_4 + i,
                              &txpwr);
            (*channel++).hw_value = txpwr & 0xFF;
            (*channel++).hw_value = txpwr >> 8;
    }

    ....


    if (!priv->is_rtl8187b) {
            for (i = 0; i < 2; i++) {
                    eeprom_93cx6_read(&eeprom,
                                      RTL8187_EEPROM_TXPWR_CHAN_6 + i,
                                      &txpwr);
                    (*channel++).hw_value = txpwr & 0xFF;
                    (*channel++).hw_value = txpwr >> 8;
            }
    } else {
            eeprom_93cx6_read(&eeprom, RTL8187_EEPROM_TXPWR_CHAN_6,
                              &txpwr);
            (*channel++).hw_value = txpwr & 0xFF;

            eeprom_93cx6_read(&eeprom, 0x0A, &txpwr);
            (*channel++).hw_value = txpwr & 0xFF;

            eeprom_93cx6_read(&eeprom, 0x1C, &txpwr);
            (*channel++).hw_value = txpwr & 0xFF;
            (*channel++).hw_value = txpwr >> 8;
    }

My concern with this code is that I would have thought the very first call to (*channel++).hw_value = ... would have incremented the channel pointer before dereferencing it, thereby starting at element [1] of channels and missing element [0]. Also, regardless of which of the if/else branches get executed, I count 14 calls to (*channel++)..., so I would have thought the final call to (*channel++) would actually be pointing at (non-existent) channel[15] and overwriting the memory of whatever variable happens to follow channels in the stack. Can anyone point out where I might have gone wrong in my interpretation?

The post-increment returns the old value of its operand. The first `*channel++` changes `channel` to point to the second element and dereferences the first one. See [++ on a dereferenced pointer in C?](http://stackoverflow.com/questions/859770/on-a-dereferenced-pointer-in-c). — DCoder, Sep 10 '12 at 04:50
I thought the post-increment operator had higher precedence than the dereference operator, or am I conflating the role of precedence with something else? — Bryce Thomas, Sep 10 '12 at 04:53
That is correct, but it doesn't disagree with what I said. Post-increment returns the original value, not the incremented one. — DCoder, Sep 10 '12 at 04:59
It's fascinating that someone who doesn't understand something so basic as post-decrement is reading kernel code. Start with baby steps. "am I conflating the role of precedence with something else?" -- You're not conflating anything, you're simply not understanding the semantics of the post-decrement operator; you're viewing it as identical to the pre-decrement operator. — Jim Balter, Sep 10 '12 at 05:48

score 1 · Answer 1 · answered Sep 10 '12 at 05:17

1

`*channel++`

Interpretation: 1) *channel i.e, value at address stored in channel is worked upon. 2) after semi - colon address of channel is incremented.

above are steps of post increment.

Hence,


    for(i=0;i

"CAN" simply mean if channel is already at position 0 (zero)



    channel[0].hw_value =xyz;
    channel++;

answered Sep 10 '12 at 05:17

rohank

81
1
9

'after semi - colon address of channel is incremented' -- actually it's before the semi-colon (or whatever is the next sequence point). e.g., `a = 1; b = (a++, a);` sets `b` to 2. – Jim Balter Sep 10 '12 at 06:00
C11 6.5.2.4 "The value computation of the result is sequenced before the side effect of updating the stored value of the operand.". So neither the next sequence point nor the semi-colon determines when it is updated. Though in practice the compiler is free to perform any number of optimizations as long as it is done before the next sequence point, so the value might not get updated before then, regardless of the text cited. – Lundin Sep 10 '12 at 06:45

score 0 · Answer 2 · answered Sep 10 '12 at 06:43

0

The operator precedence rules of C say that postfix operators have higher precedence than unary operators. Therefore the expression is equivalent to

*(channel++)

First it takes note that the pointer channel needs incrementing by 1 unit. Due to the nature of the postfix increment, this change doesn't take place until after the operand itself has been evaluated.

Next it takes the contents of the operand, which is still channel. It does something with the contents, then once that is done, the pointer gets incremented.

Now, to be picky, this code relies on undefined behavior, since the variable channel is modified twice without a sequence point in between. Strictly speaking the code is free to go haywire, but in practice all compilers implement this code in a deterministic way.

answered Sep 10 '12 at 06:43

Lundin

195,001
40
254
396

`the variable channel is modified twice without a sequence point in between` - sorry, where is this in the code? I see `channel` modified once, and where it points to modified, but I don't see `channel` modified 2x without a sequence point. Probably right in front of my eyes. I agree if that was the case, it would be undefined behavior. – Dan Sep 10 '12 at 12:21
@Dan Yes that is what I meant: channel++, channel is modified one time. *channel =, it is modified a second time. Both operations causes channel to point at a different kind of data. I suppose this is a matter of definition though, perhaps the C standard think that altering the contents and altering the address are two different things. If so, my answer might be incorrect, though in that case I would be interested to hear where in the standard it makes a difference between changing contents and address. I never write ++ one-liner goo like this myself so I haven't given it much thought. – Lundin Sep 10 '12 at 14:57

Does the following Linux driver C code have an off by one error?

2 Answers2