Function sscanf must be assigned to variable otherwise strange behavior

Question

Consider this code:

#define TRANSLATOR_requestElectricityMeterWrite()  do{addr = word_getAddress(); value = word_getValue(); }while(0)

uint16_t value;
uint8_t addr;

bool dispatcher(void)
{
    TRANSLATOR_requestElectricityMeterWrite(); 
    return true;
} // AFTER this point (during debug) program goes to default handler

int main(void)
{
   if(dispatcher())
      continue;
      . . . .
      . . . . 
}

uint16_t word_getValue(void)
{
    uint16_t value;
    sscanf("ABCD", "%4x", (unsigned int *)&value);
    return value;
}

uint8_t word_getAddress(void)
{
    uint8_t address;
    sscanf("00", "%2x", (unsigned int *)&address);
        ;
    return address;
}

When the code above is run, the statement inside if causes program to crash(goes to some default handler).

But when I change the two(word_getValue and word_getAddres) functions to this:

uint16_t word_getValue(void)
{
    uint16_t value;
    int i = 0;i++;
    i = sscanf(WORD_getValueString(), "%4x", (unsigned int *)(&value));
    return value;
}

uint8_t word_getAddress(void)
{
    uint8_t address;
    int i = 0;i++;
    i = sscanf(WORD_getNameString(), "%2x", (unsigned int *)(&address));
    return address;
}

It works. The addition if the dummy i seems to solve that problem. But why doesn't it work the other way?

GNU ARM v4.8.3 toolchain

Where are `WORD_getValueString()` and `WORD_getNameString()` defined? It seems that you are showing to us a different version of your code. — David Ranieri, Aug 12 '16 at 12:41
In another file. But their declarations are included. Neither warnings nor Errors were reported on build. — Hairi, Aug 12 '16 at 12:44
I suggest to post a [Minimal, Complete, and Verifiable code](http://stackoverflow.com/help/mcve) — David Ranieri, Aug 12 '16 at 12:45
`continue` outside loop. Your compiler should warn. Why does it not? Did you enable at least the recommended warnings? What do you mean with "crash"? Which "default handler" is this? What do the exception registers show? — too honest for this site, Aug 12 '16 at 16:01

score 3 · Accepted Answer · answered Aug 12 '16 at 12:52

Both functions invoke undefined behavior, hence anything can happen. Adding an extra local variable changes the location of the destination variable, hiding the effect of its incorrect size.

sscanf("ABCD", "%4x", (unsigned int *)&value);

sscanf will store sizeof(unsigned int) bytes (probably 4) into variable value, which has only 2 bytes.

sscanf(WORD_getNameString(), "%2x", (unsigned int *)(&address));

Will store sizeof(unsigned int) bytes into variable address, which has only 1 byte.

The easiest way to fix this problem is to parse into an unsigned int and store the parsed value to the destination separately, or simply return the value:

uint16_t word_getValue(void) {
    unsigned int value;
    if (sscanf(WORD_getValueString(), "%4x", &value) == 1)
        return value;
    // could not parse a value, return some default value or error code
    return 0;
}

uint8_t word_getAddress(void) {
    unsigned int address;
    if (sscanf(WORD_getNameString(), "%2x", &address) == 1)
        return address;
    // could not parse a value, return some default value or error code
    return 0;
}

You might also want to verify if the parsed value is within range for the destination type, but since you limit the parse to respectively 4 and 2 hex digits, overflow cannot happen.

Does the return types `uint8_t` and `uint16_t` explicitly cast the returned value? Because now I return `unsigned int` which on my platform is `uint32_t`. — Hairi, Aug 12 '16 at 13:12
The `unsigned int` values returned by these functions are implicitly converted to the `uint8_t` and `uint16_t` return types respectively. The conversion is fully defined, the values are truncated and given how they are computed by `sscanf`, they are within the range of the return type. — chqrlie, Aug 12 '16 at 13:29
Great, so the example provided by you does perfect job. I have to admit that I was very frustrated by the behavior of the board I am debugging. This is a good example for me and other newbies of the C language dangers, especially when memory is involved. 10x :) @chqrlie — Hairi, Aug 12 '16 at 14:00
@Hairi: The conversion is well defined, but can still lead to problems. Enable conversion warnings (and all other relevant warnings). If you get such a warning, add an explicit cast to silence the compiler **only iff** you are 200% sure the conversion will be corect **under all circumstances** (as always when you add a cast). — too honest for this site, Aug 13 '16 at 18:11

score 1 · Answer 2 · answered Aug 12 '16 at 12:47

1

%x format requires unsigned argument (suppose it's uint32_t on your platform). If you pass uint16_t or uint8_t it can corrupt memory. In your case it corrupt stack and overwrites return address. Try use %4hx for uint16_t and %2hhx for uint8_t.

answered Aug 12 '16 at 12:47

Moncruist

36
2

1

`%4hx` assumes the destination is an `unsigned short`. Although it is probably the case, it is still technically risky as we don't know if `uint16_t` is the same type as `unsigned short`. – chqrlie Aug 12 '16 at 12:59

Function sscanf must be assigned to variable otherwise strange behavior

2 Answers2