Andreas Boll pushed to branch upstream-unstable at X Strike Force / lib / libx11


Commits:
2d20890e by Arthur Huillet at 2017-03-07T20:39:51Z
_XDefaultError: set XlibDisplayIOError flag before calling exit

_XReply isn't reentrant, and it can lead to deadlocks when the default error
handler is called: _XDefaultError calls exit(1). It is called indirectly by
_XReply when a X protocol error comes in that isn't filtered/handled by an
extension or the application. This means that if the application (or one of its
loaded shared libraries such as the NVIDIA OpenGL driver) has registered any
_fini destructor, _fini will get called while still on the call stack of
_XReply. If the destructor interacts with the X server and calls _XReply, it
will hit a deadlock, looping on the following in _XReply:

    ConditionWait(dpy, dpy->xcb->reply_notify);

It is legal for an application to make Xlib calls during _fini, and that is
useful for an OpenGL driver to avoid resource leaks on the X server side, for
example in the dlopen/dlclose case. However, the driver can not readily tell
whether its _fini is being called because Xlib called exit, or for another
reason (dlclose), so it is hard to cleanly work around this issue in the driver.

This change makes it so _XReply effectively becomes a no-op when called after
_XDefaultError was called, as though an XIOError had happened. The dpy
connection isn't broken at that point, but any call to _XReply is going to 
hang.
This is a bit of a kludge, because the more correct solution would be to make
_XReply reentrant, maybe by broadcasting the reply_notify condition before
calling the default error handler. However, such a change would carry a grater
risk of introducing regressions in Xlib.

This change will drop some valid requests on the floor, but this should not
matter, as it will only do so in the case where the application is dying: X will
clean up after it once exit() is done running. There is the case of
XSetCloseDownMode(RETAIN_PERMANENT), but an application using that and wishing
to clean up resources in _fini would currently be hitting a deadlock, which is
hardly a better situation.

Signed-off-by: Aaron Plattner <aplatt...@nvidia.com>
Reviewed-by: Jamey Sharp <ja...@minilop.net>

- - - - -
c6dadd4c by Alan Coopersmith at 2017-03-24T22:19:18Z
Make Xkb{Get,Set}NamedIndicator spec & manpages match code

The XKB Library spec and the man pages for XkbGetNamedIndicator &
XkbSetNamedIndicator included a device_spec argument neither function
takes, and do not include the XkbGetNamedDeviceIndicator &
XkbSetNamedDeviceIndicator variants that do take it (along with two
other arguments).

This updates them to match the interfaces the code has provided for
decades.

This has been reported multiple times, so this fixes:
https://bugs.freedesktop.org/show_bug.cgi?id=251
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=729812
Sun Bug 4528016 XkbSetNamedIndicator & XkbGetNamedIndicator man pages are 
wrong
  (filed: alan.coopersm...@sun.com 2001-11-15 - now aka Oracle bug 15087506)
X.Org Group Defect Id #9418

Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>
Reviewed-by: Adam Jackson <a...@redhat.com>

- - - - -
b856d5d9 by Alan Coopersmith at 2017-03-24T22:19:27Z
Clarify state parameter to XkbSetNamedDeviceIndicator

Checking a Bool to see if it's NULL does not work well in C.
Also reported in https://bugs.freedesktop.org/show_bug.cgi?id=251

Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>
Reviewed-by: Adam Jackson <a...@redhat.com>

- - - - -
7d2010fe by Alan Coopersmith at 2017-04-07T07:13:03Z
Improve table formatting in XkbChangeControls & XkbKeyNumGroups man pages

Includes fix for Solaris Bug 24564279: "XkbKeyNumGroups.3x11 man page
contains some malformed text" caused by extra whitespace after .TE macros

Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>

- - - - -
4fe66b1c by Ryan C. Gordon at 2017-08-06T23:25:25Z
Valgrind fix for XStoreColor and XStoreColors.

If the "pad" field isn't set, Valgrind will report it as 
uninitialized
memory accesses when the struct is copied into the Display's send buffer.

In practice, this is (probably) harmless, but Valgrind is correct in
believing it's a bug.

https://bugs.freedesktop.org/attachment.cgi?id=133189

Reviewed-by: Alan Coopersmith <alan.coopersm...@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>

- - - - -
83107a67 by walter harms at 2017-08-14T16:02:39Z
fix shadow warning

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
6ec901eb by walter harms at 2017-08-14T16:02:39Z
_XIOError(dpy); will never return so remore dead

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
916dffad by walter harms at 2017-08-14T16:02:40Z
remove argument check for free() adjust one inden

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
0355c392 by walter harms at 2017-08-14T16:02:40Z
fix shadow char_size

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
d02c2466 by walter harms at 2017-08-14T16:12:35Z
fix more shadow warning

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
714921f0 by walter harms at 2017-08-14T16:12:35Z
no need to check argument for _XkbFree()

simplify code by removing unneeded checks

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
c1c14af4 by walter harms at 2017-08-14T16:28:18Z
remove stray extern

remove stray extern

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
7c78fc57 by walter harms at 2017-08-14T16:28:18Z
no need to check args for Xfree()

simplify code

Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
ed9f0d34 by walter harms at 2017-08-14T16:28:18Z
fix memleak in error path

V2: remove unneeded NULL (reported by eric.engest...@imgtec.com)

fix mem leak in error path
Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
433477fc by walter harms at 2017-08-14T16:54:44Z
fix memleak in error path

free all mem on error
Signed-off-by: walter harms <wha...@bfs.de>

- - - - -
9abe8380 by walter harms at 2017-08-20T19:41:41Z
no need to check XFree arguments

You can save a bit of code. The is no need to check XFree arguments bring  
free_fontdataOM in line with other free function and check for NULL arg

Signed-off-by: harms wha...@bfs.de

- - - - -
bf82ec04 by walter harms at 2017-08-20T19:44:26Z
mark _XDefaultIOError as no_return

mark _XDefaultIOError as no_return. No one comes back from exit() ...

Signed-off-by: harms wha...@bfs.de

- - - - -
2911c39c by walter harms at 2017-08-20T19:47:05Z
Fixes: warning: variable 'req' set but not,used

Fixes: warning: variable 'req' set but not used 
[-Wunused-but-set-variable]
       by marking req _X_UNUSED
        Solution was discussed on xorg-devel ML
       Peter Hutter, Alan Coopersmith
        Re: [PATCH libX11 3/5] fix: warning: pointer targets in passing 
argument 2 of '_XSend' differ in signedness [-Wpointer-sign]

Signed-off-by: harms wha...@bfs.de

- - - - -
e02dfe54 by wharms at 2017-08-20T19:50:33Z
add _X_UNUSED to avoid unused variable warnings

- - - - -
36a1ac02 by wharms at 2017-08-20T19:51:57Z
remove empty line

- - - - -
e835a9dc by wharms at 2017-09-03T12:17:45Z
silence gcc warning assignment discards 'const' qualifier from pointer 
target type

- - - - -
34f4464f by Alan Coopersmith at 2018-03-07T22:50:32Z
If XGetImage fails to create image, don't dereference it to bounds check

Reported by gcc 7.3:

GetImage.c:110:25: warning: potential null pointer dereference 
[-Wnull-dereference]
  if (planes < 1 || image->height < 1 || image->bytes_per_line < 
1 ||
                    ~~~~~^~~~~~~~

Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4

Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

- - - - -
a9dafdd5 by Michal Srb at 2018-03-23T04:32:05Z
Use flexible array member instead of fake size.

The _XimCacheStruct structure is followed in memory by two strings containing
fname and encoding. The memory was accessed using the last member of the
structure `char fname[1]`. That is a lie, prohibits us from using sizeof and
confuses checkers. Lets declare it properly as a flexible array, so compilers
don't complain about writing past that array. As bonus we can replace the
XOffsetOf with regular sizeof.

Fixes GCC8 error:
  In function 'strcpy',
      inlined from '_XimWriteCachedDefaultTree' at imLcIm.c:479:5,
      inlined from '_XimCreateDefaultTree' at imLcIm.c:616:2,
      inlined from '_XimLocalOpenIM' at imLcIm.c:700:5:
  /usr/include/bits/string_fortified.h:90:10: error: '__builtin_strcpy'
  forming offset 2 is out of the bounds [0, 1] [-Werror=array-bounds]
     return __builtin___strcpy_chk (__dest, __src, __bos (__dest));

Caused by this line seemingly writing past the fname[1] array:
  imLcIm.c:479:  strcpy (m->fname+strlen(name)+1, encoding);

Reviewed-by: Keith Packard <kei...@keithp.com>
Signed-off-by: Peter Hutterer <peter.hutte...@who-t.net>

- - - - -
50a5a989 by Bhavi Dhingra at 2018-03-24T18:13:46Z
Fix possible memory leak in cmsProp.c:140

https://bugs.freedesktop.org/show_bug.cgi?id=96814

Reviewed-by: Alan Coopersmith <alan.coopersm...@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>

- - - - -
82ca6308 by Alan Coopersmith at 2018-03-30T22:46:18Z
Use size_t for buffer sizes in SetHints.c

These variables store values returned from strlen() as a size_t
and are passed to Xmalloc, which expects a size_t, so lets stop
converting back and forth to int along the way.

Reported by: Konstantin SKliarov
Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>
Reviewed-by: Matthieu Herrb <matth...@herrb.eu>

- - - - -
796f754c by Alan Coopersmith at 2018-05-05T21:45:57Z
Change fall through comment in lcDB.c to match gcc's requirements

Needs to match one of the regexps shown under
https://gcc.gnu.org/onlinedocs/gcc-7.3.0/gcc/Warning-Options.html#index-Wimplicit-fallthrough

Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>

- - - - -
9c5845ff by Martin Natano at 2018-05-14T01:38:58Z
Don't rebuild ks_tables.h if nothing changed.

ks_tables.h is always considered out of date due to the forced rebuild
of the makekeys util. This means the file is also rebuilt during 'make
install', which is usually performed as root, which can to lead
permission problems later on.

Signed-off-by: Martin Natano <nat...@natano.net>
Signed-off-by: Alan Coopersmith <alan.coopersm...@oracle.com>

- - - - -
b676e623 by Samuel Thibault at 2018-06-13T13:46:58Z
XkbOpenDisplay.3: fix typo

XkbOpenDisplay returns a pointer to Display, not a Display.

Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

- - - - -
d81da209 by Tobias Stoeckmann at 2018-07-17T13:23:14Z
Validation of server response in XListHosts.

If a server sends an incorrect length in its response, a client is prone
to perform an out of boundary read while processing the data.

The length field of xHostEntry is used to specify the amount of bytes
used to represent the address. It is 16 bit, which means that it is not
possible to perform an arbitrary memory access, but it might be enough
to read sensitive information, e.g. malloc-related pointers and offsets.

Signed-off-by: Tobias Stoeckmann <tob...@stoeckmann.org>
Reviewed-by: Matthieu Herrb <matth...@herrb.eu>

- - - - -
b469da14 by Tobias Stoeckmann at 2018-08-21T14:42:29Z
Fixed off-by-one writes (CVE-2018-14599).

The functions XGetFontPath, XListExtensions, and XListFonts are
vulnerable to an off-by-one override on malicious server responses.

The server replies consist of chunks consisting of a length byte
followed by actual string, which is not NUL-terminated.

While parsing the response, the length byte is overridden with '\0',
thus the memory area can be used as storage of C strings later on. To
be able to NUL-terminate the last string, the buffer is reserved with
an additional byte of space.

For a boundary check, the variable chend (end of ch) was introduced,
pointing at the end of the buffer which ch initially points to.
Unfortunately there is a difference in handling "the end of ch".

While chend points at the first byte that must not be written to,
the for-loop uses chend as the last byte that can be written to.

Therefore, an off-by-one can occur.

I have refactored the code so chend actually points to the last byte
that can be written to without an out of boundary access. As it is not
possible to achieve "ch + length < chend" and "ch + length + 
1 > chend"
with the corrected chend meaning, I removed the inner if-check.

Signed-off-by: Tobias Stoeckmann <tob...@stoeckmann.org>

- - - - -
dbf72805 by Tobias Stoeckmann at 2018-08-21T14:43:06Z
Fixed out of boundary write (CVE-2018-14600).

The length value is interpreted as signed char on many systems
(depending on default signedness of char), which can lead to an out of
boundary write up to 128 bytes in front of the allocated storage, but
limited to NUL byte(s).

Casting the length value to unsigned char fixes the problem and allows
string values with up to 255 characters.

Signed-off-by: Tobias Stoeckmann <tob...@stoeckmann.org>

- - - - -
e8372276 by Tobias Stoeckmann at 2018-08-21T14:43:22Z
Fixed crash on invalid reply (CVE-2018-14598).

If the server sends a reply in which even the first string would
overflow the transmitted bytes, list[0] (or flist[0]) will be set to
NULL and a count of 0 is returned.

If the resulting list is freed with XFreeExtensionList or
XFreeFontPath later on, the first Xfree call:

    Xfree (list[0]-1)
 turns into
    Xfree (NULL-1)

which will most likely trigger a segmentation fault.

I have modified the code to return NULL if the first string would
overflow, thus protecting the freeing functions later on.

Signed-off-by: Tobias Stoeckmann <tob...@stoeckmann.org>

- - - - -
17370424 by Matthieu Herrb at 2018-08-21T14:53:40Z
Remove statement with no effect.

Signed-off-by: Matthieu Herrb <matth...@herrb.eu>

- - - - -
733f64bf by Matthieu Herrb at 2018-08-21T14:54:50Z
libX11 1.6.6

Signed-off-by: Matthieu Herrb <matth...@herrb.eu>

- - - - -


30 changed files:

- configure.ac
- man/xkb/Makefile.am
- man/xkb/XkbChangeControls.man
- + man/xkb/XkbGetNamedDeviceIndicator.man
- man/xkb/XkbGetNamedIndicator.man
- man/xkb/XkbKeyNumGroups.man
- man/xkb/XkbOpenDisplay.man
- + man/xkb/XkbSetNamedDeviceIndicator.man
- man/xkb/XkbSetNamedIndicator.man
- modules/im/ximcp/imCallbk.c
- modules/im/ximcp/imDefIc.c
- modules/im/ximcp/imInsClbk.c
- modules/im/ximcp/imLcIm.c
- modules/im/ximcp/imLcLkup.c
- modules/om/generic/omGeneric.c
- specs/XKB/ch08.xml
- src/DisName.c
- src/FSWrap.c
- src/FontNames.c
- src/GetFPath.c
- src/GetIFocus.c
- src/GetImage.c
- src/GetKCnt.c
- src/GetPCnt.c
- src/GetPntMap.c
- src/GetSSaver.c
- src/GrServer.c
- src/LiHosts.c
- src/ListExt.c
- src/Macros.c


The diff was not included because it is too large.


View it on GitLab: 
https://salsa.debian.org/xorg-team/lib/libx11/compare/42f4d7af9cf6d1dbfa575552e057328b054a20c9...733f64bfeb311c1d040b2f751bfdef9c9d0f89ef

-- 
View it on GitLab: 
https://salsa.debian.org/xorg-team/lib/libx11/compare/42f4d7af9cf6d1dbfa575552e057328b054a20c9...733f64bfeb311c1d040b2f751bfdef9c9d0f89ef
You're receiving this email because of your account on salsa.debian.org.

Reply via email to