For type-safety turn xfreeData() macro into an inline function.
Also adding some checks against accidentially free()'ing global data.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The analyzer giving a false alarm on potential NULL-pointer deref here.
Even though that case can't happen, it's also not immediately clear
to the human reader.
To make both the analyzer as well human reader happier, reformulating
this function to by using fast-return pattern.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
There might be a chance that RegionUninit() is trying to free() some
struct that's actually coming from .data segment.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The analyzer is wrong here, because the free'd closure pointer really points
to some calloc()'d chunk, instead of the PolyText()'s stack frame.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
It's safer not relying on all ScreenProc's actually filled.
../dix/events.c: In function ‘CheckPhysLimits’:
../dix/events.c:780:14: warning: dereference of NULL ‘pScreen’ [CWE-476] [-Wanalyzer-null-dereference]
780 | (*pScreen->SetCursorPosition)
| ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The pointer to the window properties is currently inside the WindowOptional
structure, which may or may not exist at any given time. Thus, before accessing
those fields, at least need to check whether it exists, potentially need to
create it first.
Since a pointer is small (in relation to WindowRec) and windows having properties
is a pretty common, we can make our life much simpler here by moving the pointer
directly into WindowRec, so we don't need extra WindowOptionalRec allocation.
This also fixes an analyzer warning on potential NULL dereference issue:
| ../dix/property.c: In function ‘dixChangeWindowProperty’:
|../dix/property.c:343:37: warning: dereference of NULL ‘*pWin.optional’ [CWE-476] [-Wanalyzer-null-dereference]
| 343 | pProp->next = pWin->optional->userProps;
| | ~~~~~~~~~~~~~~^~~~~~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
There's a (theoretical) chance that origGC might be NULL, so better
be cautious and check for this - doesn't cost us much, probably just
another JZ instruction.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Be more cautious on possible NULL pointers or not yet registered
devPrivates. Better a gracefully fail instead of hard segfault.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Even though it *should* never be actually hit, it's still safer
to check for NULL instead of letting us crash with segfault.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Even though it's unlikely ever getting it, still safer to have some
extra checks / asserts than unexpected segfault.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Safer (and easier to understand) if we look at the result pointer
instead of the counter for testing whether device wasn't found.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Better try to handle memory allocation gracefully than just hard
crashing by segfault.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Even though chances are really low it's ever getting hit, it's still safer
to have some sanity checks (which don't cost us much) than risking segfault.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
There's (remote) chance that the (internal) module name could become
NULL (eg. allocation failure). Even though chances to hit it are very
low, it's still better to have a check here (that doesn't cost us much),
just in case. Assert fail is still better than segfault, since we're
at least getting some hint what might have happened.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Adding paranoid extra checks against allocation failure and NULL pointers.
Even though might not be actually hit in practise, it's still better to
be cautious, just in case. And reducing analyzer noise this way.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The analyzer warnings (possible NULL dereference) are probably just
false alarms. But for safety adding assert()'s, which don't cost us
anything in non-debug builds.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
| ../glx/glxcmds.c: In function ‘xorgGlxMakeCurrent’:
| ../glx/glxcmds.c:621:24: warning: use of uninitialized value ‘status’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
| 621 | return status;
| | ^~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
If this ever happens, we clearly have a bug, so print out proper warning,
instead of silently crashing the Xserver.
| ../glx/glxcmds.c: In function ‘validGlxFBConfigForWindow’:
| ../glx/glxcmds.c:127:16: warning: dereference of NULL ‘pVisual’ [CWE-476] [-Wanalyzer-null-dereference]
| 127 | if (pVisual->class != glxConvertToXVisualType(config->visualType) ||
| | ~~~~~~~^~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
| ../glx/glxdricommon.c: In function ‘createModeFromConfig’:
| ../glx/glxdricommon.c:142:23: warning: dereference of possibly-NULL ‘config’ [CWE-690] [-Wanalyzer-possible-null-dereference]
| 142 | config->driConfig = driConfig;
Consumers can already handle returning NULL, so this seems the best compromise.
It will look like we don't have any modes at all. Certainly not nice, but at
least better than completely crashing the Xserver.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
This warning is probably a false alarm, but it's trivial to fix.
| ../glx/createcontext.c: In function ‘__glXDisp_CreateContextAttribsARB’:
| ../glx/createcontext.c:172:24: warning: dereference of NULL ‘0’ [CWE-476] [-Wanalyzer-null-dereference]
| 172 | switch (attribs[i * 2]) {
| | ~~~~~~~^~~~~~~
The situation is too complex for the analyzer to handle:
(but also not immediately clear for the human reader)
* `attribs` is only NULL when req->numAttribs is 0
* in that case, the for loop is no-op, so `attribs` won't ever be deref'ed.
We can make it easier for both analyzer as well as human reader by just putting
the whole loop into an `if (req->numAttribs)` and assigning it inside there.
There shouldn't be any (practically measurable) extra cost.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Even though risk of being actually hit is minimal, better having some extra
safety checks instead of segfaulting, just in case.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
| ../Xext/panoramiXprocs.c: In function ‘PanoramiXCopyArea’:
| ../Xext/panoramiXprocs.c:1152:13: warning: use of uninitialized value ‘pGC’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
| 1152 | if (pGC && pGC->graphicsExposures) {
| | ^~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Even though these probably never happen, it's still better having some
(really cheap) extra checks, just in case.
| ../Xext/xtest.c: In function ‘ProcXTestFakeInput’:
| ../Xext/xtest.c:385:17: warning: dereference of NULL ‘dev’ [CWE-476] [-Wanalyzer-null-dereference]
| 385 | if (!dev->key)
| | ~~~^~~~~
| ../Xext/xtest.c:442:12: warning: dereference of NULL ‘dev’ [CWE-476] [-Wanalyzer-null-dereference]
| 442 | if (dev->sendEventsProc)
| | ~~~^~~~~~~~~~~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Try not to rely on deep black magic of calloc(), instead skip the whole
part of nrects is zero.
| ../Xext/shape.c: In function ‘ProcShapeGetRectangles’:
| ../Xext/shape.c:995:24: warning: dereference of possibly-NULL ‘rects’ [CWE-690] [-Wanalyzer-possible-null-dereference]
| 995 | rects[i].x = box->x1;
| | ~~~~~~~~~~~^~~~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> ../Xext/xres.c: In function ‘DestroyFragments’:
> ../Xext/xres.c:124:9: warning: ‘free’ of ‘it’ which points to memory on the stack [CWE-590] [-Wanalyzer-free-of-non-heap]
> 124 | free(it);
> | ^~~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The ‘RRCrtcNotify() and RRCrtcSet() functions are exported, so there's chance
that a buggy driver could call them with NULL parameter, leading to segfault.
Those are hard to trace, so it's better having a BUG_* check here.
| ../randr/rrcrtc.c: In function ‘RRCrtcNotify’:
| ../randr/rrcrtc.c:187:5: warning: use of NULL ‘outputs’ where non-null expected [CWE-476] [-Wanalyzer-null-argument]
| 187 | memcpy(crtc->outputs, outputs, numOutputs * sizeof(RROutputPtr));
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ../randr/rrcrtc.c: In function ‘RRCrtcSet’:
| ../randr/rrcrtc.c:742:20: warning: dereference of NULL ‘outputs’ [CWE-476] [-Wanalyzer-null-dereference]
| 742 | if (outputs[o]) {
| | ~~~~~~~^~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
If there's no data to send, the whole reply payload can be skipped entirely.
This can also ease the whole code flow, and we don't need to rely on the
individual copy loops never trying to dereference a NULL pointer.
(what the analyzer can't proof). Also scoping several some variables that
are only used when there actually is data to send.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
If there's no data to send, the whole reply payload can be skipped entirely.
This can also ease the whole code flow, and we don't need to rely on the
individual copy loops never trying to dereference a NULL pointer.
(what the analyzer can't proof). Also scoping several some variables that
are only used when there actually is data to send.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Moving payload buffer assembly right into the same branch where the buffer is
allocated, so making the whole code flow easier to understand. Also moving the
byteswap there (when the fields should still be in CPU cache), instead of having
some callback doing it much later, so even more simplication.
As a nice by-product, that's also reducing some analyzer noise.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The code can be much simpler by just using CopySwap32Write().
And we also don't need the callback in WriteSwappedDataToClient(),
just call the corresponding write function directly.
This also makes some analyzer warnings go away.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Make it a bit easier to understand how exactly the name string is copied into
the reply payload: just do the little memcpy() right where the target position
is decided any the rest of the payload is filled.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Instead of relying on memcpy() coping with NULL buffer when size == 0,
move the call to the branch where we actually have things to copy.
This also silences yet another analyzer warning.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Instead of relying on memcpy() coping with NULL buffer when size == 0,
move the call to the branch where we actually have things to copy.
This also silences yet another analyzer warning.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Instead of relying on memcpy() coping with NULL buffer when size == 0,
move the call to the branch where we actually have things to copy.
This also silences yet another analyzer warning.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Simplifying the code flow allocating/checking/copying some buffers in
RRConfigureOutputProperty() and RRConfigureProviderProperty() so it's
easier to understand for both the human reader as well as the analyzer.
Depending on whether we have elements to process, a temporary buffer needs
to be allocated, checked for successful allocation and copy over data. The
way it's currently done is technically correct, but unnecessarily complex to
understand: instead of just branching on whether there are elements and doing
all the buffer-related things only then, the branching is done just somewhere
in the middle, only on checking for allocation failure, and relying on both
calloc() and memcpy() not doing weird things when size is zero.
It's easy to simplify by putting it all behind one if statement and so make
things easier for both human reader as well as the analyzer (so it's not
spilling out false alarms here anymore) and also drops unnecessary calls
in the zero-size case.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Enrico Weigelt, metux IT consult