On Wed, Jul 1, 2015 at 1:59 PM, Emil Velikov emil.l.velikov@gmail.com wrote:
On 1 July 2015 at 18:37, Ilia Mirkin imirkin@alum.mit.edu wrote:
On Wed, Jul 1, 2015 at 1:18 PM, Colin Ian King colin.king@canonical.com wrote:
On 01/07/15 18:12, Emil Velikov wrote:
On 1 July 2015 at 17:56, Ilia Mirkin imirkin@alum.mit.edu wrote:
On Wed, Jul 1, 2015 at 12:51 PM, Colin King colin.king@canonical.com wrote:
From: Colin Ian King colin.king@canonical.com
Various usif_ioctl helper functions do not initialize the return variable ret and some of the error handling return paths just return garbage values that were on the stack (or in a register). I believe that in all the cases, the initial ret variable should be set to -EINVAL and subsequent paths through these helper functions set it appropriately otherwise.
Found via static analysis using cppcheck:
[drivers/gpu/drm/nouveau/nouveau_usif.c:138]: (error) Uninitialized variable: ret
It sure would seem that way, wouldn't it?
#define nvif_unpack(d,vl,vh,m) ({ \ if ((vl) == 0 || ret == -ENOSYS) { \ int _size = sizeof(d); \ if (_size <= size && (d).version >= (vl) && \ (d).version <= (vh)) { \ data = (u8 *)data + _size; \ size = size - _size; \ ret = ((m) || !size) ? 0 : -E2BIG; \ } else { \ ret = -ENOSYS; \ } \ } \ (ret == 0); \ })
So actually it does get initialized, and I guess cppcheck doesn't know about macros?
Hrm, what about the case when ((vl) == 0 || ret == -ENOSYS) is false, where is ret being set in that case?
Is that actually the case for any of the callsites? gcc would complain about that...
There is one:
drm/nouveau/nvkm/engine/disp/nv50.c: if (nvif_unpack(args->v1, 1, 1, true))
Seems like a recent addition though, I don't recall it with back when was introduced.
It follows a call to nvif_unpack(0) though, which will initialize ret.