Different behaviour (local vs remote) when an iio attribute read causes buffer overflow #357

dNechita · 2020-01-28T08:22:14Z

Suppose one wants to read an iio attribute but doesn't provide enough space for the buffer where the content of the attribute will be read:

when local backend is used, the read operation is successful and the content of the attribute is truncated to fit the size of the buffer
when a backend that communicates with IIOD is used (e.g. usb, ethernet), the read operation fails with ERROR: Input/output error (-5).

This inconsistency has been found while investigating bug: #268

rgetz · 2020-01-28T13:35:14Z

Which is the expectation/intended algorithm?(@pcercuei ?)

the local backend should return the same error code,
the network should return a partial buffer?

If I look at other things...

ssize_t read(int fd, void *buf, size_t count);
size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);

On success, the number of bytes read is returned (zero indicates end of file).

the iio_*_read functions are like:

@return On success, the number of bytes written to the buffer

There is no mention of zero indicates buffer is full, and more data is there... ??

pcercuei · 2020-01-28T16:20:07Z

While it should be easy to support returning a partial buffer, I believe the behaviour should be an error code, since the attribute is not a stream (two consecutive reads will return the same data from the beginning of the attribute file) and we have no way to tell that we're getting a partial buffer.

Since this is not really ABI right now (as the behaviour is different across backends) maybe we can change it to -EFBIG, which would be a bit more descriptive and easier to discern versus the other communication errors.

rgetz · 2020-01-29T13:45:36Z

Is the error:

Error	`/usr/include/asm-generic/errno-base.h`	libiio meaning
EFBIG	File too large	The Attribute is too large to put in the provided buffer
ENOSPC	No space left on device	The provided buffer is too small to fit the entire attribute

We already use EFBIG in local.c:local_get_buffer:
https://github.com/analogdevicesinc/libiio/blob/master/local.c#L513

As long as it means the same thing in all places - it should be fine...

Thanks
-Robin

rgetz · 2020-07-31T17:06:04Z

We have other issues with usb...

causing this error, but settingBUF_SIZE to 256

diff --git a/tests/iio_common.h b/tests/iio_common.h
index 58a98e1..b517e9c 100644
--- a/tests/iio_common.h
+++ b/tests/iio_common.h
@@ -29,7 +29,7 @@
  * coming back from the kernel. Because of virtual memory,
  * only the amount of ram that is needed is used.
  */
-#define BUF_SIZE 16384
+#define BUF_SIZE 256
 
 enum backend {
        IIO_LOCAL,

with ip context, we get the error (gain_table_config is 1688 chars), and everything else works as expected (we end up with a :ERROR: Input/output error (-5)); which we will likely/eventualy change to an IIO specific Target buffer too small error).

rgetz@brain:~/github/libiio/build$ ./tests/iio_attr -u ip:192.168.2.1 -d ad9361-phy
dev 'ad9361-phy', attr 'calib_mode', value :'auto'
dev 'ad9361-phy', attr 'calib_mode_available', value :'auto manual manual_tx_quad tx_quad rf_dc_offs rssi_gain_step'
dev 'ad9361-phy', attr 'dcxo_tune_coarse', value :ERROR: No such device (-19)
dev 'ad9361-phy', attr 'dcxo_tune_coarse_available', value :'[0 0 0]'
dev 'ad9361-phy', attr 'dcxo_tune_fine', value :ERROR: No such device (-19)
dev 'ad9361-phy', attr 'dcxo_tune_fine_available', value :'[0 0 0]'
dev 'ad9361-phy', attr 'ensm_mode', value :'fdd'
dev 'ad9361-phy', attr 'ensm_mode_available', value :'sleep wait alert fdd pinctrl pinctrl_fdd_indep'
dev 'ad9361-phy', attr 'filter_fir_config', value :'FIR Rx: 0,0 Tx: 0,0'
dev 'ad9361-phy', attr 'gain_table_config', value :ERROR: Input/output error (-5)
dev 'ad9361-phy', attr 'multichip_sync', value :ERROR: Permission denied (-13)
dev 'ad9361-phy', attr 'rssi_gain_step_error', value :'lna_error: 0 0 0 0
mixer_error: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
gain_step_calib_reg_val: 0 0 0 0 0'
dev 'ad9361-phy', attr 'rx_path_rates', value :'BBPLL:983040003 ADC:245760000 R2:122880000 R1:61440000 RF:30720000 RXSAMP:30720000'
dev 'ad9361-phy', attr 'trx_rate_governor', value :'nominal'
dev 'ad9361-phy', attr 'trx_rate_governor_available', value :'nominal highest_osr'
dev 'ad9361-phy', attr 'tx_path_rates', value :'BBPLL:983040003 DAC:122880000 T2:122880000 T1:61440000 TF:30720000 TXSAMP:30720000'
dev 'ad9361-phy', attr 'xo_correction', value :'39999743'
dev 'ad9361-phy', attr 'xo_correction_available', value :'[39991744 1 40007742]'

with the usb context, after the error - the context is un-usable.

rgetz@brain:~/github/libiio/build$ ./tests/iio_attr -a usb -d ad9361-phy
Using auto-detected IIO context at URI "usb:3.44.5"
dev 'ad9361-phy', attr 'calib_mode', value :'auto'
dev 'ad9361-phy', attr 'calib_mode_available', value :'auto manual manual_tx_quad tx_quad rf_dc_offs rssi_gain_step'
dev 'ad9361-phy', attr 'dcxo_tune_coarse', value :ERROR: No such device (-19)
dev 'ad9361-phy', attr 'dcxo_tune_coarse_available', value :'[0 0 0]'
dev 'ad9361-phy', attr 'dcxo_tune_fine', value :ERROR: No such device (-19)
dev 'ad9361-phy', attr 'dcxo_tune_fine_available', value :'[0 0 0]'
dev 'ad9361-phy', attr 'ensm_mode', value :'fdd'
dev 'ad9361-phy', attr 'ensm_mode_available', value :'sleep wait alert fdd pinctrl pinctrl_fdd_indep'
dev 'ad9361-phy', attr 'filter_fir_config', value :'FIR Rx: 0,0 Tx: 0,0'
dev 'ad9361-phy', attr 'gain_table_config', value :ERROR: Input/output error (-5)
dev 'ad9361-phy', attr 'multichip_sync', value :ERROR: Connection timed out (-110)
dev 'ad9361-phy', attr 'rssi_gain_step_error', value :ERROR: Connection timed out (-110)
dev 'ad9361-phy', attr 'rx_path_rates', value :ERROR: Connection timed out (-110)
dev 'ad9361-phy', attr 'trx_rate_governor', value :ERROR: Connection timed out (-110)
dev 'ad9361-phy', attr 'trx_rate_governor_available', value :ERROR: Connection timed out (-110)
dev 'ad9361-phy', attr 'tx_path_rates', value :ERROR: Connection timed out (-110)
dev 'ad9361-phy', attr 'xo_correction', value :ERROR: Connection timed out (-110)
dev 'ad9361-phy', attr 'xo_correction_available', value :ERROR: Connection timed out (-110)

with local, we get a partial buffer read...

root@analog:/usr/local/src/libiio/build# ./tests/iio_attr -u ip:192.168.2.1 -d ad9361-phy 
^C
root@analog:/usr/local/src/libiio/build# ./tests/iio_attr -d ad9361-phy 
dev 'ad9361-phy', attr 'calib_mode', value :'auto'
dev 'ad9361-phy', attr 'calib_mode_available', value :'auto manual manual_tx_quad tx_quad rf_dc_offs rssi_gain_step'
dev 'ad9361-phy', attr 'dcxo_tune_coarse', value :'8'
dev 'ad9361-phy', attr 'dcxo_tune_coarse_available', value :'[0 1 63]'
dev 'ad9361-phy', attr 'dcxo_tune_fine', value :'5920'
dev 'ad9361-phy', attr 'dcxo_tune_fine_available', value :'[0 1 8191]'
dev 'ad9361-phy', attr 'ensm_mode', value :'fdd'
dev 'ad9361-phy', attr 'ensm_mode_available', value :'sleep wait alert fdd pinctrl pinctrl_fdd_indep'
dev 'ad9361-phy', attr 'filter_fir_config', value :'FIR Rx: 0,0 Tx: 0,0'
dev 'ad9361-phy', attr 'gain_table_config', value :'<gaintable AD9361 type=FULL dest=3 start=1300000000 end=4000000000>
-3, 0x00, 0x00, 0x20
-3, 0x00, 0x00, 0x00
-3, 0x00, 0x00, 0x00
-2, 0x00, 0x01, 0x00
-1, 0x00, 0x02, 0x00
0, 0x00, 0x03, 0x00
1, 0x00, 0x04, 0x00
2, 0x00, 0x05, 0x00
3, 0x01, 0x03, 0x20
4,'
dev 'ad9361-phy', attr 'multichip_sync', value :ERROR: Input/output error (-5)
dev 'ad9361-phy', attr 'rssi_gain_step_error', value :'lna_error: 0 0 0 0
mixer_error: 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
gain_step_calib_reg_val: 0 0 0 0 0'
dev 'ad9361-phy', attr 'rx_path_rates', value :'BBPLL:951040000 ADC:237760000 R2:118880000 R1:59440000 RF:29720000 RXSAMP:29720000'
dev 'ad9361-phy', attr 'trx_rate_governor', value :'nominal'
dev 'ad9361-phy', attr 'trx_rate_governor_available', value :'nominal highest_osr'
dev 'ad9361-phy', attr 'tx_path_rates', value :'BBPLL:951040000 DAC:237760000 T2:118880000 T1:59440000 TF:29720000 TXSAMP:29720000'
dev 'ad9361-phy', attr 'xo_correction', value :'40000000'
dev 'ad9361-phy', attr 'xo_correction_available', value :'[40000000 1 40000000]'

which we know is wrong, since gain_table_config should end with a </gaintable> tag.

So three different backends - three different results

:(

rgetz · 2020-08-06T02:07:05Z

looks like the usb backend bug has always been there (so at least it is not a regression).

Fix #357 If a target buffer is too small, right now, we try to put a partial fill in the buffer, and return sucess (which is wrong). Now we will return EFBIG, and leave the target buffer empty. This will make the local backends the same as the otherbackends. Signed-off-by: Robin Getz <[email protected]>

dNechita added the enhancement label Jan 28, 2020

rgetz added the bug label Jan 28, 2020

rgetz added this to the 0.20 Release milestone Apr 17, 2020

rgetz modified the milestones: 0.20 Release, 0.21 Release Apr 29, 2020

rgetz modified the milestones: 0.21 Release, v0.22 Release Jun 25, 2020

rgetz self-assigned this Jun 28, 2020

rgetz closed this as completed Jul 31, 2020

rgetz reopened this Jul 31, 2020

rgetz mentioned this issue Oct 7, 2020

local.c: when target buffer is too small, fail #596

Merged

commodo closed this as completed in #596 Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Different behaviour (local vs remote) when an iio attribute read causes buffer overflow #357

Different behaviour (local vs remote) when an iio attribute read causes buffer overflow #357

dNechita commented Jan 28, 2020

rgetz commented Jan 28, 2020 •

edited

Loading

pcercuei commented Jan 28, 2020

rgetz commented Jan 29, 2020

rgetz commented Jul 31, 2020

rgetz commented Aug 6, 2020

Different behaviour (local vs remote) when an iio attribute read causes buffer overflow #357

Different behaviour (local vs remote) when an iio attribute read causes buffer overflow #357

Comments

dNechita commented Jan 28, 2020

rgetz commented Jan 28, 2020 • edited Loading

pcercuei commented Jan 28, 2020

rgetz commented Jan 29, 2020

rgetz commented Jul 31, 2020

rgetz commented Aug 6, 2020

rgetz commented Jan 28, 2020 •

edited

Loading