hadess / iio-sensor-proxy

IIO accelerometer sensor to input device proxy
197 stars 64 forks source link

Invalid scale values on unsigned data channels #238

Closed roadrunner2 closed 5 years ago

roadrunner2 commented 5 years ago

A combination of factors can cause unsigned values to be scaled to extremely small values. The relevant code snippets are (just looking at the buffered light, but is also applicable to the other buffered sensors):

process_scan (IIOSensorData data, DrvData *or_data)
{
        int i;
        int level = 0;
        gdouble scale;
        gboolean present_level;
        LightReadings readings;

        ...
        process_scan_1(data.data + or_data->buffer_data->scan_size*i, or_data->buffer_data, "in_intensity_both", &level, &scale, &present_level);

        g_debug ("Light read from IIO on '%s': %d (scale %lf) = %lf", or_data->name, level, scale, level * scale);
        readings.level = level;
        if (scale)
                readings.level *= scale;
       ...
}

process_scan_1 (char              *data,
                BufferDrvData        *buffer_data,
                const char        *ch_name,
                int               *ch_val,
                gdouble           *ch_scale,
                gboolean          *ch_present)
{
    ...
                        if (!buffer_data->channels[k]->is_signed) {
                                guint32 val = iio_readu32(buffer_data->channels[k], (guint8 *) data);
                                val = val >> buffer_data->channels[k]->shift;
                                if (buffer_data->channels[k]->bits_used < 32)
                                        val &= ((guint32) 1 << buffer_data->channels[k]->bits_used) - 1;
                                *ch_val = (int) val;
                                *ch_present = TRUE;
                        } else {
                                gint32 val = iio_read32(buffer_data->channels[k], (gint8 *) data);
                                val = val >> buffer_data->channels[k]->shift;
                                if (buffer_data->channels[k]->bits_used < 32)
                                        val &= ((guint32) 1 << buffer_data->channels[k]->bits_used) - 1;
                                val = (gint32) (val << (32 - buffer_data->channels[k]->bits_used)) >> (32 - buffer_data->channels[k]->bits_used);
                                *ch_val = (int) val;
                                if (buffer_data->channels[k]->scale)
                                        *ch_scale = buffer_data->channels[k]->scale;
                                else
                                        *ch_scale = 1.0;
                                *ch_present = TRUE;
                        }
    ...
}

If the channel is unsigned, then ch_scale is never assigned a value in process_scan_1(), and hence scale in process_scan() is not set; furthermore, in process_scan() scale is declared on the stack but not initialized. This results in the use of an uninitialized value later in process_scan(). Now usually the value will happen to be 0, but e.g. if compiled with -fstack-protector-strong then the value is actually a very small float (1.061e-314).

So, first of all scale needs to be initialized to 0 in process_scan(). Second, I don't understand why the scale is only set for unsigned data channels - I didn't see any reason why it wouldn't equally apply to signed ones.

I'd be happy to create pull requests for both of these, if desired.

roadrunner2 commented 5 years ago

I forgot to say: I was running into this problem on Fedora, where all the light values reported by iio-sensor-proxy were essentially zero. This is because Fedora builds iio-sensor-proxy (and probably most other packages) with -fstack-protector-strong, hence triggering this bug.

hadess commented 5 years ago

Could you please test the patches in https://github.com/hadess/iio-sensor-proxy/pull/239 ?

roadrunner2 commented 5 years ago

Thanks for the quick response. The patches look good and testing them showed them working well now.