Move this to a little function (which also has a better name now)
inside the already platform specific ossock.c
Also removing the EMSGSIZE check: we're using TCP or local unix
socket, so EMSGSIZE is never returned.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
It's only used by record extension, not used by any drivers at all
(neither FOSS nor proprietary), so it shouldn't be in the public SDK.
Since it's never been used by any driver, it's effectively no ABI change,
so we can safely do this within ABI-25.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
This function was badly broken, among other things doing a
use after free, a NULL-pointer dereferrence, and not doing
what it was supposed to do.
Signed-off-by: stefan11111 <stefan11111@shitposting.expert>
Flushclient implicitly cast the result from a write-like call
to size_t, which turned it's error return value, -1,
into a large positive value, breaking the surrounding error-checking code.
Signed-off-by: stefan11111 <stefan11111@shitposting.expert>
Since FlushClient only writes the data in the client's buffer,
with no extra data or padding, we only write at most as many bytes
as there are in the client buffer.
we just have to set oco->count to notWritten, the amount we still
have to write.
Signed-off-by: stefan11111 <stefan11111@shitposting.expert>
FlushClient() does drops the output buffer, when it becomes empty.
This previously wasn't any problem and actually intented, because we've
returned from the write path directly after FlushClient() was called.
But now we've changed the call order, so it's called from within
OutputBufferMakeRoom(), so we need to make sure we always got valid
output buffer before trying to access it.
For the future, we should think about whether dropping output buffers
really makes sense at all. Instead we could leave them as they are
(over the client's lifetime) and maybe just trim when they've went
too big.
Fixes: 1c13cfa6ca
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Since nobody's passing in extra data here anymore, this function
can be radically simplified now.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Move up the check for broken/NULL transport connection in order to
simplify the code a bit more.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
First step for simplifying the output path - this is really complicated now:
FlushClient() is called in two cases:
a) we really need to send out critical data (eg. critical events now),
here we have no extra data
b) going to write new data in the output buffer, but it's already full
here we do have extra data
In case b) (only called from WriteToClient()) we're first trying to write out
as much as we can, and if there's still not enough room, the buffer is resized.
The write-out path is a complex look trying to write buffered data first, then
the new data. That's even more complex since using writev() with 3 pieces
(old buffer, new data, padding), and considering each of those could be written
just partially.
By the way, there's really no need that the new data is strictly written
along with the already buffered one - practically that's not even any actual
performance optimization - so it's just making things unncessarily complicated.
Therefore reduce it to what's really needed: ensure enough room in the output
buffer (and potentially flush out or resize the buffer). In a later, remove the
whole extra data part from FlushClient(), as it's not needed anymore.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
These are always enablde (x11_t is defined when XSERV_t is defined),
so no need for the #ifdef's anymore.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
When reading requests from the clients, the input buffer might be shared
and used between different clients.
If a given client sends a full request with non-zero bytes to ignore,
the bytes to ignore may still be non-zero even though the request is
full, in which case the buffer could be shared with another client who's
request will not be processed because of those bytes to ignore, leading
to a possible hang of the other client request.
To avoid the issue, make sure we have zero bytes to ignore left in the
input request when sharing the input buffer with another client.
CVE-2025-49178
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>
The BigRequest extension allows requests larger than the 16-bit length
limit.
It uses integers for the request length and checks for the size not to
exceed the maxBigRequestSize limit, but does so after translating the
length to integer by multiplying the given size in bytes by 4.
In doing so, it might overflow the integer size limit before actually
checking for the overflow, defeating the purpose of the test.
To avoid the issue, make sure to check that the request size does not
overflow the maxBigRequestSize limit prior to any conversion.
The caller Dispatch() function however expects the return value to be in
bytes, so we cannot just return the converted value in case of error, as
that would also overflow the integer size.
To preserve the existing API, we use a negative value for the X11 error
code BadLength as the function only return positive values, 0 or -1 and
update the caller Dispatch() function to take that case into account to
return the error code to the offending client.
CVE-2025-49176
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: Michel Dänzer <mdaenzer@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2024>
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>
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>
* this symbol is a server configuration flag (can be passed via cmdline)
for limiting the max size of big-requests. there shouldn't be any need
to use it outside the core X server (in server modules like drivers
or external extension) - therefore unexport it
* in order to reduce namespace pollution of public (server module API)
headers, create a new internal header for those tings (more to come)
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1275>
This breaks the xf86-input-synaptics driver:
synaptics.c: In function 'clickpad_guess_clickfingers':
synaptics.c:2638:5: error: implicit declaration of function 'BUG_RETURN_VAL' [-Werror=implicit-function-declaration]
2638 | BUG_RETURN_VAL(hw->num_mt_mask > sizeof(close_point) * 8, 0);
This reverts commit 442aec2219.
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1316>
Yet another step of uncluttering includes: move out the BUG_* macros
into a separate header, which then is included as-needed.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
The commit 9bf46610a9 "os: Immediately
queue initial WriteToClient" effectively disables buffering (of all
writes, not just the "initial" write), since the OS's network buffers
will usually be large enough to hold whatever replies we have sent.
This does improve performance when drawing over a Unix socket (I measure
approximtely 10%, not the ~5x mentioned in that commit message, probably
due to the large changes in this area since that commit), but it
decreases performance when drawing over a network due to the additional
TCP packets. This decrease is small (~10%) in most cases, but if the two
machines have mismatched Nagle / tcp_delay settings it can cause
XGetWindowAttributes to take 200ms (because it's composed of two
requests, the 2nd of which might wait for the ack which is delayed).
Avoid network slowdowns by making the immediate flush conditional on
who->local.
Signed-off-by: Peter Harris <pharris@opentext.com>
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>
You might as well, it's harmless. Better, some cleanup code (like DRI2
swap wait) needs to run both normally and at client exit, so it
simplifies the callers to not need to check first. See 4308f5d3 for a
similar example.
Props: @ajax (Adam Jackson)
Fixes: xorg/xserver#211
Signed-off-by: Daniel Llewellyn <diddledan@ubuntu.com>
Otherwise this is broken on cygwin:
rrlease.c: In function ‘ProcRRCreateLease’:
rrlease.c:305:9: error: implicit declaration of function ‘WriteFdToClient’ [-Werror=implicit-function-declaration]
if (WriteFdToClient(client, fd, TRUE) < 0) {
Signed-off-by: Adam Jackson <ajax@redhat.com>
A client can send a big request where the 32B "length" field has value
0. When the big request header is removed and the length corrected,
the value will underflow to 0xFFFFFFFF. Functions processing the
request later will think that the client sent much more data and may
touch memory beyond the receive buffer.
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
oc->trans_conn is set to NULL when the connection is closed. At this
point, oc->fd is no longer valid and shouldn't be used. Move
dereference of oc->fd up into YieldControlNoInput where the state of
oc->trans_conn can be checked in a single place.
Reviewed-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
AbortClient performs most of the same operations as
CloseDownFileDescriptor except that it doesn't call ospoll_remove,
leaving that unaware that the file descriptor has been closed.
If the file descriptor is re-used before the server comes back around
to clean up, and that new file descriptor is passed to SetNotifyFd,
then that function will mistakenly re-interpret the stale ClientPtr
returned by ospoll_data as a struct notify * instead and mangle data
badly.
To fix this, the patch does:
1) Change CloseDownFileDescriptor so that it can be called multiple
times on the same OsCommPtr. The calls related to the file
descriptor are moved inside the check for trans_conn and
oc->trans_conn is set to NULL after cleaning up.
2) Move the XdmcpCloseDisplay call into CloseDownFileDescriptor. I
don't think the actually matters as we just need to know at some
point that the session client has exited. Moving it avoids the
possibility of having this accidentally trigger from another client
with the same fd which closes down at around the same time.
3) Change AbortClient to call CloseDownFileDescriptor. This makes sure
that all of the fd-related clean up happens in the same way
everywhere, in particular ensures that ospoll is notified about the
closed file descriptor at the time it is closed and not some time later.
Debian-bug: https://bugs.debian.org/862824
Reviewed-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
There are three copies of the same short sequence of operations to
close down a client when a write error occurs. Create a new function,
AbortClient, which performs these operations and then call it from the
three places.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-and-Tested-by: Michel Dänzer <michel.daenzer@amd.com>
The clever pointer tricks were actually not working, and we were doing
the byte-by-byte moves in general. By just doing the memcpy and
obvious byte swap code, we end up generating actual byte swap
instructions, thanks to optimizing compilers.
text data bss dec hex filename
before: 2240807 51552 132016 2424375 24fe37 hw/xfree86/Xorg
after: 2215167 51552 132016 2398735 249a0f hw/xfree86/Xorg
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
The input thread should generate events, not send them. Make it easier to
find the instances where it's doing so.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Tested-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
When a client with pending output is ready (has request data pending),
FlushAllOutput will skip it to get all of the requests processed
before sending any queued output. That means FlushAllOutput is going
to return with some output pending to a client which isn't known to be
write blocked. And that means NewOutputPending needs to be set so that
FlushAllOutput will get called again to actually go flush this client.
It might be interesting to try just flushing the client to send any
queued data along the way. This patch just restores the server
behavior to what it was before the ospoll changes.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Tested-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Tested-by: Matthieu Herrb <matthieu@herrb.eu>
This change has two effects:
1. Only calls FlushCallbacks when we're actually flushing data to a
client. The unnecessary FlushCallback calls could cause significant
performance degradation with compositing, which is significantly
reduced even without any driver changes.
2. By passing the ClientPtr to FlushCallbacks, drivers can completely
eliminate unnecessary flushing of GPU commands by keeping track of
whether we're flushing any XDamageNotify events to the client for
which the corresponding rendering commands haven't been flushed to
the GPU yet.
Reviewed-by: Adam Jackson <ajax@redha.com>
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Eliminates all of the fd_set mangling in the server main thread
v2: Listen for POLLOUT while writes are blocked.
v3: Only mark client not ready on EAGAIN return from read
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
This converts the dispatch loop into using a list of ready clients
instead of an array. This changes the WaitForSomething API so that it
notifies DIX when a client becomes ready to read, instead of returning
the set of ready clients.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
This matches a change made in xcb and improves performance for a small
increase in memory usage.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
This adds the ability to be notified when a file descriptor is
available for writing.
Reviewed-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
