frederik
/
xserver
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

dix/privates.c: Avoid undefined behaviour after realloc() 

Adding the offset between the realloc result and the old allocation to
update pointers into the new allocation is undefined behaviour: the
old pointers are no longer valid after realloc() according to the C
standard. While this works on almost all architectures and compilers,
it causes  problems on architectures that track pointer bounds (e.g.
CHERI or Arm's Morello): the DevPrivateKey pointers will still have the
bounds of the previous allocation and therefore any dereference will
result in a run-time trap.

I found this due to a crash (dereferencing an invalid capability) while
trying to run `XVnc` on a CHERI-RISC-V system. With this commit I can
successfully connect to the XVnc instance running inside a QEMU with a
VNC viewer on my host.

This also changes the check whether the allocation was moved to use
uintptr_t instead of a pointer since according to the C standard:
"The value of a pointer becomes indeterminate when the object it
points to (or just past) reaches the end of its lifetime." Casting to an
integer type avoids this undefined behaviour.

Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
This commit is contained in:
Alex Richardson 2021-07-23 09:23:45 +01:00 committed by Povilas Kanapickas
parent b9218fadf3
commit f9f705bf3c
1 changed files with 7 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
dix/privates.c
View File

@ -155,14 +155,13 @@ dixMovePrivates(PrivatePtr *privates, int new_offset, unsigned bytes)
static Bool static Bool
fixupOneScreen(ScreenPtr pScreen, FixupFunc fixup, unsigned bytes) fixupOneScreen(ScreenPtr pScreen, FixupFunc fixup, unsigned bytes)
{ {
intptr_t dist; uintptr_t old;
char *old;
char *new; char *new;
DevPrivateKey *keyp, key; DevPrivateKey *keyp, key;
DevPrivateType type; DevPrivateType type;
int size; int size;
old = (char *) pScreen->devPrivates; old = (uintptr_t) pScreen->devPrivates;
size = global_keys[PRIVATE_SCREEN].offset; size = global_keys[PRIVATE_SCREEN].offset;
if (!fixup (&pScreen->devPrivates, size, bytes)) if (!fixup (&pScreen->devPrivates, size, bytes))
return FALSE; return FALSE;
@ -182,9 +181,7 @@ fixupOneScreen(ScreenPtr pScreen, FixupFunc fixup, unsigned bytes)
if (fixup == dixMovePrivates) if (fixup == dixMovePrivates)
new += bytes; new += bytes;
dist = new - old; if ((uintptr_t) new != old) {
if (dist) {
for (type = PRIVATE_XSELINUX; type < PRIVATE_LAST; type++) for (type = PRIVATE_XSELINUX; type < PRIVATE_LAST; type++)
/* Walk the privates list, being careful as the /* Walk the privates list, being careful as the
@ -199,10 +196,11 @@ fixupOneScreen(ScreenPtr pScreen, FixupFunc fixup, unsigned bytes)
* is contained within the allocation. Privates * is contained within the allocation. Privates
* stored elsewhere will be left alone * stored elsewhere will be left alone
*/ */
if (old <= (char *) key && (char *) key < old + size) if (old <= (uintptr_t) key && (uintptr_t) key < old + size)
{ {
/* Compute new location of key */ /* Compute new location of key (deriving from the new
key = (DevPrivateKey) ((char *) key + dist); * allocation to avoid UB) */
key = (DevPrivateKey) (new + ((uintptr_t) key - old));
/* Patch the list */ /* Patch the list */
*keyp = key; *keyp = key;