The RecordSanityCheckRegisterClients() checks for the request length,
but does not check for integer overflow.
A client might send a very large value for either the number of clients
or the number of protocol ranges that will cause an integer overflow in
the request length computation, defeating the check for request length.
To avoid the issue, explicitly check the number of clients against the
limit of clients (which is much lower than an maximum integer value) and
the number of protocol ranges (multiplied by the record length) do not
exceed the maximum integer value.
This way, we ensure that the final computation for the request length
will not overflow the maximum integer limit.
CVE-2025-49179
This issue was discovered by Nils Emmerich <nemmerich@ernw.de> and
reported by Julian Suleder via ERNW Vulnerability Disclosure.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2024>
Since most of the extension init logic (and on/off switches for them)
is driven from miext, this seems the appropriate place for the header.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Only key difference that calloc(), in contrast to rellocarray(),
is zero-initializing. The overhead is hard to measure on today's
machines, and it's safer programming practise to always allocate
zero-initialized, so one can't forget to do it explicitly.
Cocci rule:
@@
expression COUNT;
expression LEN;
@@
- xallocarray(COUNT,LEN)
+ calloc(COUNT,LEN)
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
* use their actual path instead of relying this to be in compiler's
include path list.
* no need to do it only conditionally, no #ifdef needed
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Retrieves the ClientPtr for the owner of given resource.
This way reducing the sites directly accessing clients[] array.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Make it type-safe and a bit more obvious what it really does,
also adding some inline documentation. Since it's just some
bit shifting magic, it's qualified for inlining.
The CLIENT_ID() macro isn't used by any external modules, so the
new function doesn't need to be in a public header.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Using calloc() instead of malloc() as preventive measure, so there
never can be any hidden bugs or leaks due uninitialized memory.
The extra cost of using this compiler intrinsic should be practically
impossible to measure - in many cases a good compiler can even deduce
if certain areas really don't need to be zero'd (because they're written
to right after allocation) and create more efficient machine code.
The code pathes in question are pretty cold anyways, so it's probably
not worth even thinking about potential extra runtime costs.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Even though the situation probably never happens, but better being extra
cautious, just in case.
| ../record/set.c: In function ‘IntervalListCreateSet’:
| ../record/set.c:364:5: warning: use of NULL ‘stackIntervals’ where non-null expected [CWE-476] [-Wanalyzer-null-argument]
| 364 | memcpy(&prls[1], stackIntervals, nIntervals * sizeof(RecordSetInterval));
| | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The request struct's length fields aren't used anymore - we have the
client->req_len field instead, which also is bigreq-compatible.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1639>
PANORAMIX was the original working title of the extension, before it became
official standard. Just nobody cared about fixing the symbols to the official
naming.
For backwards compatibility with drivers, the old PANORAMIX symbol will
still be set.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1258>
The symbol controls whether to include dix-config.h, and it's always set,
thus we don't need it (and dozens of ifdef's) anymore.
This commit only removes them from our own source files, where we can
guarantee that dix-config.h is present - leaving the (potentially exported)
headers untouched.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
It's only used for record extension, no external callers, thus doesn't
need to be exported. Since it's just for retrieving one struct value,
it's not needed at all - we can do this directly (just like we do in
many other places)
Note: the check on noPanoramixExtensions is superfluous, since the only
call site already does it.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1345>
Windows' native headers using some our RT_* define's names for other things.
Since the naming isn't very nice anyways, introducing some new ones
(X11_RESTYPE_NONE, X11_RESTYPE_FONT, X11_RESTYPE_CURSOR) and define the old
ones as an alias to them, in case some out-of-tree code still uses them.
With thins change, we don't need to be so extremely careful about include
ordering and have explicit #undef's in order to prevent name clashes on
Win32 targets.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1355>
This header isn't installed, so no external modules could use the
functions declared there. Thus we can unexport it all.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1358>
I was seeing SIGBUS errors when running TigerVNC inside the record code
because the offsets into the memory buffer were not sufficiently aligned
to use the RecordSetRec (alignof(void*)). The current code ensures that
all offsets are aligned to sizeof(unsigned long), but that may not be
sufficient to load/store a pointer. Architectures where this is not true
include Arm Morello which has 16-byte pointers and 8-byte longs.
ZDI-CAN-14952, CVE-2021-4011
This vulnerability was discovered and the fix was suggested by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
CVE-2020-14362 ZDI-CAN-11574
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
Most (but not all) of these were found by using
codespell --builtin clear,rare,usage,informal,code,names
but not everything reported by that was fixed.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
If a client is in the process of being closed down, then its client->osPrivate
pointer will be set to NULL by CloseDownConnection. This can cause a crash if
freeing the client's resources results in a call to AttendClient. For example,
if the client has a pending sync fence:
Thread 1 "X" received signal SIGSEGV, Segmentation fault.
AttendClient (client=0x5571c4aed9a0) at ../os/connection.c:942
(gdb) bt
#0 AttendClient (client=0x5571c4aed9a0) at ../os/connection.c:942
#1 0x00005571c3dbb865 in SyncAwaitTriggerFired (pTrigger=<optimized out>) at ../Xext/sync.c:694
#2 0x00005571c3dd5749 in miSyncDestroyFence (pFence=0x5571c5063980) at ../miext/sync/misync.c:120
#3 0x00005571c3dbbc69 in FreeFence (obj=<optimized out>, id=<optimized out>) at ../Xext/sync.c:1909
#4 0x00005571c3d7a01d in doFreeResource (res=0x5571c506e3d0, skip=skip@entry=0) at ../dix/resource.c:880
#5 0x00005571c3d7b1dc in FreeClientResources (client=0x5571c4aed9a0) at ../dix/resource.c:1146
#6 FreeClientResources (client=0x5571c4aed9a0) at ../dix/resource.c:1109
#7 0x00005571c3d5525f in CloseDownClient (client=0x5571c4aed9a0) at ../dix/dispatch.c:3473
#8 0x00005571c3d55eeb in Dispatch () at ../dix/dispatch.c:492
#9 0x00005571c3d59e96 in dix_main (argc=3, argv=0x7ffe7854bc28, envp=<optimized out>) at ../dix/main.c:276
#10 0x00007fea4837cb6b in __libc_start_main (main=0x5571c3d1d060 <main>, argc=3, argv=0x7ffe7854bc28, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffe7854bc18) at ../csu/libc-start.c:308
#11 0x00005571c3d1d09a in _start () at ../Xext/sync.c:2378
(gdb) print client->osPrivate
$1 = (void *) 0x0
Since the client is about to be freed, its ignore count doesn't matter and
AttendClient can simply be a no-op. Check for client->clientGone in AttendClient
and remove similar checks from two callers that had them.
Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
Separate each statement of the form "assert(a && b);" into "assert(a);"
and "assert(b);" for more precise diagnostics, except for this clever
use in drmmode_display.c where it was used to pass a hint to developers:
assert(num_infos <= 32 && "update return type");
This is a work in progress that builds Xvfb, Xephyr, Xwayland, Xnest,
and Xdmx so far. The outline of Xquartz/Xwin support is in tree, but
hasn't been built yet. The unit tests are also not done.
The intent is to build this as a complete replacement for the
autotools system, then eventually replace autotools. meson is faster
to generate the build, faster to run the bulid, shorter to write the
build files in, and less error-prone than autotools.
v2: Fix indentation nits, move version declaration to project(), use
existing meson_options for version-config.h's vendor name/web.
Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
If a client sends a RecordUnregisterClients request with an nClients
field larger than INT_MAX / 4, an integer overflow leads to an
out of boundary access in RecordSanityCheckClientSpecifiers.
An example line with libXtst would be:
XRecordUnregisterClients(dpy, rc, clients, 0x40000001);
Reviewed-by: Adam Jackson <ajax@redhat.com>
This touches everything that ends up in the Xorg binary; the big missing
part is GLX since that's all generated code. Cuts about 14k from the
binary on amd64.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
This just removes the comment markers from around the formals in
several function prototypes near where pointer -> void * changes were
made. There are plenty more of these to fix.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
This lets us stop using the 'pointer' typedef in Xdefs.h as 'pointer'
is used throughout the X server for other things, and having duplicate
names generates compiler warnings.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
When building on Solaris with _XOPEN_SOURCE set to a recent XPG release,
<stdlib.h> and other core headers start including <sys/regset.h>, which
has a bunch of unfortunately named macros such as "CS", "ES", etc. for
x86 & x64 registers which clash with existing variable & struct member
names in Xorg - so #undef these so they don't interfere with our use.
(Yes, have filed a bug against the system headers for exposing these,
but this solves the problem for building on existing releases.)
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
Create extinit.h (and xf86Extensions.h, for Xorg-specific extensions) to
hold all our extension initialisation prototypes, rather than
duplicating them everywhere.
Signed-off-by: Daniel Stone <daniel@fooishbar.org>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Tested-by: Daniel Stone <daniel@fooishbar.org>
When passing variable pointers to functions or otherwise doing long
sequences to compute values for replies, create & use some new
temporary variables, to allow for simpler initialization of reply
structures in the following patches.
Move memsets & other initializations to group with the rest of the
filling in of the reply structure, now that they're not needed so
early in the code path.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Tested-by: Daniel Stone <daniel@fooishbar.org>
Casting return to (void) was used to tell lint that you intended
to ignore the return value, so it didn't warn you about it.
Casting the third argument to (char *) was used as the most generic
pointer type in the days before compilers supported C89 (void *)
(except for a couple places it's used for byte-sized pointer math).
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Tested-by: Daniel Stone <daniel@fooishbar.org>
This is strictly the application of the script 'x-indent-all.sh'
from util/modular. Compared to the patch that Daniel posted in
January, I've added a few indent flags:
-bap
-psl
-T PrivatePtr
-T pmWait
-T _XFUNCPROTOBEGIN
-T _XFUNCPROTOEND
-T _X_EXPORT
The typedefs were needed to make the output of sdksyms.sh match the
previous output, otherwise, the code is formatted badly enough that
sdksyms.sh generates incorrect output.
The generated code was compared with the previous version and found to
be essentially identical -- "assert" line numbers and BUILD_TIME were
the only differences found.
The comparison was done with this script:
dir1=$1
dir2=$2
for dir in $dir1 $dir2; do
(cd $dir && find . -name '*.o' | while read file; do
dir=`dirname $file`
base=`basename $file .o`
dump=$dir/$base.dump
objdump -d $file > $dump
done)
done
find $dir1 -name '*.dump' | while read dump; do
otherdump=`echo $dump | sed "s;$dir1;$dir2;"`
diff -u $dump $otherdump
done
Signed-off-by: Keith Packard <keithp@keithp.com>
Acked-by: Daniel Stone <daniel@fooishbar.org>
Acked-by: Alan Coopersmith <alan.coopersmith@oracle.com>
The record extension needs the major and minor opcodes in the reply
hook, but the request buffer may have been freed by the time the hook
is invoked. Saving the request major and minor codes as the request is
executed avoids fetching from the defunct request buffer.
This patch also eliminates the public MinorOpcodeOfRequest function,
inlining it into Dispatch. Usages of that function have been replaced
with direct access to the new ClientRec field.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Jamey Sharp <jamey@minilop.net>
Any pad bytes in replies are written to the client from a zeroed
array. However, record extension tries to incorrectly access the pad
bytes from the end of reply data.
Signed-off-by: Rami Ylimäki <rami.ylimaki@vincit.fi>
Reviewed-by: Erkki Seppälä <erkki.seppala@vincit.fi>
Also, fix whitespace, mainly around
swaps(&rep.sequenceNumber)
Reviewed-by: Peter Harris <pharris@opentext.com>
Signed-off-by: Matt Turner <mattst88@gmail.com>
Since RecordDeleteContext frees its argument, don't fall through to free
it again.
Error: Double free (CWE 415)
Double free of pointer 'malloc(1072)' defined by malloc
at line 1964 of record/record.c in function 'ProcRecordCreateContext'.
Previously freed at line 1960 with RecordDeleteContext.
'malloc(1072)' was allocated at line 1926 with malloc.
[ This bug was found by the Parfait 0.3.7 bug checking tool.
For more information see http://labs.oracle.com/projects/parfait/ ]
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Jeremy Huddleston <jeremyhu@apple.com>
dasha_uwu
Mike Gelfand
Olivier Fourdan
Enrico Weigelt, metux IT consult
Alex Richardson
Povilas Kanapickas
Matthieu Herrb
Alan Coopersmith
Aaron Plattner
Adam Jackson
Adam Richter
Eric Anholt
Tobias Stoeckmann
Julien Cristau
Peter Hutterer
Keith Packard
Daniel Stone
Rami Ylimäki
Matt Turner