Possible value overflow in lego_sensor_default_scale for S32 and S32_BE type

[bmegli]

I am not sure if this is the right place to discuss code. Still it's better to have it kept as issue.

This is of low priority, I believe that all current sensor implementations should behave correctly (for various reasons).

lego_sensor_default_scale in https://github.com/ev3dev/lego-linux-drivers/blob/master/sensors/lego_sensor_class.c

fragment:

    if (mode_info->raw_min != mode_info->si_min
            || mode_info->raw_max != mode_info->si_max) {
        *value = (*value - mode_info->raw_min)
            * (mode_info->si_max - mode_info->si_min)
            / (mode_info->raw_max - mode_info->raw_min)
            + mode_info->si_min;
    }

The value may overflow for (example):

raw_min=INT_MIN
raw_max=INT_MAX
si_min=-100 //or whatever, different from INT_MIN
si_max=100 //or whatever > 1, diffrent from INT_MAX

Example of isolated code:

struct lego_sensor_mode_info {
    int raw_min;
    int raw_max;
    int si_min;
    int si_max;
};

void lego_sensor_default_scale(struct lego_sensor_mode_info *mode_info, long int *value)
{

    if (mode_info->raw_min != mode_info->si_min
            || mode_info->raw_max != mode_info->si_max) {
        *value = (*value - mode_info->raw_min) //e.g. for value 1  and raw_min INT_MIN we get INT_MAX here
            * (mode_info->si_max - mode_info->si_min) //this is where we can blow up
            / (mode_info->raw_max - mode_info->raw_min)
            + mode_info->si_min;
    }
}

int main(int argc, char **argv)
{
    struct lego_sensor_mode_info mode_info={INT32_MIN, INT32_MAX, -100, 100};

    long int val=1;
    lego_sensor_default_scale(&mode_info, &val);
    printf("%ld\n", val); //2147450880!
        return 0;
}

Expected result: somewhere between -100 and 100 Result: 2147450880

Possible simple fix: make scaling on long long values but this requires fairly new gcc (gcc doesn't emit long long assembler instructions in arm toolchains up to some version from what I remember)

[dlech]

I would prefer to use s64 instead of long long. Also, you will need to use do_div for the division (this divides 64 bit value by 32 bit value).

Something like this maybe?

void lego_sensor_default_scale(struct lego_sensor_mode_info *mode_info, s32 *value)
{
        if (mode_info->raw_min != mode_info->si_min
                    || mode_info->raw_max != mode_info->si_max) {
                s64 tmp;

                tmp = *value - mode_info->raw_min;
                tmp *= mode_info->si_max - mode_info->si_min;
                do_div(tmp, mode_info->raw_max - mode_info->raw_min);
                *value = (s32)tmp + mode_info->si_min;
        }
}

[bmegli]

Something along the line but with additional casts and div may be harder.

All of those can also overflow INT32: value-raw_min (e.g even. 0-INT_MIN oveflows) si_max-si_min raw_max-raw_min (e.g. INT_MAX-INT_MIN > 2*INT-MAX)

So it's not that simple after all. I will think tommorow.

I believe all sensor code I have seen with S32 type is unnaffected now (it's ok).

[bmegli]

This should be safe.

We need explicit conversion for at least one argument in each operation since every line could overflow. Also div64_s64 for division, divisor can also be > 2 INT_MAX

    if (mode_info->raw_min != mode_info->si_min
            || mode_info->raw_max != mode_info->si_max) {
        s64 tmp = (s64)*value - mode_info->raw_min;
        tmp *= (s64)mode_info->si_max - mode_info->si_min;
        tmp=div64_s64(tmp, (s64)mode_info->raw_max - mode_info->raw_min);
        *value=(s32)(tmp + mode_info->si_min);      
    }

But it is also slower than your original code. I am not sure if you would want it, all the code as-is-now is ok and only needs one to be carefull when adding sensor.

[dlech]

I don't think the extra time is a big deal. Values are only scaled when you actually read the valueN attribute. It's not like this is being run many times in the background.

[dlech]

Another possibility is to put this is a new function and assign it to only the sensors that need it. There is already code in place for custom scaling functions.