Created attachment 20444 [details]
Test case for VIDIOC_S_CTRL with invalid value

Hmmm... This is a nasty one.

Clamping the control value to the valid range would be easy if the driver was aware of the valid control values. However, those values are reported at runtime by the device, and might even be allowed to change dynamically (the UVC specification isn't clear on whether bounds changes are runtime are legal or not).

I could either query all controls for their minimum and maximum value when probing the device and cache the data, or perform the queries when modifying a control (with or without cache).

The first solution should be avoided, as many buggy UVC cameras will crash if I flood them with requests.

The second solution is better, but will add overhead (4 USB requests instead of one) every time a control is set. Some buggy devices might crash more frequently, and buggy devices that report erroneous min/max values will see their controls becoming unsettable. I'd appreciate feedback on this before committing a patch.

The uvcvideo could fill a cache entry in case when the first VIDIOC_QUERYCTL is called. If the VIDIOC_S_CTRL is called when the cache entry is still empty then the min and max values could be quired first, then return ERANGE for VIDIOC_S_CTRL or set the value and return 0.

Other possibility would be, if the cache entry is not filled to read back the new setting with VIDIOC_G_CTRL and check whether the device could set the value or not.

It is difficult to create a driver which conforms to UVC specification and at the same time supports buggy devices. I would prefer a driver which conforms to the UVC specification and then later have some workarounds for the buggy devices.

One additional comment: V4L2 specification uses signed 32 bit numbers for setting a control. In USB Video Class specification 1.1 there are controls which are limited to 16bit values, for example Brightness Control (chapter 4.2.2.3.2), Contrast Control (chapter 4.2.2.3.3), Gain Control (chapter 4.2.2.3.4), etc.

If a VIDIOC_S_CTRL call arrives from the user space where the value cannot be expressed as a 16bit value then the result can be automatically ERANGE without asking anything from the device. The attached test case addresses this case: it tries to set the brithtness to 0x80000000 which is -2147483648 as a 32bit signed value and cannot be expressed using only 16bits.

(In reply to comment #3)
> The uvcvideo could fill a cache entry in case when the first VIDIOC_QUERYCTRL
> is called.

I mean only one cache entry could be filled: serving the VIDIOC_QUERYCTRL request will fetch the min and max values anyway, so here no additional USB communication is needed. A V4L2 application would usually enumerate all the controls before using them.

Created attachment 20671 [details]
uvcvideo: check out-of-bound values for VIDIOC_S_CTRL

This patch uses the mapping->data_type and mapping->size (in bits) information to decide wheter the value can be expressed in the given bits or not.

Is this issue resolved or is the proposed patch from Márton still relevant?

The attached test case still fails with 2.6.33-rc4:

test.c:59: VIDIOC_QUERYCTRL, id=9963776 (V4L2_CID_BASE+0), ret1=0, errno1=0
test.c:66: queryctrl = {.id=9963776, .type=1, .name="Brightness", .minimum=-64, .maximum=64, .step=1, .default_value=-10, .flags=0x0, .reserved[]={ 0x0, 0x0 } }
test.c:86: VIDIOC_G_CTRL, id=9963776 (V4L2_CID_BASE+0), ret_orig=0, errno_orig=0, control_orig.value=-10
test.c:103: VIDIOC_S_CTRL, id=9963776 (V4L2_CID_BASE+0), value=-2147483648, ret_set=0, errno_set=0
test.c:170: VIDIOC_G_CTRL, id=9963776 (V4L2_CID_BASE+0), ret_get=0, errno_get=0, control_new.value=0
test.c:186: FAILED: new value is not converted to the lower limit: control_new.value=0, queryctrl.minimum=-64
test.c:208: VIDIOC_S_CTRL, id=9963776 (V4L2_CID_BASE+0), value=-10, ret_set=0, errno_set=0
FAILED

Márton,
That is with(In reply to comment #8)
> The attached test case still fails with 2.6.33-rc4:
> 
> test.c:59: VIDIOC_QUERYCTRL, id=9963776 (V4L2_CID_BASE+0), ret1=0, errno1=0
> test.c:66: queryctrl = {.id=9963776, .type=1, .name="Brightness",
> .minimum=-64,
> .maximum=64, .step=1, .default_value=-10, .flags=0x0, .reserved[]={ 0x0, 0x0
> }
> }
> test.c:86: VIDIOC_G_CTRL, id=9963776 (V4L2_CID_BASE+0), ret_orig=0,
> errno_orig=0, control_orig.value=-10
> test.c:103: VIDIOC_S_CTRL, id=9963776 (V4L2_CID_BASE+0), value=-2147483648,
> ret_set=0, errno_set=0
> test.c:170: VIDIOC_G_CTRL, id=9963776 (V4L2_CID_BASE+0), ret_get=0,
> errno_get=0, control_new.value=0
> test.c:186: FAILED: new value is not converted to the lower limit:
> control_new.value=0, queryctrl.minimum=-64
> test.c:208: VIDIOC_S_CTRL, id=9963776 (V4L2_CID_BASE+0), value=-10,
> ret_set=0,
> errno_set=0
> FAILED

That is without your patch from #6, correct?

> That is without your patch from #6, correct?
Yes, I tested with vanilla 2.6.33-rc4, without the patch.

Unfortunately, the attached patch does not fully solves the problem, just helps not to send bugous data to the USB device.

Hi Márton,

I've committed a fix to the http://linuxtv.org/git/?p=pinchartl/uvcvideo.git;a=summary git repository. Could you pleas test it ?

Forgot to mention that the fix is in the uvcvideo branch.

I've tested 7c465907f1cc8656a9c8e693e8cd44c3cbd6b31a together with CNF7129. This device has a brightness setting range between -64 and 64, with the default value of -10.

> test.c:59: VIDIOC_QUERYCTRL, id=9963776 (V4L2_CID_BASE+0), ret1=0, errno1=0
> test.c:66: queryctrl = {.id=9963776, .type=1, .name="Brightness",
> .minimum=-64, .maximum=64, .step=1, .default_value=-10, .flags=0x0,
> .reserved[]={ 0x0, 0x0 } }
> test.c:86: VIDIOC_G_CTRL, id=9963776 (V4L2_CID_BASE+0), ret_orig=0,
> errno_orig=0, control_orig.value=-10
> test.c:103: VIDIOC_S_CTRL, id=9963776 (V4L2_CID_BASE+0), value=-2147483648,
> ret_set=0, errno_set=0
> test.c:170: VIDIOC_G_CTRL, id=9963776 (V4L2_CID_BASE+0), ret_get=0,
> errno_get=0, control_new.value=0
> test.c:186: FAILED: new value is not converted to the lower limit:
> control_new.value=0, queryctrl.minimum=-64
> test.c:208: VIDIOC_S_CTRL, id=9963776 (V4L2_CID_BASE+0), value=-10,
> ret_set=0, errno_set=0
> FAILED

What currently happens is that setting the value -2147483648 will result in 0, so I think the tested version does not fix this problem.

In case of positive values the same software version seems to be working. I tested with values 65 and 2147483647 (=0x7FFFFFFF). Each of these values are correctly limited to the value 64 and the return value is zero which means success.

As I was having a closer look on the source code it seems that in function uvc_ctrl_set(), case V4L2_CTRL_TYPE_INTEGER, there is only a limitation to the max value.

You're right, sorry. I've updated the patch in the repository. Could you please try the new version ?

No problem.

I tested 4bb2783544e7bc9d1e2154e0cdcd803dfd68dc65 on EeePC 901 with CNF7129.
The following test cases are related from http://v4l-test.sourceforge.net/ v0.19:
 * VIDIOC_G_CTRL ... passed
 * VIDIOC_S_CTRL ... passed
 * VIDIOC_S_CTRL with invalid value parameter ... passed

Tested-by: Márton Németh <nm127@freemail.hu>