Re: [xserver,1/4] os: move xf86PrivsElevated here

[Antoine Martin]

On 09/03/18 05:11, Ben Crocker wrote:
> Sent that a little too soon; please consider "Reviewed-by: Ben Crocker
> <bcroc...@redhat.com <mailto:bcroc...@redhat.com>>" to be
> added after each of the "Signed-off-by: Nicolai Haehnle
> <nicolai.haeh...@amd.com <mailto:nicolai.haeh...@amd.com>>" lines.
You can add:
Reviewed-by: Antoine Martin <anto...@nagafix.co.uk>

You should probably re-send the patch series as individual emails.

Cheers
Antoine


> 
> Also please note that I rebased Nicolai's original patch,
> https://patchwork.freedesktop.org/series/18684/,
> against a very recent copy of master.
> 
>   Thanks,
>   Ben
> 
> 
> On Thu, Mar 8, 2018 at 4:53 PM, Ben Crocker <bcroc...@redhat.com
> <mailto:bcroc...@redhat.com>> wrote:
> 
> From: Nicolai Hähnle <nhaeh...@gmail.com <mailto:nhaeh...@gmail.com>>
> 
> From: Nicolai Hähnle <nicolai.haeh...@amd.com
> <mailto:nicolai.haeh...@amd.com>>
> 
> Having different types of code all trying to check for elevated
> privileges
> is a bad idea. This implementation is the most thorough one.
> 
> Signed-off-by: Nicolai Hähnle <nicolai.haeh...@amd.com
> <mailto:nicolai.haeh...@amd.com>>
> ---
>  hw/xfree86/common/xf86Init.c | 59
> +
>  include/os.h                 |  3 +++
>  os/utils.c                   | 63
> 
>  3 files changed, 67 insertions(+), 58 deletions(-)
> 
> diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
> index e61fe66..758b926 100644
> --- a/hw/xfree86/common/xf86Init.c
> +++ b/hw/xfree86/common/xf86Init.c
> @@ -230,78 +230,21 @@ xf86PrintBanner(void)
>      xf86ErrorFVerb(0, "Current version of pixman: %s\n",
>                     pixman_version_string());
>      xf86ErrorFVerb(0, "\tBefore reporting problems, check "
>                     "" __VENDORDWEBSUPPORT__ "\n"
>                     "\tto make sure that you have the latest
> version.\n");
>  }
> 
>  Bool
>  xf86PrivsElevated(void)
>  {
> -    static Bool privsTested = FALSE;
> -    static Bool privsElevated = TRUE;
> -
> -    if (!privsTested) {
> -#if defined(WIN32)
> -        privsElevated = FALSE;
> -#else
> -        if ((getuid() != geteuid()) || (getgid() != getegid())) {
> -            privsElevated = TRUE;
> -        }
> -        else {
> -#if defined(HAVE_ISSETUGID)
> -            privsElevated = issetugid();
> -#elif defined(HAVE_GETRESUID)
> -            uid_t ruid, euid, suid;
> -            gid_t rgid, egid, sgid;
> -
> -            if ((getresuid(, , ) == 0) &&
> -                (getresgid(, , ) == 0)) {
> -                privsElevated = (euid != suid) || (egid != sgid);
> -            }
> -            else {
> -                printf("Failed getresuid or getresgid");
> -                /* Something went wrong, make defensive assumption */
> -                privsElevated = TRUE;
> -            }
> -#else
> -            if (getuid() == 0) {
> -                /* running as root: uid==euid==0 */
> -                privsElevated = FALSE;
> -            }
> -            else {
> -                /*
> -                 * If there are saved ID's the process might still
> be privileged
> -                 * even though the above test succeeded. If
> issetugid() and
> -                 * getresgid() aren't available, test this by
> trying to set
> -                 * euid to 0.
> -                 */
> -                unsigned int oldeuid;
> -
> -                oldeuid = geteuid();
> -
> -                if (seteuid(0) != 0) {
> -                    privsElevated = FALSE;
> -                }
> -                else {
> -                    if (seteuid(oldeuid) != 0) {
> -                        FatalError("Failed to drop privileges. 
> Exiting\n");
> -                    }
> -                    privsElevated = TRUE;
> -                }
> -            }
> -#endif
> -        }
> -#endif
> -        privsTested = TRUE;
> -    }
> -    return privsElevated;
> +    return PrivsElevated();
>  }
> 
>  static void
>  TrapSignals(void)
>  {
>      if (xf86Info.notrap

[PATCH xserver v2] fixing -logfile when used with -displayfd

[Antoine Martin]

Hi,

v2 updated with the feedback from Alan: the LogSetDisplay function is
the right place for this fix.

Trivial way to reproduce the bug:
Xorg -logfile /tmp/mylog -config /etc/xpra/xorg.conf -displayfd 2

The server then moans:
Failed to rename log file "/tmp/mylog" to "/tmp/mylog": No such file or
directory

And the log file is created but immediately renamed to "/tmp/mylog.old"

This is caused by the changes to the log file handling introduced by
this commit:
https://cgit.freedesktop.org/xorg/xserver/commit/?id=edcb6426f20c3be5dd5f50b76a686754aef2f64e

And below is the trivial fix for it. We only need to rename the logfile
if the log filename contains the magic substitution string "%s".

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
diff --git a/os/log.c b/os/log.c
index 91e55a532..a3b28ccb4 100644
--- a/os/log.c
+++ b/os/log.c
@@ -296,7 +296,7 @@ LogInit(const char *fname, const char *backup)
 void
 LogSetDisplay(void)
 {
-if (saved_log_fname) {
+if (saved_log_fname && strstr(saved_log_fname, "%s")) {
 char *logFileName;

 logFileName = LogFilePrep(saved_log_fname, saved_log_backup,
display);
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/7] glamor: Fix loose ends in color depth 30 support.

[Antoine Martin]

On 20/02/18 11:06, Mario Kleiner wrote:
>  /* XXX handle 2 10 10 10 and 1555 formats; presumably the pixmap private 
> knows this? */
Maybe this comment should now be updated?

Otherwise, everything looks straightforward, so for the whole series:
Reviewed-by: Antoine Martin <anto...@nagafix.co.uk>

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: fine grained scrolling via uinput [SOLVED - mostly]

[Antoine Martin]

(snip)

>> And it all seems to work pretty well so far. Applications like Firefox
>> can now scroll a few pixels at a time.
>>
>> Questions:
>> 1) Anything wrong with this approach?
> 
> well. on the face of it, it's fine. but remember, you're not triggering a
> behaviour directly, you're emulating a physical device with specific
> hardware properties that libinput currently treats in a specific way. Keep
> that in mind, as libinput's behaviour may change over time.
Noted.

> amongst other things, you're emulating a wheel which is handled differently
> in libinput than e.g. two-finger scrolling.
For two finger scrolling, we should be able to use a uinput touch
device, this is planned.

Mapping the multi-platform client side API events (X11, win32, macos,
javascript..) into uinput server events is going to be the "fun" part.

>> 2) what is the valid range for MOUSE_WHEEL_CLICK_COUNT?
>> If I set MOUSE_WHEEL_CLICK_COUNT too high, I get:
>> (EE) event19 - (EE) Xpra Virtual Pointer: (EE) mouse wheel click count
>> is present but invalid, using 15 degrees for angle instead instead
>> (and this error is logged twice)
> 
> see evdev_read_wheel_click_count_prop() in libinput/src/evdev.c.
> libinput assumes anything with <1 degre per click as invalid because no such
> devices currently exist (to my knowledge)
>  
>> 3) Despite setting a flat AccelProfile above, "libinput list-devices" shows:
>> Accel profiles:   flat *adaptive
>> What am I doing wrong here?
> 
> from libinput-list-devices(1)
>An xorg.conf(5) configuration entry or Wayland compositor setting
>may  have  changed  configurations  on  a device.  The  libinput
>list-devices tool only shows the device's default configuration, not
>the current configuration.
The Xorg log shows:
(**) Option "AccelProfile" "flat"
(**) Option "config_info" "udev:/sys/devices/virtual/input/input53/event20"
(II) XINPUT: Adding extended input device "Xpra Virtual Pointer" (type:
MOUSE, id 6)
(**) Option "AccelerationScheme" "none"
(**) Xpra Virtual Pointer: (accel) selected scheme none/0
(**) Xpra Virtual Pointer: (accel) acceleration factor: 2.000
(**) Xpra Virtual Pointer: (accel) acceleration threshold: 4
(..)
Despite requesting "AccelProfile" "flat", it looks like I am getting an
acceleration factor? How can I disable it so that a synthetic REL_X
event with a value X always corresponds to a motion of exactly X pixels?

$ xinput list-props 6
Device 'Xpra Virtual Pointer':
Device Enabled (114):   1
Coordinate Transformation Matrix (116): 1.00, 0.00,
0.00, 0.00, 1.00, 0.00, 0.00, 0.00, 1.00
libinput Accel Speed (244): 0.00
libinput Accel Speed Default (245): 0.00
libinput Accel Profiles Available (246):1, 1
libinput Accel Profile Enabled (247):   0, 1
libinput Accel Profile Enabled Default (248):   1, 0
(...)

>> 4) the first scroll event is always ignored - what can I do to fix that?
> 
> you can't. it's a long-standing XI2.1 protocol design flaw:
> http://who-t.blogspot.co.at/2012/06/xi-21-protocol-design-issues.html
It's an ugly workaround, but synthesizing a dummy REL_WHEEL event on
startup "fixes" things. (only enabled for XInput version older than 2.2
for now, in the hope that this will change)

>> 5) I tried using a udev hwdb.d rule for setting the click count, but
>> that didn't seem to have any effect:
>> cat > /lib/udev/hwdb.d/71-mouse-xpra.hwdb <> # allow xpra to use smooth scrolling
>> mouse:*:name:Xpra Virtual Pointer:
>>  MOUSE_WHEEL_CLICK_ANGLE=1
>>  MOUSE_WHEEL_CLICK_COUNT=360
>> EOF
>> udevadm hwdb --update
> 
> you did reboot or re-trigger the rules after that?
I thought I did.

>> Does it matter? Any reason to use this rather than the udev rule?
> 
> the effect of using udev rules and hwdb entries is the same. hwdb entries
> are easier to manage when you end up having a lot of them. I don't know
> specifically what's wrong here though, nothing immediately sticks out.
I'll stick with the plain udev rule then.

>> 6) to get the same motion as with an XTest wheel click, I'm having to
>> send REL_WHEEL=+-30, I would have thought it should be 15 instead.
>> Where did I get my maths wrong?
> 
> is that the first scroll event getting swallowed as above?
I don't think so. Will try again.

>> 7) when the uinput device is created, udev will chown it to root:input,
>> is there any way we can tell udev to just leave it alone?
>> We know the uid and gid we want to chown to, and currently we have to
>> wait for udev to do its thing then do what we really want, which is both
>> ugly and racy.
> 
> add a udev rule that sorts high enough and changes the mode. telling udev to
> leave it alone is generally harder than just overriding what it does with a
> higher-sorted rule. this can be part of your exisiting udev rule with
> MODE=...  and GROUP=...
I came up with a really ugly hack to ensure the correct uid is set when
the device 

Re: [PATCH v2 xf86-video-dummy] Add glamor acceleration which enables native OpenGL support (v2)

[Antoine Martin]

On 06/08/17 09:46, Yu, Qiang wrote:
> How do you run the xserver?
https://xpra.org/trac/wiki/Xdummy

> And do you have this file
> /dev/dri/renderD128
Yes.

> and right permission to access?
Yes.

As per ajax's reply, I've revised my reasonable-but-invalid expectations.
I'm primarily interested in 3D acceleration for client applications
running on an Xorg server with the dummy driver, whereas this patch set
accelerates the 2D rendering within the X server itself.

Please do keep me CCed on these patch sets, they will get tested - just
not necessarily in a timely manner..

Cheers
Antoine

> 
> Regards,
> Qiang
> ________
> From: Antoine Martin <anto...@nagafix.co.uk>
> Sent: Saturday, August 5, 2017 8:54:11 PM
> To: xorg-devel@lists.x.org; Yu, Qiang
> Subject: Re: [PATCH v2 xf86-video-dummy] Add glamor acceleration which 
> enables native OpenGL support (v2)
> 
> On 09/03/17 09:26, Qiang Yu wrote:
>> v2:
>>   Update configure.ac for auto enable glamor support and print
>>   corresponding error message when necessary.
>>
>> Enable glamor acceleration in xorg.conf by:
>> Section "Device"
>>   ...
>>   Driver "dummy"
>>   Option "Render" "/dev/dri/renderD128"
>>   ...
>> EndSection
> With an nvidia driver, I get:
> [119367.649] (II) Loading /usr/lib64/xorg/modules/libglamoregl.so
> [119367.649] (II) Module glamoregl: vendor="X.Org Foundation"
> [119367.649]compiled for 1.19.3, module version = 1.0.0
> [119367.649]ABI class: X.Org ANSI C Emulation, version 0.4
> [119367.649] (II) glamor: OpenGL accelerated X.org driver based.
> [119367.654] (II) glamor: EGL version 1.4 (DRI2):
> [119367.654] EGL_MESA_drm_image required.
> [119367.654] (EE) DUMMY(0): glamor initialization failed
> 
> And with modesetting or intel drivers:
> [   738.822] (II) glamor: OpenGL accelerated X.org driver based.
> [   738.827] (II) glamor: EGL version 1.4 (DRI2):
> [   738.836] (II) DUMMY(0): glamor initialized
> [   738.836] (--) Depth 24 pixmap format is 32 bpp
> [   738.957] (II) DUMMY(0): Using 3904 scanlines of offscreen memory
> [   738.957] (==) DUMMY(0): Backing store enabled
> [   738.957] (==) DUMMY(0): Silken mouse enabled
> [   738.957] (==) RandR enabled
> [   738.959] (II) SELinux: Disabled by boolean
> [   738.960] (II) AIGLX: Screen 0 is not DRI2 capable
> [   738.960] (EE) AIGLX: reverting to software rendering
> 
> And in both cases, glxinfo reports that my dummy screen uses software
> rendering. (Accelerated: no, Gallium 0.4 on llvmpipe..)
> 
> What am I doing wrong?
> 
> Cheers
> Antoine
> 
> 
>>
>> GPU is chosen by the Render option which specifies the render
>> node of the GPU DRM device.
>>
>> We could use the dumb buffer instead of gbm buffer as the
>> screen front buffer. But dumb buffer requires open
>> /dev/dri/cardx which has the limitation of only one instance
>> of OpenGL enabled xserver can start.
>>
>> With gbm buffer, we can use /dev/dri/renderDx which has no
>> limitation on the number of OpenGL enabled xserver and even
>> won't conflict with a "real" xserver using radeon/amdgpu DDX.
>>
>> Due to using renderDx, only DRI3 OpenGL is supported.
>>
>> DGA is disabled when glamor is enabled, we can enable it with
>> new gbm_bo_map/unmap API, but consider a more effiction way
>> is just using DRI3BufferFromPixmap for the root window pixmap.
>>
>> Signed-off-by: Qiang Yu <qiang...@amd.com>
>> ---
>>  configure.ac   |  38 ++
>>  src/Makefile.am|   7 ++-
>>  src/dummy.h|   9 
>>  src/dummy_dga.c|   3 ++
>>  src/dummy_driver.c | 143 
>> -
>>  5 files changed, 188 insertions(+), 12 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 4eb7fae..39d2157 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -68,6 +68,44 @@ AM_CONDITIONAL([DGA], [test "x$DGA" = xyes])
>>  # Obtain compiler/linker options for the driver dependencies
>>  PKG_CHECK_MODULES(XORG, [xorg-server >= 1.4.99.901] xproto fontsproto 
>> $REQUIRED_MODULES)
>>
>> +# Check glamor support
>> +AC_ARG_ENABLE(glamor,
>> +   AS_HELP_STRING([--disable-glamor],
>> +  [Disable glamor, a new GL-based acceleration 
>> [default=auto]]),
>> +   [GLAMOR="$enableval"],
>> +   [GLAMOR=auto])
>> +
>> +SAVE_CPPFLAGS="$CPPFLAGS"
>> +CPPFLAGS="$CPPFLAGS $XORG_CFLAGS"
>&

Re: [PATCH] fixing -logfile when used with -displayfd

[Antoine Martin]

On 28/08/17 13:02, Alan Coopersmith wrote:
> I admit not looking at this code in a year and a half and barely
> remembering
> what it does, but that seems like the wrong place to add the check -
> wouldn't
> it make more sense to change
> if (displayfd != -1) {
> to
> if ((displayfd != -1) && strstr(saved_log_fname, "%s")) {
> in LogInit, so that the saved_log_* variables never get set in the first
> place?
You're right, this is a much more logical place to patch.
Only you need to call strstr on "fname" here, like so:


@@ -247,7 +247,7 @@ LogInit(const char *fname, const char *backup)
 char *logFileName = NULL;

 if (fname && *fname) {
-if (displayfd != -1) {
+if (displayfd != -1 && strstr(fname, "%s")) {
 /* Display isn't set yet, so we can't use it in filenames
yet. */
 char pidstring[32];
 snprintf(pidstring, sizeof(pidstring), "pid-%ld",


Antoine


> 
> -alan-
> 
> On 08/27/17 10:21 PM, Antoine Martin wrote:
>> Bump. This is a cosmetic bug, but it trips up many people when they
>> cannot find the log file that they had requested.
>>
>> This should be applied to older branches too since this particular bug
>> was also applied to 1.18.1 (a fairly big change too, especially compared
>> to this one liner fix for it)
>>
>> Cheers
>> Antoine
>>
>>
>> On 05/08/17 17:09, Antoine Martin wrote:
>>> Hi,
>>>
>>> Trivial way to reproduce the bug:
>>> Xorg -logfile /tmp/mylog -config /etc/xpra/xorg.conf -displayfd 2
>>>
>>> The server then moans:
>>> Failed to rename log file "/tmp/mylog" to "/tmp/mylog": No such file or
>>> directory
>>>
>>> And the log file is created, but immediately renamed to "/tmp/mylog.old"
>>>
>>> This is caused by the changes to the log file handling introduced by
>>> this commit:
>>> https://cgit.freedesktop.org/xorg/xserver/commit/?id=edcb6426f20c3be5dd5f50b76a686754aef2f64e
>>>
>>> And below is the trivial fix for it.
>>>
>>> ---
>>> Don't try to add the pidstring to the log filename if it doesn't contain
>>> the "%s" placeholder for it.
>>>
>>> Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
>>> ---
>>> diff --git a/os/log.c b/os/log.c
>>> index 91e55a532..a3b28ccb4 100644
>>> --- a/os/log.c
>>> +++ b/os/log.c
>>> @@ -296,7 +296,7 @@ LogInit(const char *fname, const char *backup)
>>>   void
>>>   LogSetDisplay(void)
>>>   {
>>> -    if (saved_log_fname) {
>>> +    if (saved_log_fname && strstr(saved_log_fname, "%s")) {
>>>   char *logFileName;
>>>
>>>   logFileName = LogFilePrep(saved_log_fname, saved_log_backup,
>>> display);
>>>
>>
> 
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] fixing -logfile when used with -displayfd

[Antoine Martin]

Bump. This is a cosmetic bug, but it trips up many people when they
cannot find the log file that they had requested.

This should be applied to older branches too since this particular bug
was also applied to 1.18.1 (a fairly big change too, especially compared
to this one liner fix for it)

Cheers
Antoine


On 05/08/17 17:09, Antoine Martin wrote:
> Hi,
> 
> Trivial way to reproduce the bug:
> Xorg -logfile /tmp/mylog -config /etc/xpra/xorg.conf -displayfd 2
> 
> The server then moans:
> Failed to rename log file "/tmp/mylog" to "/tmp/mylog": No such file or
> directory
> 
> And the log file is created, but immediately renamed to "/tmp/mylog.old"
> 
> This is caused by the changes to the log file handling introduced by
> this commit:
> https://cgit.freedesktop.org/xorg/xserver/commit/?id=edcb6426f20c3be5dd5f50b76a686754aef2f64e
> And below is the trivial fix for it.
> 
> ---
> Don't try to add the pidstring to the log filename if it doesn't contain
> the "%s" placeholder for it.
> 
> Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
> ---
> diff --git a/os/log.c b/os/log.c
> index 91e55a532..a3b28ccb4 100644
> --- a/os/log.c
> +++ b/os/log.c
> @@ -296,7 +296,7 @@ LogInit(const char *fname, const char *backup)
>  void
>  LogSetDisplay(void)
>  {
> -if (saved_log_fname) {
> +if (saved_log_fname && strstr(saved_log_fname, "%s")) {
>  char *logFileName;
> 
>  logFileName = LogFilePrep(saved_log_fname, saved_log_backup,
> display);
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

fine grained scrolling via uinput [SOLVED - mostly]

[Antoine Martin]

Hi,

We've got a solution to the "smooth scrolling" query from last year:
https://lists.freedesktop.org/archives/xorg/2016-June/058138.html

I believe that this solution is actually easier than implementing a
trackstick-like device: we adjust MOUSE_WHEEL_CLICK_COUNT on a uinput
device to give us 1 degree resolution, then multiply at runtime if we
need the "regular" 15 degree events.

There are quite a few places that need configuring to make this work:

1) ensure that those devices won't be picked up by real Xorg servers:
cat > /etc/X11/xorg.conf.d/90-xpra-virtual.conf << EOF
# Ignore all xpra virtual devices by default,
# these will be enabled explicitly when needed.
Section "InputClass"
Identifier "xpra-virtual-device"
MatchProduct "Xpra"
Option "Ignore" "true"
EndSection
EOF

2) Tell udev to add our click count setting:
cat > /lib/udev/rules.d/71-xpra-mouse.rules << EOF
ACTION=="add|change", ATTRS{name}=="Xpra Virtual Pointer", \
ENV{MOUSE_WHEEL_CLICK_ANGLE}="1", ENV{MOUSE_WHEEL_CLICK_COUNT}="360"
EOF
udevadm control --reload-rules && udevadm trigger

3) to be able to use uinput devices, xpra's xorg.conf has to enable
"AutoEnableDevices" and "AutoAddDevices", and so we also have to tell it
to ignore all the devices except the ones we will assign to it:
Section "InputClass"
  Identifier "ignore-non-xpra-devices"
  NoMatchProduct "Xpra"
  Option "Ignore" "true"
EndSection

4) When starting a dummy Xorg server, we create a uinput device with
REL_WHEEL and BTN_* (as root so we can access /dev/uinput), then figure
out what path it's available under using UI_GET_SYSNAME and add this
section:
Section "InputClass"
Identifier "xpra-virtual-pointer"
MatchProduct "Xpra Virtual Pointer"
MatchDevicePath "/dev/input/eventNNN"
Option "Ignore" "False"
MatchIsPointer "True"
Driver "libinput"
Option "AccelProfile" "flat"
EndSection
(the Xorg server doesn't run as root, we can drop privileges after
dealing with uinput and chowning the new device)

5) To send scroll events, we send these events to the uinput device:
REL_WHEEL, 30
or
REL_WHEEL, -30

--

And it all seems to work pretty well so far. Applications like Firefox
can now scroll a few pixels at a time.

Questions:
1) Anything wrong with this approach?

2) what is the valid range for MOUSE_WHEEL_CLICK_COUNT?
If I set MOUSE_WHEEL_CLICK_COUNT too high, I get:
(EE) event19 - (EE) Xpra Virtual Pointer: (EE) mouse wheel click count
is present but invalid, using 15 degrees for angle instead instead
(and this error is logged twice)

3) Despite setting a flat AccelProfile above, "libinput list-devices" shows:
Accel profiles:   flat *adaptive
What am I doing wrong here?

4) the first scroll event is always ignored - what can I do to fix that?

5) I tried using a udev hwdb.d rule for setting the click count, but
that didn't seem to have any effect:
cat > /lib/udev/hwdb.d/71-mouse-xpra.hwdb 

Re: [PATCH v2 xf86-video-dummy] Add glamor acceleration which enables native OpenGL support (v2)

[Antoine Martin]

On 06/08/17 09:46, Yu, Qiang wrote:
> How do you run the xserver?
I usually start it via xpra, which runs something like this (on Fedora):

/usr/libexec/Xorg -noreset -novtswitch -nolisten tcp \
+extension GLX +extension RANDR +extension RENDER \
   -auth $HOME/.Xauthority -logfile $XDG_RUNTIME_DIR/xpra/Xorg.10.log \
   -config /etc/xpra/xorg.conf -depth 24

And starting it directly without xpra makes no difference.
The xorg.conf we use can be found here:
http://xpra.org/svn/Xpra/trunk/src/etc/xpra/xorg.conf
To which I added:
  Option "Render" "/dev/dri/renderD128"

> And do you have this file
> /dev/dri/renderD128
> and right permission to access?
Yes, I chmoded it:
crw-rw-rw-+ 1 root video 226, 128 Aug  2 17:04 /dev/dri/renderD128

Cheers
Antoine


> 
> Regards,
> Qiang
> ________
> From: Antoine Martin <anto...@nagafix.co.uk>
> Sent: Saturday, August 5, 2017 8:54:11 PM
> To: xorg-devel@lists.x.org; Yu, Qiang
> Subject: Re: [PATCH v2 xf86-video-dummy] Add glamor acceleration which 
> enables native OpenGL support (v2)
> 
> On 09/03/17 09:26, Qiang Yu wrote:
>> v2:
>>   Update configure.ac for auto enable glamor support and print
>>   corresponding error message when necessary.
>>
>> Enable glamor acceleration in xorg.conf by:
>> Section "Device"
>>   ...
>>   Driver "dummy"
>>   Option "Render" "/dev/dri/renderD128"
>>   ...
>> EndSection
> With an nvidia driver, I get:
> [119367.649] (II) Loading /usr/lib64/xorg/modules/libglamoregl.so
> [119367.649] (II) Module glamoregl: vendor="X.Org Foundation"
> [119367.649]compiled for 1.19.3, module version = 1.0.0
> [119367.649]ABI class: X.Org ANSI C Emulation, version 0.4
> [119367.649] (II) glamor: OpenGL accelerated X.org driver based.
> [119367.654] (II) glamor: EGL version 1.4 (DRI2):
> [119367.654] EGL_MESA_drm_image required.
> [119367.654] (EE) DUMMY(0): glamor initialization failed
> 
> And with modesetting or intel drivers:
> [   738.822] (II) glamor: OpenGL accelerated X.org driver based.
> [   738.827] (II) glamor: EGL version 1.4 (DRI2):
> [   738.836] (II) DUMMY(0): glamor initialized
> [   738.836] (--) Depth 24 pixmap format is 32 bpp
> [   738.957] (II) DUMMY(0): Using 3904 scanlines of offscreen memory
> [   738.957] (==) DUMMY(0): Backing store enabled
> [   738.957] (==) DUMMY(0): Silken mouse enabled
> [   738.957] (==) RandR enabled
> [   738.959] (II) SELinux: Disabled by boolean
> [   738.960] (II) AIGLX: Screen 0 is not DRI2 capable
> [   738.960] (EE) AIGLX: reverting to software rendering
> 
> And in both cases, glxinfo reports that my dummy screen uses software
> rendering. (Accelerated: no, Gallium 0.4 on llvmpipe..)
> 
> What am I doing wrong?
> 
> Cheers
> Antoine
> 
> 
>>
>> GPU is chosen by the Render option which specifies the render
>> node of the GPU DRM device.
>>
>> We could use the dumb buffer instead of gbm buffer as the
>> screen front buffer. But dumb buffer requires open
>> /dev/dri/cardx which has the limitation of only one instance
>> of OpenGL enabled xserver can start.
>>
>> With gbm buffer, we can use /dev/dri/renderDx which has no
>> limitation on the number of OpenGL enabled xserver and even
>> won't conflict with a "real" xserver using radeon/amdgpu DDX.
>>
>> Due to using renderDx, only DRI3 OpenGL is supported.
>>
>> DGA is disabled when glamor is enabled, we can enable it with
>> new gbm_bo_map/unmap API, but consider a more effiction way
>> is just using DRI3BufferFromPixmap for the root window pixmap.
>>
>> Signed-off-by: Qiang Yu <qiang...@amd.com>
>> ---
>>  configure.ac   |  38 ++
>>  src/Makefile.am|   7 ++-
>>  src/dummy.h|   9 
>>  src/dummy_dga.c|   3 ++
>>  src/dummy_driver.c | 143 
>> -
>>  5 files changed, 188 insertions(+), 12 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 4eb7fae..39d2157 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -68,6 +68,44 @@ AM_CONDITIONAL([DGA], [test "x$DGA" = xyes])
>>  # Obtain compiler/linker options for the driver dependencies
>>  PKG_CHECK_MODULES(XORG, [xorg-server >= 1.4.99.901] xproto fontsproto 
>> $REQUIRED_MODULES)
>>
>> +# Check glamor support
>> +AC_ARG_ENABLE(glamor,
>> +   AS_HELP_STRING([--disable-glamor],
>> +  [Disable glamor, a new GL-based acceleration 
>> [default=auto]]),
>> +

Re: xf86-video-dummy patch series - was "Cleanups Redux"

[Antoine Martin]

On 06/08/17 07:16, Aaron Plattner wrote:
> On 08/05/2017 08:02 AM, Antoine Martin wrote:
>> On 17/12/16 02:04, Emil Velikov wrote:
>>> Hi Bob,
>>>
>>> On 9 December 2016 at 22:25, Bob Terek <x...@esoterek.com> wrote:
>>>>
>>>> On 12/09/2016 03:13 AM, Emil Velikov wrote:
>>>>
>>>>> On 6 December 2016 at 22:41, Bob Terek <x...@esoterek.com> wrote:
>>>>>>
>>>>>> Resubmitting some of Aaron Plattner's cleanup patches to
>>>>>> xf86-video-dummy:
>>>>>>
>>>>>>https://lists.x.org/archives/xorg-devel/2015-January/045395.html
>>>>>> . . .
>>>>>> . . .
>>>>
>>>>
>>>>> Afaict the patches are literally unchanged since Aaron's submit. As
>>>>> such changing the authorship is a bit ... bad.
>>>>
>>>>
>>>> Oops, sorry for the breach in protocol. Do I need to do something specific
>>>> to "withdraw" the patch I sent? Should I do something at Patchwork?
>>>>
>> I've restored the "author" and added Bob as "Reviewed-by" instead.
>>
>>> Updating patchwork would be very good, indeed.
>>>
>>>> I'm going to submit an alternative approach to Aaron's 6/6, and was going
>>>> to include the remaining cleanups, but then it was thought the cleanups
>>>> should be addressed separately. So then for some reason I thought they
>>>> needed to be submitted again, against the current version. . .
>>>>
>>> Agreed.
>> These patches have been reviewed and tested quite a few times now.
>> I've just created a git repo with all the uncontroversial pending
>> changes to the dummy driver, 6 so far:
>> https://github.com/totaam/xf86-video-dummy/commits/master
>> What else can I do to help move this along?
> 
> I've pushed all of these except "support for 30 bit depth in dummy driver". I 
> want to check that changing the colormap size to 1024 for depth 24 doesn't 
> change behavior.
Thanks!

FWIW: I am reasonably confident that the 30-bit patch is safe as we've
been shipping it in the Fedora, CentOS, Debian and Ubuntu repositories
for a while. If X11 applications were suddenly to break because of it, I
would have heard the complaints by now.

Antoine


> 
> remote: Updating patchwork state for 
> https://patchwork.freedesktop.org/project/Xorg/list/
> remote: I: patch #41059 updated using rev 
> 12e3e2030171b7a5df074a56293eb16da40cd99b.
> remote: I: patch #41060 updated using rev 
> 7c3b090e80a9b364434120262f9bef5686cd2e2e.
> remote: E: failed to find patch for rev 
> 87249af5faf85c8d093e910c069faa4db0aee843.
> remote: I: patch #125912 updated using rev 
> 33e68185665b2d065525ac03332f080026b18d8d.
> remote: I: patch #168436 updated using rev 
> 7957ad83b53b57f376164b10742d4e35223c9dcc.
> remote: E: failed to find patch for rev 
> 5e90221dc68ae0893acd5c9b12d702269202558d.
> remote: I: 4 patch(es) updated to state Accepted.
> To git.freedesktop.org:/git/xorg/driver/xf86-video-dummy
>af0f808922f2..5e90221dc68a  master -> master
> 
> 
>> Thanks
>> Antoine
>>
>>
>>
>>
>>>
>>> Thanks
>>> Emil
>>> ___
>>> xorg-devel@lists.x.org: X.Org development
>>> Archives: http://lists.x.org/archives/xorg-devel
>>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>>>
>>
>> ___
>> xorg-devel@lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: https://lists.x.org/mailman/listinfo/xorg-devel
>>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

xf86-video-dummy patch series - was "Cleanups Redux"

[Antoine Martin]

On 17/12/16 02:04, Emil Velikov wrote:
> Hi Bob,
> 
> On 9 December 2016 at 22:25, Bob Terek  wrote:
>>
>> On 12/09/2016 03:13 AM, Emil Velikov wrote:
>>
>>> On 6 December 2016 at 22:41, Bob Terek  wrote:

 Resubmitting some of Aaron Plattner's cleanup patches to
 xf86-video-dummy:

   https://lists.x.org/archives/xorg-devel/2015-January/045395.html
 . . .
 . . .
>>
>>
>>> Afaict the patches are literally unchanged since Aaron's submit. As
>>> such changing the authorship is a bit ... bad.
>>
>>
>> Oops, sorry for the breach in protocol. Do I need to do something specific
>> to "withdraw" the patch I sent? Should I do something at Patchwork?
>>
I've restored the "author" and added Bob as "Reviewed-by" instead.

> Updating patchwork would be very good, indeed.
> 
>> I'm going to submit an alternative approach to Aaron's 6/6, and was going
>> to include the remaining cleanups, but then it was thought the cleanups
>> should be addressed separately. So then for some reason I thought they
>> needed to be submitted again, against the current version. . .
>>
> Agreed.
These patches have been reviewed and tested quite a few times now.
I've just created a git repo with all the uncontroversial pending
changes to the dummy driver, 6 so far:
https://github.com/totaam/xf86-video-dummy/commits/master
What else can I do to help move this along?

Thanks
Antoine




> 
> Thanks
> Emil
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2 xf86-video-dummy] Add glamor acceleration which enables native OpenGL support (v2)

[Antoine Martin]

On 09/03/17 09:26, Qiang Yu wrote:
> v2:
>   Update configure.ac for auto enable glamor support and print
>   corresponding error message when necessary.
> 
> Enable glamor acceleration in xorg.conf by:
> Section "Device"
>   ...
>   Driver "dummy"
>   Option "Render" "/dev/dri/renderD128"
>   ...
> EndSection
With an nvidia driver, I get:
[119367.649] (II) Loading /usr/lib64/xorg/modules/libglamoregl.so
[119367.649] (II) Module glamoregl: vendor="X.Org Foundation"
[119367.649]compiled for 1.19.3, module version = 1.0.0
[119367.649]ABI class: X.Org ANSI C Emulation, version 0.4
[119367.649] (II) glamor: OpenGL accelerated X.org driver based.
[119367.654] (II) glamor: EGL version 1.4 (DRI2):
[119367.654] EGL_MESA_drm_image required.
[119367.654] (EE) DUMMY(0): glamor initialization failed

And with modesetting or intel drivers:
[   738.822] (II) glamor: OpenGL accelerated X.org driver based.
[   738.827] (II) glamor: EGL version 1.4 (DRI2):
[   738.836] (II) DUMMY(0): glamor initialized
[   738.836] (--) Depth 24 pixmap format is 32 bpp
[   738.957] (II) DUMMY(0): Using 3904 scanlines of offscreen memory
[   738.957] (==) DUMMY(0): Backing store enabled
[   738.957] (==) DUMMY(0): Silken mouse enabled
[   738.957] (==) RandR enabled
[   738.959] (II) SELinux: Disabled by boolean
[   738.960] (II) AIGLX: Screen 0 is not DRI2 capable
[   738.960] (EE) AIGLX: reverting to software rendering

And in both cases, glxinfo reports that my dummy screen uses software
rendering. (Accelerated: no, Gallium 0.4 on llvmpipe..)

What am I doing wrong?

Cheers
Antoine


> 
> GPU is chosen by the Render option which specifies the render
> node of the GPU DRM device.
> 
> We could use the dumb buffer instead of gbm buffer as the
> screen front buffer. But dumb buffer requires open
> /dev/dri/cardx which has the limitation of only one instance
> of OpenGL enabled xserver can start.
> 
> With gbm buffer, we can use /dev/dri/renderDx which has no
> limitation on the number of OpenGL enabled xserver and even
> won't conflict with a "real" xserver using radeon/amdgpu DDX.
> 
> Due to using renderDx, only DRI3 OpenGL is supported.
> 
> DGA is disabled when glamor is enabled, we can enable it with
> new gbm_bo_map/unmap API, but consider a more effiction way
> is just using DRI3BufferFromPixmap for the root window pixmap.
> 
> Signed-off-by: Qiang Yu 
> ---
>  configure.ac   |  38 ++
>  src/Makefile.am|   7 ++-
>  src/dummy.h|   9 
>  src/dummy_dga.c|   3 ++
>  src/dummy_driver.c | 143 
> -
>  5 files changed, 188 insertions(+), 12 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 4eb7fae..39d2157 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -68,6 +68,44 @@ AM_CONDITIONAL([DGA], [test "x$DGA" = xyes])
>  # Obtain compiler/linker options for the driver dependencies
>  PKG_CHECK_MODULES(XORG, [xorg-server >= 1.4.99.901] xproto fontsproto 
> $REQUIRED_MODULES)
>  
> +# Check glamor support
> +AC_ARG_ENABLE(glamor,
> +   AS_HELP_STRING([--disable-glamor],
> +  [Disable glamor, a new GL-based acceleration 
> [default=auto]]),
> +   [GLAMOR="$enableval"],
> +   [GLAMOR=auto])
> +
> +SAVE_CPPFLAGS="$CPPFLAGS"
> +CPPFLAGS="$CPPFLAGS $XORG_CFLAGS"
> +if test "x$GLAMOR" != "xno"; then
> + AC_CHECK_HEADERS([glamor.h], [HAS_GLAMOR=yes], [HAS_GLAMOR=no], 
> [#include "xorg-server.h"])
> + PKG_CHECK_MODULES(GBM, [gbm], [], [HAS_GLAMOR=no])
> +fi
> +CPPFLAGS="$SAVE_CPPFLAGS"
> +
> +case "$GLAMOR,$HAS_GLAMOR" in
> + yes,yes | auto,yes)
> + GLAMOR=yes
> + ;;
> + yes,no)
> + AC_MSG_ERROR([GLAMOR requested, but glamor and/or gbm not 
> found.])
> + ;;
> + no,*)
> + ;;
> + *)
> + AC_MSG_NOTICE([GLAMOR disabled because glamor and/or gbm not 
> found.])
> + GLAMOR=no
> + ;;
> +esac
> +
> +AC_MSG_CHECKING([whether to include GLAMOR support])
> +AC_MSG_RESULT([$GLAMOR])
> +
> +if test "x$GLAMOR" != "xno"; then
> +AC_DEFINE(USE_GLAMOR, 1, [Enable glamor acceleration])
> +fi
> +AM_CONDITIONAL(GLAMOR, test x$GLAMOR != xno)
> +
>  # Checks for libraries.
>  
>  
> diff --git a/src/Makefile.am b/src/Makefile.am
> index da1dd9a..9faa9c4 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -25,7 +25,7 @@
>  # _ladir passes a dummy rpath to libtool so the thing will actually link
>  # TODO: -nostdlib/-Bstatic/-lgcc platform magic, not installing the .a, etc.
>  
> -AM_CFLAGS = $(XORG_CFLAGS) $(PCIACCESS_CFLAGS)
> +AM_CFLAGS = $(XORG_CFLAGS)
>  
>  dummy_drv_la_LTLIBRARIES = dummy_drv.la
>  dummy_drv_la_LDFLAGS = -module -avoid-version
> @@ -38,6 +38,11 @@ dummy_drv_la_SOURCES = \
>   dummy_driver.c \
>   dummy.h
>  
> +if GLAMOR
> +AM_CFLAGS += $(GBM_CFLAGS)
> +dummy_drv_la_LIBADD += $(GBM_LIBS)
> +endif
> +
>  if DGA
>  

[PATCH] fixing -logfile when used with -displayfd

[Antoine Martin]

Hi,

Trivial way to reproduce the bug:
Xorg -logfile /tmp/mylog -config /etc/xpra/xorg.conf -displayfd 2

The server then moans:
Failed to rename log file "/tmp/mylog" to "/tmp/mylog": No such file or
directory

And the log file is created, but immediately renamed to "/tmp/mylog.old"

This is caused by the changes to the log file handling introduced by
this commit:
https://cgit.freedesktop.org/xorg/xserver/commit/?id=edcb6426f20c3be5dd5f50b76a686754aef2f64e
And below is the trivial fix for it.

---
Don't try to add the pidstring to the log filename if it doesn't contain
the "%s" placeholder for it.

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
diff --git a/os/log.c b/os/log.c
index 91e55a532..a3b28ccb4 100644
--- a/os/log.c
+++ b/os/log.c
@@ -296,7 +296,7 @@ LogInit(const char *fname, const char *backup)
 void
 LogSetDisplay(void)
 {
-if (saved_log_fname) {
+if (saved_log_fname && strstr(saved_log_fname, "%s")) {
 char *logFileName;

 logFileName = LogFilePrep(saved_log_fname, saved_log_backup,
display);
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] support for 30 bit depth in dummy driver

[Antoine Martin]

Ping. Please apply.

This patch has been used for almost a year now, and got tested more
thoroughly lately with a patch to VirtualGL. To test it with xpra 2.1:
xpra start --pixel-depth 30 --start=xterm --attach=yes

Thanks
Antoine



Here's an updated version with clean offsets against current HEAD:

diff --git a/src/dummy.h b/src/dummy.h
index c3fdd6e..0dd8906 100644
--- a/src/dummy.h
+++ b/src/dummy.h
@@ -68,7 +68,7 @@ typedef struct dummyRec
 int overlay_offset;
 int videoKey;
 int interlace;
-dummy_colors colors[256];
+dummy_colors colors[1024];
 pointer* FBBase;
 Bool(*CreateWindow)() ; /* wrapped CreateWindow */
 Bool prop;
diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index 2656602..c64c60f 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -307,6 +307,7 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
case 15:
case 16:
case 24:
+case 30:
break;
default:
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
@@ -321,8 +322,8 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
pScrn->rgbBits = 8;

 /* Get the depth24 pixmap format */
-if (pScrn->depth == 24 && pix24bpp == 0)
-   pix24bpp = xf86GetBppFromDepth(pScrn, 24);
+if (pScrn->depth >= 24 && pix24bpp == 0)
+   pix24bpp = xf86GetBppFromDepth(pScrn, pScrn->depth);

 /*
  * This must happen after pScrn->display has been set because
@@ -612,7 +613,7 @@ DUMMYScreenInit(SCREEN_INIT_ARGS_DECL)
 if(!miCreateDefColormap(pScreen))
return FALSE;

-if (!xf86HandleColormaps(pScreen, 256, pScrn->rgbBits,
+if (!xf86HandleColormaps(pScreen, 1024, pScrn->rgbBits,
  DUMMYLoadPalette, NULL,
  CMAP_PALETTED_TRUECOLOR
 | CMAP_RELOAD_ON_MODE_SWITCH))




On 20/09/16 22:45, Antoine Martin wrote:
> This patch makes it possible to start the server using the dummy
> driver with a 30 bit depth.
> The colormap size is changed from 256 to 1024 to prevent crashes.
> 
> This updated patch adds the missing header file to the commit.
> 
> Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
> ---
>  src/dummy.h| 2 +-
>  src/dummy_driver.c | 7 ---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/dummy.h b/src/dummy.h
> index c3fdd6e..0dd8906 100644
> --- a/src/dummy.h
> +++ b/src/dummy.h
> @@ -68,7 +68,7 @@ typedef struct dummyRec
>  int overlay_offset;
>  int videoKey;
>  int interlace;
> -dummy_colors colors[256];
> +dummy_colors colors[1024];
>  pointer* FBBase;
>  Bool(*CreateWindow)() ; /* wrapped CreateWindow */
>  Bool prop;
> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
> index 737f11c..c84000f 100644
> --- a/src/dummy_driver.c
> +++ b/src/dummy_driver.c
> @@ -313,6 +313,7 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
>   case 15:
>   case 16:
>   case 24:
> +case 30:
>   break;
>   default:
>   xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
> @@ -327,8 +328,8 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
>   pScrn->rgbBits = 8;
>  
>  /* Get the depth24 pixmap format */
> -if (pScrn->depth == 24 && pix24bpp == 0)
> - pix24bpp = xf86GetBppFromDepth(pScrn, 24);
> +if (pScrn->depth >= 24 && pix24bpp == 0)
> + pix24bpp = xf86GetBppFromDepth(pScrn, pScrn->depth);
>  
>  /*
>   * This must happen after pScrn->display has been set because
> @@ -637,7 +638,7 @@ DUMMYScreenInit(SCREEN_INIT_ARGS_DECL)
>  if(!miCreateDefColormap(pScreen))
>   return FALSE;
>  
> -if (!xf86HandleColormaps(pScreen, 256, pScrn->rgbBits,
> +if (!xf86HandleColormaps(pScreen, 1024, pScrn->rgbBits,
>   DUMMYLoadPalette, NULL, 
>   CMAP_PALETTED_TRUECOLOR 
>| CMAP_RELOAD_ON_MODE_SWITCH))
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-dummy] fix a memory leak in probe

[Antoine Martin]

Reviewed by: Antoine Martin <anto...@nagafix.co.uk>

This ptr should be freed! As per the docstrings of xf86MatchDevice:
""
  The function allocates an array of GDevPtr and
   * returns this via sectlist and returns the number of elements in
   * this list as return value.
""
And other places do the same, ie: xf86platformProbeDev



On 22/07/17 19:43, Xiaolei Yu wrote:
> 
> Signed-off-by: Xiaolei Yu <dreifachst...@gmail.com>
> ---
>  src/dummy_driver.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
> index 2656602..5c5f00f 100644
> --- a/src/dummy_driver.c
> +++ b/src/dummy_driver.c
> @@ -263,6 +263,9 @@ DUMMYProbe(DriverPtr drv, int flags)
>   }
>   }
>  }
> +
> +free(devSections);
> +
>  return foundScreen;
>  }
>  
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: twm deadlocks the server

[Antoine Martin]

On 14/07/17 07:49, Aaron Plattner wrote:
> On 07/03/2017 10:41 PM, Antoine Martin wrote:
>> Hi,
>>
>> I've just come across this easy DoS with twm and
>> X.Org X Server 1.19.3 from Fedora 26.
>>
>> Steps:
>> /usr/libexec/Xorg -noreset -novtswitch -config /etc/xpra/xorg.conf :10&
>> #verify we can access the display:
>> DISPLAY=:10 xprop -root
>> #start xterm so we have a window, then twm:
>> DISPLAY=:10 xterm&
>> DISPLAY=:10 twm&
>> #now click on the title bar of the xterm:
>> DISPLAY=:10 xdotool mousemove 90 10 mousedown 1
>> #and now the X11 server is inaccessible:
>> DISPLAY=:10 xprop -root
> 
> Isn't this just twm grabbing the server? If you can connect to the
> server to launch twm, you can also just call XGrabServer() and not let go.
Indeed it is - thanks for the reminder.
Killing twm is the only option at that point.

AFAICT, the only way we can solve this problem in xpra is to use virtual
input devices instead of XTest.

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

twm deadlocks the server

[Antoine Martin]

Hi,

I've just come across this easy DoS with twm and
X.Org X Server 1.19.3 from Fedora 26.

Steps:
/usr/libexec/Xorg -noreset -novtswitch -config /etc/xpra/xorg.conf :10&
#verify we can access the display:
DISPLAY=:10 xprop -root
#start xterm so we have a window, then twm:
DISPLAY=:10 xterm&
DISPLAY=:10 twm&
#now click on the title bar of the xterm:
DISPLAY=:10 xdotool mousemove 90 10 mousedown 1
#and now the X11 server is inaccessible:
DISPLAY=:10 xprop -root

Cheers
Antoine

PS: the xorg.conf to use for the dummy driver can be found here:
http://xpra.org/trac/browser/xpra/tags/v2.0.x/src/etc/xpra/xorg.conf
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

voting confirmation

[Antoine Martin]

Hi,

Apologies for sending an image link to this list, but I can't think of a
better way to explain the somewhat confusing page that showed up after I
cast my ballot:
http://imgur.com/r8nSblg
Did it take my vote into account? Did I actually vote? And not for "".
I had agreed to the changed membership agreement, but again it shows "".

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Virtual Touch Events for XTEST

[Antoine Martin]

On 23/02/17 09:59, Peter Hutterer wrote:
> On Wed, Feb 22, 2017 at 12:25:07PM +0530, Prakash P wrote:
>> ​​
>> Hi,
>>
>> I want to create an virtual test environment for my web application. I got
>> it working with the help of xvfb and running google-chrome inside it. I am
>> able to send keytrokes and mouse events through the xdotool
>> http://www.semicomplete.com/projects/xdotool which internally uses XTEST.
>>
>> I do have some test cases where I have to send touch events to the chrome
>> browser. Unfortunately the xdotool does not support sending touch events. I
>> am unable to find any info about whether XTEST supports touch events or not..
> 
> it doesn't support touch events, so going down that route isn't possible.
> 
>> I tried creating a "virtual touch device" using user mode input subsystem
>> "uinput" which works only on the logged in x-org session. I am able to send
>> touch events [Code attached in the bottom].
>>
>> But the Xvfb dont care about the virtual uinput devices the "xinput list"
>> revealed only the following.
>>
>>> sudo DISPLAY=localhost:2.0 xinput list
> 
> xvfb only initializes dummy input devices that don't do anything.
> 
>> *⎡ Virtual core pointer  id=2[master pointer
>> (3)]⎜   ↳ Virtual core XTEST pointerid=4[slave
>> pointer  (2)]⎜   ↳ Xvfb mouseid=6
>> [slave  pointer  (2)]⎣ Virtual core keyboard
>> id=3[master keyboard (2)]↳ Virtual core XTEST
>> keyboard   id=5[slave  keyboard (3)]↳ Xvfb
>> keyboard id=7[slave  keyboard (3)]*
>>
>>
>> I also tried the virtual uinput device with "Xorg X11 dummy video driver"
>> https://xpra.org/trac/wiki/Xdummy which is also not detecting the my
>> "virtual touch device" ( I ran the xorg as non root).
>>
>>
>> I am struck. I don't know where to proceed further.
>> Is there any other options available? Do I have to implement something else?
> 
> The only option I can see is to run a full Xorg with the uinput touch
> device. Short of that, I don't think anything will help.
Xdummy is a "full Xorg" just running with the dummy video driver.

This question is very similar to the one I asked last year, also using
Xdummy:
https://lists.freedesktop.org/archives/xorg/2016-June/058138.html
"emit smooth scrolling/pixel perfect events"

Unfortunately, I didn't understand enough about libinput to make any
progress on this.

I did try a few things but since the plumbing between uinput and the
userspace application handling the events didn't give me any diagnostics
I could understand, I couldn't really tell if the changes I was making
were going in the right direction or not.
Any more noob friendly pointers would be much appreciated.

Cheers
Antoine


> 
> Cheers,
>Peter
> 
>>
>> Any help would be greatly appreciated.
>>
>> Thanks,
>> Prakash
>>
>>
>> Notes,
>> 1. Starting X virtual frame buffer -  "Xvfb :2 -screen 0 1600x900x24"
>> 2. virtual touch device uinput code -
>> https://drive.google.com/open?id=0B2lQr8WlvKmzZjhJOHB0b0x4ZGc   (with the
>> help of  http://thiemonge.org/getting-started-with-uinput )
>> 3. For the xvfb - I used CentOS 7.3 with Xorg -  1.17.2, pixman 0.34.0,
>> kernel -3.10.0
>> 4. For the x dummy driver - I used ubuntu 16.10 with Xorg 1.18.4, pixman -
>> 0.33.6 , kernel -4.10.
>>
>>
>> -- 
>> Anyone who has never made a mistake has never tried anything new.
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Xorg 1.19.1 SIGPIPE on application close

[Antoine Martin]

On 06/02/17 21:46, Julien Cristau wrote:
> On 02/06/2017 03:32 PM, Antoine Martin wrote:
>> Hi,
>>
>> I can reproduce this Xorg server crash fairly reliably using xpra and
>> glxspheres simply by closing the application window:
>>
>> Thread 1 "Xorg" received signal SIGPIPE, Broken pipe.
>> 0x757ca83d in writev () at ../sysdeps/unix/syscall-template.S:84
>> 84   T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS)
> 
> That's not a crash.  SIGPIPE on client disconnect is expected, and is
> not fatal.
DOH!
And it turns out that this has nothing to do with the Xorg server: my
script ended up killing Xorg when IT died. Sorry for the noise.

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Xorg 1.19.1 SIGPIPE on application close

[Antoine Martin]

Hi,

I can reproduce this Xorg server crash fairly reliably using xpra and
glxspheres simply by closing the application window:

Thread 1 "Xorg" received signal SIGPIPE, Broken pipe.
0x757ca83d in writev () at ../sysdeps/unix/syscall-template.S:84
84  T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS)
(gdb) bt
#0  0x757ca83d in writev () at ../sysdeps/unix/syscall-template.S:84
#1  0x005a2f4c in _XSERVTransSocketWritev (ciptr=0xbb7cf0,
buf=0x7fffc960, size=) at
/usr/include/X11/Xtrans/Xtranssock.c:2367
#2  0x0059dbfd in FlushClient (who=who@entry=0xbb4740,
oc=oc@entry=0xbb4700, __extraBuf=__extraBuf@entry=0x7fffd050,
extraCount=extraCount@entry=32) at io.c:855
#3  0x0059e13f in WriteToClient (who=who@entry=0xbb4740,
count=count@entry=32, __buf=__buf@entry=0x7fffd050) at io.c:768
#4  0x00442552 in WriteEventsToClient
(pClient=pClient@entry=0xbb4740, count=, count@entry=1,
events=events@entry=0x7fffd050) at events.c:6000
#5  0x004426f0 in TryClientEvents (client=0xbb4740,
dev=, pEvents=0x7fffd050, count=1, mask=, filter=, grab=0x0) at events.c:2021
#6  0x00445e5a in DeliverEventToInputClients
(dev=dev@entry=0x7fffcca0, inputclients=0xbb4d10,
win=win@entry=0x8b49e0, events=events@entry=0x7fffd050,
count=count@entry=1, filter=filter@entry=524288, grab=0x0,
client_return=0x7fffcc48, mask_return=0x7fffcc44) at events.c:2170
#7  0x00446147 in DeliverEventToWindowMask
(mask_return=0x7fffcc44, client_return=0x7fffcc48, grab=0x0,
filter=524288, count=1, events=0x7fffd050, win=0x8b49e0,
dev=0x7fffcca0)
at events.c:2213
#8  0x00446147 in DeliverEventsToWindow
(pDev=pDev@entry=0x7fffcca0, pWin=0x8b49e0,
pEvents=pEvents@entry=0x7fffd050, count=count@entry=1,
filter=filter@entry=524288, grab=grab@entry=0x0)
at events.c:2277
#9  0x004468d7 in DeliverEvents (pWin=pWin@entry=0xbb7980,
xE=xE@entry=0x7fffd050, count=count@entry=1,
otherParent=otherParent@entry=0x0) at events.c:2826
#10 0x00467937 in DeleteWindow (value=0xbb7980, wid=) at window.c:1096
#11 0x0045bd32 in doFreeResource (res=0xbb7860,
skip=skip@entry=0) at resource.c:880
#12 0x0045cf06 in FreeClientResources
(client=client@entry=0xb6b030) at resource.c:1146
#13 0x004363af in CloseDownClient (client=0xb6b030) at
dispatch.c:3474
#14 0x0059f2d1 in ospoll_wait (ospoll=0x855910,
timeout=) at ospoll.c:412
#15 0x005982ec in WaitForSomething (are_ready=)
at WaitFor.c:226
#16 0x00436dca in Dispatch () at dispatch.c:422
#17 0x0043b018 in dix_main (argc=20, argv=0x7fffdfa8,
envp=) at main.c:287
#18 0x756ed401 in __libc_start_main (main=0x424cc0 ,
argc=20, argv=0x7fffdfa8, init=, fini=, rtld_fini=, stack_end=0x7fffdf98)
at ../csu/libc-start.c:289
#19 0x00424cfa in _start ()

It looks to me like it's trying to write to the client that has just
gone away?
xorg-x11-server-Xorg-1.19.1-3.fc25.x86_64

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Antoine Martin]

On 24/09/16 00:20, Aaron Plattner wrote:
> On 09/22/2016 04:30 PM, Bob Terek wrote:
>> On 09/21/2016 10:22 AM, Aaron Plattner wrote:
>>> On 09/20/2016 02:07 AM, Eric Engestrom wrote:
>>>> On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:
>>>>> Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
>>>>
>>>> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
>>>
>>> Looks good to me too (although I'm cheating since this chunk is
>>> identical to part of
>>> https://patchwork.freedesktop.org/patch/41058/)
>>
>> Shouldn't the first 5 of Aaron's patches be applied, since they are all
>> cleanup items?
>>
>> https://lists.x.org/archives/xorg-devel/2015-January/045395.html
> 
> I never pushed them because they were never reviewed. Would it help if I
> resent them?
Yes, please re-send and I'll make sure to test and review this week.

>> Patch 6 supposedly caused a server crash, but the first 5 should be ok?
> 
> Patch 6 was kind of controversial so I don't know if we want it anyway.
IIRC, I was the one who reported a crash when I tested it - I didn't
investigate it further.

Sounds like Bob Terek's approach is much more complete anyway.

Cheers
Antoine



> 
>> -- 
>> Bob Terek
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Antoine Martin]

On 25/09/16 18:52, Bob Terek wrote:
> 
> On 09/23/2016 07:20 AM, Aaron Plattner wrote:
> 
>> On 09/22/2016 04:30 PM, Bob Terek wrote:
> 
>>> Shouldn't the first 5 of Aaron's patches be applied, since they are all
>>> cleanup items?
>>>
>>> https://lists.x.org/archives/xorg-devel/2015-January/045395.html
>>
>> I never pushed them because they were never reviewed. Would it help if I
>> resent them?
> 
> Why don't you hold off for a bit. . .
> 
> 
>>> Patch 6 supposedly caused a server crash, but the first 5 should be ok?
>>
>> Patch 6 was kind of controversial so I don't know if we want it anyway.
> 
> Welp, on that topic, your patch 6 was an alternative approach to support
> arbitrary pixel dimensions for the framebuffer. The more conventional
> approach had been posted by Boichat,
> https://lists.x.org/archives/xorg-devel/2014-November/044580.html . I'd
> like to suggest that Boichat's approach be used, but extended even further:
> 
> In 2014 I had also modified the dummy driver while working with Teradici
> Corporation to support not only arbitrary pixel dimensions, but also
> multiple monitors. The latter feature might not make sense to some
> folks, but if you have a server-side Xserver mapped to a hardware thin
> client sitting across a network, which has multiple physical monitors
> attached, you want the Xserver to be configured in the exact same manner
> as the tin client. Even though there is just a virtual framebuffer in
> main memory, X applications need to know the number/size/location of
> monitors so that toolbars are placed properly, windows are fullscreen'ed
> properly, etc. And when the user moves from one hardware thin client to
> another, which may have a different monitor configuration, the Xserver
> session needs to change to that configuration.
> 
> At Teradici we made dummy be RandR 1.2 compliant in a manner similar to
> Boichat, but added xorg.conf parameters to specify how many CRTC/Outputs
> should be created at startup. Thus the configuration could be
> manipulated via the standard RandR API (either programatically or
> through the xrandr command). This might happen when the hardware thin
> client connects to an existing X session, and server-side session
> management software will issue what is needed to reconfigure the
> Xserver. The end result is that the hardware thin client could be
> supported by multiple CRTC/Output's, with typical VESA modes, but a
> software thin client could use arbitrary pixel dimensions on a single
> CRTC/Output to map exactly the size of the window it was running in.
> 
> Teradici and I would like to submit these patches to dummy to support
> the functionality that many want. The dummy driver is so simple that I
> don't see this as overly complex. Yes, it is a bit more code to add, but
> it is very straightforward, nothing at all surprising. And it is
> backwards compatible, the multiple CRTC/Output support does not have to
> be utilized, the default is 1. Our modified driver behaves just like the
> current one, except that arbitrary modes may be added and switched to
> via the RandR protocol.
> 
> I can post these patches, and include Aaron's cleanups, if folks will
> support this approach. It has been tested via Teradici for over a year
> and seems to be stable. I am in the process of updating the code to the
> current version and so far so good.
> 
> Thoughts?
That sounds great, I'd be happy to test and review those changes.
It think it probably makes sense to apply the cleanup patches first, to
remove code before adding some more?
These RandR bits probably warrant bumping the version number too, 0.4.0?

I was going to add comments to Aaron's cleanup patches, but patchwork
won't let me add comments.

Cheers
Antoine


> 
> Thanks,
> 
> Bob Terek
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-dummy v2] Remove pointless empty functions

[Antoine Martin]

On 22/09/16 23:14, Aaron Plattner wrote:
> These functions might be useful in a real driver, but with no
> hardware, they're pointless.  Get rid of them.
> 
> v2: Rebase, get rid of pointless calls to DUMMYAdjustFrame, return TRUE from
> DUMMYSwitchMode.
> 
> Signed-off-by: Aaron Plattner <aplatt...@nvidia.com>
Both:
Reviewed-by: Antoine Martin <anto...@nagafix.co.uk>
Tested-by: Antoine Martin <anto...@nagafix.co.uk>

Cheers
Antoine

> ---
>  src/dummy_driver.c | 44 +---
>  1 file changed, 1 insertion(+), 43 deletions(-)
> 
> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
> index cf150539e10b..265660280549 100644
> --- a/src/dummy_driver.c
> +++ b/src/dummy_driver.c
> @@ -65,9 +65,6 @@ static ModeStatus DUMMYValidMode(SCRN_ARG_TYPE arg, 
> DisplayModePtr mode,
>  static Bool  DUMMYSaveScreen(ScreenPtr pScreen, int mode);
>  
>  /* Internally used functions */
> -static Bool dummyModeInit(ScrnInfoPtr pScrn, DisplayModePtr mode);
> -static void  dummySave(ScrnInfoPtr pScrn);
> -static void  dummyRestore(ScrnInfoPtr pScrn, Bool restoreText);
>  static Bool  dummyDriverFunc(ScrnInfoPtr pScrn, xorgDriverFuncOp op,
>   pointer ptr);
>  
> @@ -461,14 +458,6 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
>  static Bool
>  DUMMYEnterVT(VT_FUNC_ARGS_DECL)
>  {
> -SCRN_INFO_PTR(arg);
> -
> -/* Should we re-save the text mode on each VT enter? */
> -if(!dummyModeInit(pScrn, pScrn->currentMode))
> -  return FALSE;
> -
> -DUMMYAdjustFrame(ADJUST_FRAME_ARGS(pScrn, pScrn->frameX0, 
> pScrn->frameY0));
> -
>  return TRUE;
>  }
>  
> @@ -476,8 +465,6 @@ DUMMYEnterVT(VT_FUNC_ARGS_DECL)
>  static void
>  DUMMYLeaveVT(VT_FUNC_ARGS_DECL)
>  {
> -SCRN_INFO_PTR(arg);
> -dummyRestore(pScrn, TRUE);
>  }
>  
>  static void
> @@ -535,15 +522,6 @@ DUMMYScreenInit(SCREEN_INIT_ARGS_DECL)
>  
>  if (!(dPtr->FBBase = malloc(pScrn->videoRam * 1024)))
>   return FALSE;
> -
> -/*
> - * next we save the current state and setup the first mode
> - */
> -dummySave(pScrn);
> -
> -if (!dummyModeInit(pScrn,pScrn->currentMode))
> - return FALSE;
> -DUMMYAdjustFrame(ADJUST_FRAME_ARGS(pScrn, pScrn->frameX0, 
> pScrn->frameY0));
>  
>  /*
>   * Reset visual list.
> @@ -665,8 +643,7 @@ DUMMYScreenInit(SCREEN_INIT_ARGS_DECL)
>  Bool
>  DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
>  {
> -SCRN_INFO_PTR(arg);
> -return dummyModeInit(pScrn, mode);
> +return TRUE;
>  }
>  
>  /* Mandatory */
> @@ -683,7 +660,6 @@ DUMMYCloseScreen(CLOSE_SCREEN_ARGS_DECL)
>  DUMMYPtr dPtr = DUMMYPTR(pScrn);
>  
>  if(pScrn->vtSema){
> - dummyRestore(pScrn, TRUE);
>   free(dPtr->FBBase);
>  }
>  
> @@ -725,24 +701,6 @@ DUMMYValidMode(SCRN_ARG_TYPE arg, DisplayModePtr mode, 
> Bool verbose, int flags)
>  return(MODE_OK);
>  }
>  
> -static void
> -dummySave(ScrnInfoPtr pScrn)
> -{
> -}
> -
> -static void 
> -dummyRestore(ScrnInfoPtr pScrn, Bool restoreText)
> -{
> -}
> -
> -static Bool
> -dummyModeInit(ScrnInfoPtr pScrn, DisplayModePtr mode)
> -{
> -dummyRestore(pScrn, FALSE);
> -
> -return(TRUE);
> -}
> -
>  Atom VFB_PROP  = 0;
>  #define  VFB_PROP_NAME  "VFB_IDENT"
>  
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] remove dead code in dummy driver

[Antoine Martin]

On 22/09/16 03:22, Aaron Plattner wrote:
> On 09/20/2016 02:07 AM, Eric Engestrom wrote:
>> On Tue, Sep 20, 2016 at 01:34:40PM +0700, Antoine Martin wrote:
>>> Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
>>
>> Reviewed-by: Eric Engestrom <eric.engest...@imgtec.com>
> 
> Looks good to me too (although I'm cheating since this chunk is identical to 
> part of https://patchwork.freedesktop.org/patch/41058/)
That one is even better.

You can add to that one:
Reviewed-by: Antoine Martin <anto...@nagafix.co.uk>

Cheers
Antoine



> 
> Pushed:
> remote: Updating patchwork state for 
> https://patchwork.freedesktop.org/project/Xorg/list/
> remote: I: patch #111435 updated using rev 
> 367c778240b4266958f33cec3653d5389e283557.
> remote: I: 1 patch(es) updated to state Accepted.
> To git+ssh://git.freedesktop.org/git/xorg/driver/xf86-video-dummy
>8706f60ab457..367c778240b4  HEAD -> master
> 
>>> ---
>>>  src/dummy_driver.c | 19 ---
>>>  1 file changed, 19 deletions(-)
>>>
>>> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
>>> index c84000f..ec1acf3 100644
>>> --- a/src/dummy_driver.c
>>> +++ b/src/dummy_driver.c
>>> @@ -700,25 +700,6 @@ DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
>>>  void
>>>  DUMMYAdjustFrame(ADJUST_FRAME_ARGS_DECL)
>>>  {
>>> -SCRN_INFO_PTR(arg);
>>> -int Base; 
>>> -
>>> -Base = (y * pScrn->displayWidth + x) >> 2;
>>> -
>>> -/* Scale Base by the number of bytes per pixel. */
>>> -switch (pScrn->depth) {
>>> -case  8 :
>>> -   break;
>>> -case 15 :
>>> -case 16 :
>>> -   Base *= 2;
>>> -   break;
>>> -case 24 :
>>> -   Base *= 3;
>>> -   break;
>>> -default :
>>> -   break;
>>> -}
>>>  }
>>>  
>>>  /* Mandatory */
>>> -- 
>>> 2.7.4
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] simplify convoluted code in dummy LoadPalette

[Antoine Martin]

On 20/09/16 22:54, Antoine Martin wrote:
> Only the 15-bit mode does anything different,
> make that clearer and remove the redundant code for other bit depths
> 
> Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
> ---
>  src/dummy_driver.c | 17 -
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/src/dummy_driver.c b/src/dummy_driver.c
> index c84000f..5e0bc03 100644
> --- a/src/dummy_driver.c
> +++ b/src/dummy_driver.c
> @@ -492,26 +492,17 @@ DUMMYLoadPalette(
> LOCO *colors,
> VisualPtr pVisual
>  ){
> -   int i, index, shift, Gshift;
> +   int i, index, shift=0;
> DUMMYPtr dPtr = DUMMYPTR(pScrn);
>  
> -   switch(pScrn->depth) {
> -   case 15:  
> - shift = Gshift = 1;
> - break;
> -   case 16:
> - shift = 0; 
> -Gshift = 0;
> - break;
> -   default:
> - shift = Gshift = 0;
> - break;
> +   if (pScrn->depth==15) {
> + shift = 1
> }
>  
> for(i = 0; i < numColors; i++) {
> index = indices[i];
> dPtr->colors[index].red = colors[index].red << shift;
> -   dPtr->colors[index].green = colors[index].green << Gshift;
> +   dPtr->colors[index].green = colors[index].green << shift;
> dPtr->colors[index].blue = colors[index].blue << shift;
> } 
>
Note: this patch does not change the current behaviour - which may well
be wrong: the fact that there was a separate variable for Gshift makes
me think that maybe the 15-bit mode is meant to use Gshift=1 but keep
shift=0? (or the other way around even)

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] simplify convoluted code in dummy LoadPalette

[Antoine Martin]

Only the 15-bit mode does anything different,
make that clearer and remove the redundant code for other bit depths

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
 src/dummy_driver.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index c84000f..5e0bc03 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -492,26 +492,17 @@ DUMMYLoadPalette(
LOCO *colors,
VisualPtr pVisual
 ){
-   int i, index, shift, Gshift;
+   int i, index, shift=0;
DUMMYPtr dPtr = DUMMYPTR(pScrn);
 
-   switch(pScrn->depth) {
-   case 15:
-   shift = Gshift = 1;
-   break;
-   case 16:
-   shift = 0; 
-Gshift = 0;
-   break;
-   default:
-   shift = Gshift = 0;
-   break;
+   if (pScrn->depth==15) {
+   shift = 1
}
 
for(i = 0; i < numColors; i++) {
index = indices[i];
dPtr->colors[index].red = colors[index].red << shift;
-   dPtr->colors[index].green = colors[index].green << Gshift;
+   dPtr->colors[index].green = colors[index].green << shift;
dPtr->colors[index].blue = colors[index].blue << shift;
} 
 
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] support for 30 bit depth in dummy driver

[Antoine Martin]

This patch makes it possible to start the server using the dummy
driver with a 30 bit depth.
The colormap size is changed from 256 to 1024 to prevent crashes.

This updated patch adds the missing header file to the commit.

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
 src/dummy.h| 2 +-
 src/dummy_driver.c | 7 ---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/dummy.h b/src/dummy.h
index c3fdd6e..0dd8906 100644
--- a/src/dummy.h
+++ b/src/dummy.h
@@ -68,7 +68,7 @@ typedef struct dummyRec
 int overlay_offset;
 int videoKey;
 int interlace;
-dummy_colors colors[256];
+dummy_colors colors[1024];
 pointer* FBBase;
 Bool(*CreateWindow)() ; /* wrapped CreateWindow */
 Bool prop;
diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index 737f11c..c84000f 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -313,6 +313,7 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
case 15:
case 16:
case 24:
+case 30:
break;
default:
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
@@ -327,8 +328,8 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
pScrn->rgbBits = 8;
 
 /* Get the depth24 pixmap format */
-if (pScrn->depth == 24 && pix24bpp == 0)
-   pix24bpp = xf86GetBppFromDepth(pScrn, 24);
+if (pScrn->depth >= 24 && pix24bpp == 0)
+   pix24bpp = xf86GetBppFromDepth(pScrn, pScrn->depth);
 
 /*
  * This must happen after pScrn->display has been set because
@@ -637,7 +638,7 @@ DUMMYScreenInit(SCREEN_INIT_ARGS_DECL)
 if(!miCreateDefColormap(pScreen))
return FALSE;
 
-if (!xf86HandleColormaps(pScreen, 256, pScrn->rgbBits,
+if (!xf86HandleColormaps(pScreen, 1024, pScrn->rgbBits,
  DUMMYLoadPalette, NULL, 
  CMAP_PALETTED_TRUECOLOR 
 | CMAP_RELOAD_ON_MODE_SWITCH))
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] remove dead code in dummy driver

[Antoine Martin]

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
 src/dummy_driver.c | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index c84000f..ec1acf3 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -700,25 +700,6 @@ DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
 void
 DUMMYAdjustFrame(ADJUST_FRAME_ARGS_DECL)
 {
-SCRN_INFO_PTR(arg);
-int Base; 
-
-Base = (y * pScrn->displayWidth + x) >> 2;
-
-/* Scale Base by the number of bytes per pixel. */
-switch (pScrn->depth) {
-case  8 :
-   break;
-case 15 :
-case 16 :
-   Base *= 2;
-   break;
-case 24 :
-   Base *= 3;
-   break;
-default :
-   break;
-}
 }
 
 /* Mandatory */
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] support for 30 bit depth in dummy driver

[Antoine Martin]

This patch makes it possible to start the server using the dummy
driver with a 30 bit depth.
The colormap size is changed from 256 to 1024 to prevent crashes.

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
 src/dummy_driver.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index 737f11c..c84000f 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -313,6 +313,7 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
case 15:
case 16:
case 24:
+case 30:
break;
default:
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
@@ -327,8 +328,8 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
pScrn->rgbBits = 8;
 
 /* Get the depth24 pixmap format */
-if (pScrn->depth == 24 && pix24bpp == 0)
-   pix24bpp = xf86GetBppFromDepth(pScrn, 24);
+if (pScrn->depth >= 24 && pix24bpp == 0)
+   pix24bpp = xf86GetBppFromDepth(pScrn, pScrn->depth);
 
 /*
  * This must happen after pScrn->display has been set because
@@ -637,7 +638,7 @@ DUMMYScreenInit(SCREEN_INIT_ARGS_DECL)
 if(!miCreateDefColormap(pScreen))
return FALSE;
 
-if (!xf86HandleColormaps(pScreen, 256, pScrn->rgbBits,
+if (!xf86HandleColormaps(pScreen, 1024, pScrn->rgbBits,
  DUMMYLoadPalette, NULL, 
  CMAP_PALETTED_TRUECOLOR 
 | CMAP_RELOAD_ON_MODE_SWITCH))
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: 30-bpp mode for dummy - exposes a bug somewhere else?

[Antoine Martin]

(snip)
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x7fddc2a78b98 in DUMMYLoadPalette (pScrn=,
>> numColors=, indices=, colors=0xd01a80,
>> pVisual=)
>> at dummy_driver.c:513
>> 513 dPtr->colors[index].red = colors[index].red << shift;
>> (gdb) bt
>> #0  0x7fddc2a78b98 in DUMMYLoadPalette (pScrn=,
>> numColors=, indices=, colors=0xd01a80,
>> pVisual=)
>> at dummy_driver.c:513
>> #1  0x00480fb2 in CMapRefreshColors ()
>> #2  0x004816e8 in CMapReinstallMap ()
>> #3  0x004817ca in CMapSwitchMode ()
>> #4  0x0047407a in xf86SwitchMode ()
>> #5  0x00497a53 in xf86RandRSetMode ()
>> #6  0x0049808a in xf86RandRSetConfig ()
>> #7  0x004fa398 in RRCrtcSet ()
>> #8  0x00507a46 in ProcRRSetScreenConfig ()
>> #9  0x00436daf in Dispatch ()
>> #10 0x0043add3 in dix_main ()
>> #11 0x7fddc7fb8731 in __libc_start_main (main=0x424d20 ,
>> argc=19, argv=0x7ffc98ec5e18, init=, fini=> out>, rtld_fini=,
>> stack_end=0x7ffc98ec5e08) at ../csu/libc-start.c:289
>> #12 0x00424d59 in _start ()
>> (gdb)
>>
>> It's always crashing in palette or colormap functions, ie:
>> CMapDestroyColormap or DUMMYLoadPalette, etc.
>>
>> Could it be that there's a bug somewhere else that is only being
>> triggered with 30-bpp modes? Trying to use more colours / space than is
>> available in the current colourmap perhaps?
>> I had temporarily prevented crashes with only some (!) randr resizings
>> by clamping the "numColors" value to 256 in DUMMYLoadPalette.
> 
> I suspect it's related to DUMMYScreenInit calling xf86HandleColormaps
> with 256 for maxColors. That (and possibly other places) probably needs
> to be changed to allow for 1024 entries in colourmaps.
You're absolutely right, that fixes the problem.
I will send a patch shortly.

Just a thought: if a value lower than 1024 is likely to cause crashes
like this one, shouldn't there be some stronger validation of input
values in xf86HandleColormaps?

Thanks!
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

30-bpp mode for dummy - exposes a bug somewhere else?

[Antoine Martin]

Hi,

Adding support for 10 bits per pixel mode to the dummy driver:
-- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -313,6 +313,7 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
case 15:
case 16:
case 24:
+case 30:
break;
default:
xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
@@ -327,8 +328,8 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
pScrn->rgbBits = 8;

 /* Get the depth24 pixmap format */
-if (pScrn->depth == 24 && pix24bpp == 0)
-   pix24bpp = xf86GetBppFromDepth(pScrn, 24);
+if (pScrn->depth >= 24 && pix24bpp == 0)
+   pix24bpp = xf86GetBppFromDepth(pScrn, pScrn->depth);

 /*
  * This must happen after pScrn->display has been set because


This looks simple enough and works very well, stable. Unfortunately,
this also causes any calls to RandR to crash the server:

Program received signal SIGSEGV, Segmentation fault.
0x7fddc2a78b98 in DUMMYLoadPalette (pScrn=,
numColors=, indices=, colors=0xd01a80,
pVisual=)
at dummy_driver.c:513
513dPtr->colors[index].red = colors[index].red << shift;
(gdb) bt
#0  0x7fddc2a78b98 in DUMMYLoadPalette (pScrn=,
numColors=, indices=, colors=0xd01a80,
pVisual=)
at dummy_driver.c:513
#1  0x00480fb2 in CMapRefreshColors ()
#2  0x004816e8 in CMapReinstallMap ()
#3  0x004817ca in CMapSwitchMode ()
#4  0x0047407a in xf86SwitchMode ()
#5  0x00497a53 in xf86RandRSetMode ()
#6  0x0049808a in xf86RandRSetConfig ()
#7  0x004fa398 in RRCrtcSet ()
#8  0x00507a46 in ProcRRSetScreenConfig ()
#9  0x00436daf in Dispatch ()
#10 0x0043add3 in dix_main ()
#11 0x7fddc7fb8731 in __libc_start_main (main=0x424d20 ,
argc=19, argv=0x7ffc98ec5e18, init=, fini=, rtld_fini=,
stack_end=0x7ffc98ec5e08) at ../csu/libc-start.c:289
#12 0x00424d59 in _start ()
(gdb)

It's always crashing in palette or colormap functions, ie:
CMapDestroyColormap or DUMMYLoadPalette, etc.

Could it be that there's a bug somewhere else that is only being
triggered with 30-bpp modes? Trying to use more colours / space than is
available in the current colourmap perhaps?
I had temporarily prevented crashes with only some (!) randr resizings
by clamping the "numColors" value to 256 in DUMMYLoadPalette.

If you want to try it for yourself, just apply the 30-bpp patch at the
top of this file and run:
Xorg +extension RANDR  -config xorg.conf :100
With this config:
http://xpra.org/trac/raw-attachment/ticket/909/xorg.conf
Then you can crash the server reliably with:
DISPLAY=:100 xrandr -s 640x480

I think I am out of my depth (pun), how do I debug from here?

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

DUMMYAdjustFrame does nothing useful

[Antoine Martin]

Hi,

This function does absolutely nothing except dereferencing some
pointers. Can we get rid of it? (or just replace it with a no-op)
What is it meant to do?

Here it is in full:
* Mandatory */
void
DUMMYAdjustFrame(ADJUST_FRAME_ARGS_DECL)
{
SCRN_INFO_PTR(arg);
int Base;

Base = (y * pScrn->displayWidth + x) >> 2;

/* Scale Base by the number of bytes per pixel. */
switch (pScrn->depth) {
case  8 :
break;
case 15 :
case 16 :
Base *= 2;
break;
case 24 :
Base *= 3;
break;
case 30 :
Base *= 4;
default :
break;
}
}

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Hiding keyboard state

[Antoine Martin]

On 21/06/16 09:17, Keith Packard wrote:
> Antoine Martin <anto...@nagafix.co.uk> writes:
> 
>> This may cause us problems with xpra: we use XQueryKeymap to see which
>> keys are down when trying to synchronize remote and local keyboard
>> state, in particular for the modifiers.
> 
> The new query code returns the current state of the keyboard as known to
> each client, so you get all of the keys which your client might have
> received events about, including the full key state on enter events.
> 
>> The KeymapNotify handling in this patch just copies the new keymap,
>> can't that be used to see which keys are pressed? Or does setting a new
>> keymap also reset pressed keys?
> 
> Yes, you receive the full state of the keyboard whenever you receive
> focus, and this patch doesn't change that. The goal is to eliminate the
> ability to poll the keymap rapidly to see what *other* clients are
> receiving.
> 
>> The most important requirement we have is to be able to force unset all
>> the modifiers when a new client connects, so maybe we can just set a new
>> keymap every time to achieve that?
> 
> Inside the server you can do whatever you like, as usual.
> 
>> Since the actual keyboard state is still global and not per application,
>> what happens if:
>> * another app presses Shift_R, setting the "shift" modifier,
>> * we query the keymap and find that the "shift" modifier is not set,
>> * we press some other key (say "a") via XTest, expecting that
>> applications will see "a" but they may in fact see "(shift)a", right?
> 
> Yes.
> 
>> At present, we can spot that "shift" is (un)set and toggle it as needed,
>> using the correct key too (Shift_L vs Shift_R), will this no longer be
>> the case?
> 
> To work without knowing the state of the keys, you would simply need to
> always send events to set the modifiers to the appropriate state.
For the sake of argument, we have encountered a number of applications
that respond to any key event (including modifiers) and so we have had
to work extra hard to try to minimize unnecessary XTest key events.
(handling AltGr with MS Windows client comes to mind)

But I don't think this is a huge issue for us anyway because we don't
want to support simultaneous input from non-xpra clients, and if
applications modify the keyboard state programmatically.. things are
likely to get out of sync no matter what we do.

>> I understand the motivation though.. And we may be ok for most use
>> cases, but is there a chance of making this optional somehow?
>> A fair number of users rely on xpra + virtualization for application
>> isolation and therefore the security of the X11 keyboard within the
>> server is of no concern to them, but "correct" keyboard handling is.
> 
> I've also started working on a more complete inter-client isolation
> notion where we have privileged clients that have access to global
> server state (like the testing extensions). With this, we'd be able to
> have your clients run with that special ability and have that expose the
> global keymap instead of the per-client one.
Just a thought: could this be tied somehow with selection ownership?
A window manager would be expected to have more privileged access, and
this is usually arbitrated through selections.

>> From a later reply to this thread:
>>> Alternatively, we could restrict this request to "special"
>> applications using the technique I've outlined for access to the testing
>> extensions.
>> Is this the "Disabling RECORD by default" thread or another?
> 
> Yes, of course that subject isn't accurate anymore as we need to disable
> all testing extensions and a bunch of other global server state as well.
> 
> Thanks for helping think about this stuff; the goal is to provide
> reasonable security while not breaking too many things.
Excellent, we'll be happy to test any security changes, if anything to
ensure that it doesn't break our stuff too much!

Cheers
Antoine




signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/5] ephyr: Don't configure window while responding to configure events

[Antoine Martin]

On 14/06/16 23:02, Keith Packard wrote:
> This leads to and endless sequence of window resizes.
> 
> Signed-off-by: Keith Packard <kei...@keithp.com>
Tested-by: Antoine Martin <anto...@nagafix.co.uk>

Can we get this one into the stable queue?
It applies cleanly to 1.18 and fixes a bug that's quite easy to trigger.

Thanks
Antoine


> ---
>  hw/kdrive/ephyr/ephyr.c | 2 ++
>  hw/kdrive/ephyr/hostx.c | 8 
>  hw/kdrive/ephyr/hostx.h | 3 +++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/hw/kdrive/ephyr/ephyr.c b/hw/kdrive/ephyr/ephyr.c
> index 2114c1c..2bc5ccc 100644
> --- a/hw/kdrive/ephyr/ephyr.c
> +++ b/hw/kdrive/ephyr/ephyr.c
> @@ -613,7 +613,9 @@ ephyrResizeScreen (ScreenPtr   pScreen,
>  size.width = newwidth;
>  size.height = newheight;
>  
> +hostx_size_set_from_configure(TRUE);
>  ret = ephyrRandRSetConfig (pScreen, screen->randr, 0, );
> +hostx_size_set_from_configure(FALSE);
>  if (ret) {
>  RROutputPtr output;
>  
> diff --git a/hw/kdrive/ephyr/hostx.c b/hw/kdrive/ephyr/hostx.c
> index cdb12b0..d84c33b 100644
> --- a/hw/kdrive/ephyr/hostx.c
> +++ b/hw/kdrive/ephyr/hostx.c
> @@ -79,6 +79,7 @@ struct EphyrHostXVars {
>  KdScreenInfo **screens;
>  
>  long damage_debug_msec;
> +Bool size_set_from_configure;
>  };
>  
>  /* memset ( missing> ) instead of below  */
> @@ -878,6 +879,7 @@ hostx_screen_init(KdScreenInfo *screen,
>  xallocarray(scrpriv->ximg->stride, buffer_height);
>  }
>  
> +if (!HostX.size_set_from_configure)
>  {
>  uint32_t mask = XCB_CONFIG_WINDOW_WIDTH | XCB_CONFIG_WINDOW_HEIGHT;
>  uint32_t values[2] = {width, height};
> @@ -1213,6 +1215,12 @@ hostx_load_keymap(KeySymsPtr keySyms, CARD8 *modmap, 
> XkbControlsPtr controls)
>  return TRUE;
>  }
>  
> +void
> +hostx_size_set_from_configure(Bool ss)
> +{
> +HostX.size_set_from_configure = ss;
> +}
> +
>  xcb_connection_t *
>  hostx_get_xcbconn(void)
>  {
> diff --git a/hw/kdrive/ephyr/hostx.h b/hw/kdrive/ephyr/hostx.h
> index 96d7394..6e0b07b 100644
> --- a/hw/kdrive/ephyr/hostx.h
> +++ b/hw/kdrive/ephyr/hostx.h
> @@ -151,6 +151,9 @@ hostx_paint_rect(KdScreenInfo *screen,
>  Bool
>  hostx_load_keymap(KeySymsPtr keySyms, CARD8 *modmap, XkbControlsPtr 
> controls);
>  
> +void
> +hostx_size_set_from_configure(Bool);
> +
>  xcb_connection_t *
>  hostx_get_xcbconn(void);
>  
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Hiding keyboard state

[Antoine Martin]

On 24/11/15 02:04, Keith Packard wrote:
> 
> One of the many security holes in X is that any application can monitor
> the state of the keyboard device by querying the list of pressed keys on
> a regular basis. Here's a simple patch which makes that request report
> only key state which the client itself has already seen through X
> events.
This may cause us problems with xpra: we use XQueryKeymap to see which
keys are down when trying to synchronize remote and local keyboard
state, in particular for the modifiers.

The KeymapNotify handling in this patch just copies the new keymap,
can't that be used to see which keys are pressed? Or does setting a new
keymap also reset pressed keys?

The most important requirement we have is to be able to force unset all
the modifiers when a new client connects, so maybe we can just set a new
keymap every time to achieve that?

Since the actual keyboard state is still global and not per application,
what happens if:
* another app presses Shift_R, setting the "shift" modifier,
* we query the keymap and find that the "shift" modifier is not set,
* we press some other key (say "a") via XTest, expecting that
applications will see "a" but they may in fact see "(shift)a", right?
At present, we can spot that "shift" is (un)set and toggle it as needed,
using the correct key too (Shift_L vs Shift_R), will this no longer be
the case?

I understand the motivation though.. And we may be ok for most use
cases, but is there a chance of making this optional somehow?
A fair number of users rely on xpra + virtualization for application
isolation and therefore the security of the X11 keyboard within the
server is of no concern to them, but "correct" keyboard handling is.

From a later reply to this thread:
> Alternatively, we could restrict this request to "special"
applications using the technique I've outlined for access to the testing
extensions.
Is this the "Disabling RECORD by default" thread or another?

Cheers
Antoine


> With this patch in place, grabbing the keyboard should be sufficient to
> hide key presses from other clients.
> 
> I think we need to try to fix some of these issues, even if the fixes
> break existing applications. The next thing I'd like to try is to to
> deliver input events to only one client (owner first, then
> others). After that, apply the same rules to the input extension.
> 
> In general, there are three areas that I'm wondering if we can fix:
> 
>  1) input monitoring. This seems fairly "safe" as far as existing apps
> go.
> 
>  2) output monitoring. This seems much harder as so many useful hacks
> and extensions take advantage of being able to get contents from
> other windows.
> 
>  3) breaking screen saver security. We've got an extension, let's make
> it work.
> 
> -keith
> 
> From 627815391d2d6845f7e0a66d447c6b379be9d3cb Mon Sep 17 00:00:00 2001
> From: Keith Packard 
> Date: Mon, 23 Nov 2015 10:01:10 -0800
> Subject: [PATCH xserver] Track keystate per client for QueryKeymap
> 
> This adds a per-client key state vector and uses that for
> ProcQueryKeymap instead of the device keymap.
> 
> Signed-off-by: Keith Packard 
> ---
>  dix/devices.c   |  2 +-
>  dix/events.c| 13 +
>  include/dixstruct.h |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/dix/devices.c b/dix/devices.c
> index 9b0c7d2..49b6994 100644
> --- a/dix/devices.c
> +++ b/dix/devices.c
> @@ -2405,7 +2405,7 @@ ProcQueryKeymap(ClientPtr client)
>  xQueryKeymapReply rep;
>  int rc, i;
>  DeviceIntPtr keybd = PickKeyboard(client);
> -CARD8 *down = keybd->key->down;
> +CARD8 *down = client->down;
>  
>  REQUEST_SIZE_MATCH(xReq);
>  rep = (xQueryKeymapReply) {
> diff --git a/dix/events.c b/dix/events.c
> index efaf91d..4114471 100644
> --- a/dix/events.c
> +++ b/dix/events.c
> @@ -2006,6 +2006,19 @@ TryClientEvents(ClientPtr client, DeviceIntPtr dev, 
> xEvent *pEvents,
>  }
>  }
>  
> +/* Track keyboard state per client */
> +switch (type) {
> +case KeyPress:
> +SetBit(client->down, pEvents->u.u.detail);
> +break;
> +case KeyRelease:
> +ClearBit(client->down, pEvents->u.u.detail);
> +break;
> +case KeymapNotify:
> +memcpy(client->down+1, ((xKeymapEvent *) pEvents)->map, 31);
> +break;
> +}
> +
>  if (BitIsOn(criticalEvents, type)) {
>  if (client->smart_priority < SMART_MAX_PRIORITY)
>  client->smart_priority++;
> diff --git a/include/dixstruct.h b/include/dixstruct.h
> index 8e70ae1..1e9f69e 100644
> --- a/include/dixstruct.h
> +++ b/include/dixstruct.h
> @@ -103,6 +103,7 @@ typedef struct _Client {
>  unsigned short newKeyboardNotifyMask;
>  unsigned short vMajor, vMinor;
>  KeyCode minKC, maxKC;
> +CARD8 down[DOWN_LENGTH];/* track key state for QueryKeymap */
>  
>  int smart_start_tick;
>  int smart_stop_tick;
> 
> 
> 
> 

Re: [PATCH xserver] os: add an optional offset to -displayfd parameter

[Antoine Martin]

On 13/06/16 23:20, Bernhard Miklautz wrote:
> Hi Antoine,
> 
> thank you for your feedback.
> 
> On 06/13/2016 05:40 PM, Antoine Martin wrote:
>> I think you also need to validate the displayoffset to make sure it
>> isn't greater than 65536 - X_TCP_PORT before you enter the loop.
> Currently if the supplied offset is larger than (65536 - X_TCP_PORT) the loop 
> would never
> run causing the server to exit with an error. Like:
> 
> Xvfb -displayfd 2:59536
> 
> (EE)
> Fatal server error:
> (EE) Failed to find a socket to listen on(EE)
> 
> 
> It would however, probably be a good idea to add an additional error message 
> if the startup fails
> because the supplied offset was to high. What do you think about that?
As long as it fails more or less gracefully, that's fine. And it does, so:
Reviewed-by: Antoine Martin <anto...@nagafix.co.uk>

(feel free to add better validation if you fell like it)

Cheers
Antoine

> 
> Thanks.
> Best regards,
> Bernhard
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] os: add an optional offset to -displayfd parameter

[Antoine Martin]

On 13/06/16 22:06, Bernhard Miklautz wrote:
> The displayfd option is very handy and useful. However, in some environments
> where many servers are running (for example in an automated build
> environment using Xvfb) it is often desirable to be able to specify
> an initial offset for the display search.
> 
> For example if it is known that the first 100 displays are
> already taken it isn't currently possible to start at 101 therefore
> each subsequent Xvfb launch would re-try all displays from :0 to :100.
> 
> This patch adds an optional offset option to the displayfd parameter
> that allows it to specify the initial display number to start the search
> from. The format is:
> 
> -displayfd fd[:]
> 
> The patch also updates the help as well as the Xserver man page accordingly.
> 
> Signed-off-by: Bernhard Miklautz 
What a great idea that is.

I think you also need to validate the displayoffset to make sure it
isn't greater than 65536 - X_TCP_PORT before you enter the loop.

Cheers
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH dummy] fix pointer limits when resizing v2

[Antoine Martin]

On 05/05/16 18:17, Eric Engestrom wrote:
> On Thu, May 05, 2016 at 01:07:26PM +0700, Antoine Martin wrote:
>> Pointer events are clipped to the original screen dimensions with the
>> dummy driver when using RandR to switch to a higher resolution at
>> runtime. This fixes it.
> 
> Please use `git send-email` [1] to send patches, and add the quoted
> text to the commit message.
Done.

Is this a new requirement? The wiki on submitting patches still says:
https://www.x.org/wiki/Development/Documentation/SubmittingPatches/
'Of course, you can use other e-mail clients to attach your patch.'.

Cheers
Antoine

> 
> Cheers,
>   Eric
> 
> [1] https://git-scm.com/docs/git-send-email
> 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] Fix screen and pointer limits on RandR resize

[Antoine Martin]

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
 src/dummy_driver.c | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index 9d4d5bf..737f11c 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -49,6 +49,9 @@
 #include 
 #endif
 
+/* Needed for fixing pointer limits on resize */
+#include "inputstr.h"
+
 /* Mandatory functions */
 static const OptionInfoRec *   DUMMYAvailableOptions(int chipid, int busid);
 static void DUMMYIdentify(int flags);
@@ -666,7 +669,30 @@ Bool
 DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
 {
 SCRN_INFO_PTR(arg);
-return dummyModeInit(pScrn, mode);
+if (!dummyModeInit(pScrn, mode))
+return FALSE;
+
+//ensure the screen dimensions are also updated:
+pScrn->pScreen->width = mode->HDisplay;
+pScrn->pScreen->height = mode->VDisplay;
+pScrn->virtualX = mode->HDisplay;
+pScrn->virtualY = mode->VDisplay;
+pScrn->frameX1 = mode->HDisplay;
+pScrn->frameY1 = mode->VDisplay;
+
+//ensure the pointer uses the new limits too:
+DeviceIntPtr pDev;
+SpritePtr pSprite;
+for (pDev = inputInfo.devices; pDev; pDev = pDev->next) {
+if (pDev->spriteInfo!=NULL && pDev->spriteInfo->sprite!=NULL) {
+pSprite = pDev->spriteInfo->sprite;
+pSprite->hotLimits.x2 = mode->HDisplay;
+pSprite->hotLimits.y2 = mode->VDisplay;
+pSprite->physLimits.x2 = mode->HDisplay;
+pSprite->physLimits.y2 = mode->VDisplay;
+}
+}
+return TRUE;
 }
 
 /* Mandatory */
-- 
2.5.5

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH dummy] fix pointer limits when resizing v2

[Antoine Martin]

Hi,

Patch updated to apply to git master.



Pointer events are clipped to the original screen dimensions with the
dummy driver when using RandR to switch to a higher resolution at
runtime. This fixes it.

Cheers
Antoine

From 3aa442212e3373810b632ecc5349344cd78c145d Mon Sep 17 00:00:00 2001
From: Antoine Martin <anto...@nagafix.co.uk>
Date: Thu, 5 May 2016 12:55:10 +0700
Subject: [PATCH] Fix screen and pointer limits on RandR resize

Signed-off-by: Antoine Martin <anto...@nagafix.co.uk>
---
 src/dummy_driver.c | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index 9d4d5bf..737f11c 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -49,6 +49,9 @@
 #include 
 #endif
 
+/* Needed for fixing pointer limits on resize */
+#include "inputstr.h"
+
 /* Mandatory functions */
 static const OptionInfoRec *	DUMMYAvailableOptions(int chipid, int busid);
 static void DUMMYIdentify(int flags);
@@ -666,7 +669,30 @@ Bool
 DUMMYSwitchMode(SWITCH_MODE_ARGS_DECL)
 {
 SCRN_INFO_PTR(arg);
-return dummyModeInit(pScrn, mode);
+if (!dummyModeInit(pScrn, mode))
+return FALSE;
+
+//ensure the screen dimensions are also updated:
+pScrn->pScreen->width = mode->HDisplay;
+pScrn->pScreen->height = mode->VDisplay;
+pScrn->virtualX = mode->HDisplay;
+pScrn->virtualY = mode->VDisplay;
+pScrn->frameX1 = mode->HDisplay;
+pScrn->frameY1 = mode->VDisplay;
+
+//ensure the pointer uses the new limits too:
+DeviceIntPtr pDev;
+SpritePtr pSprite;
+for (pDev = inputInfo.devices; pDev; pDev = pDev->next) {
+if (pDev->spriteInfo!=NULL && pDev->spriteInfo->sprite!=NULL) {
+pSprite = pDev->spriteInfo->sprite;
+pSprite->hotLimits.x2 = mode->HDisplay;
+pSprite->hotLimits.y2 = mode->VDisplay;
+pSprite->physLimits.x2 = mode->HDisplay;
+pSprite->physLimits.y2 = mode->VDisplay;
+}
+}
+return TRUE;
 }
 
 /* Mandatory */
-- 
2.5.5

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH][RESEND no2] xf86-video-dummy: fix pointer limits when resizing

[Antoine Martin]

Hi,

This patch seems to have got lost, so I am resending it, again.
This fixes a real bug. Comments?



Pointer events are clipped to the original screen dimensions with the
dummy driver when using RandR to switch to a higher resolution at runtime.

I don't know if this is the correct fix for it (is there a more generic
solution?), but this has been used extensively over the last year
without any noticeable side effects:

--- xf86-video-dummy-0.3.6/src/dummy_driver.c2014-11-05
19:24:02.668656601 +0700
+++ xf86-video-dummy-0.3.6.new/src/dummy_driver.c2014-11-05
19:37:53.076061853 +0700
@@ -55,6 +55,9 @@
 #include 
 #endif
 +/* Needed for fixing pointer limits on resize */
+#include "inputstr.h"
+
 /* Mandatory functions */
 static const OptionInfoRec *DUMMYAvailableOptions(int chipid, int
busid);
 static void DUMMYIdentify(int flags);
@@ -713,6 +716,26 @@
 RRTellChanged(pScrn->pScreen);
 }
 #endif
+//ensure the screen dimensions are also updated:
+pScrn->pScreen->width = mode->HDisplay;
+pScrn->pScreen->height = mode->VDisplay;
+pScrn->virtualX = mode->HDisplay;
+pScrn->virtualY = mode->VDisplay;
+pScrn->frameX1 = mode->HDisplay;
+pScrn->frameY1 = mode->VDisplay;
+
+//ensure the pointer uses the new limits too:
+DeviceIntPtr pDev;
+SpritePtr pSprite;
+for (pDev = inputInfo.devices; pDev; pDev = pDev->next) {
+if (pDev->spriteInfo!=NULL && pDev->spriteInfo->sprite!=NULL) {
+pSprite = pDev->spriteInfo->sprite;
+pSprite->hotLimits.x2 = mode->HDisplay;
+pSprite->hotLimits.y2 = mode->VDisplay;
+pSprite->physLimits.x2 = mode->HDisplay;
+pSprite->physLimits.y2 = mode->VDisplay;
+}
+}
 return TRUE;
 }

Cheers
Antoine

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH][RESEND] xf86-video-dummy: fix pointer limits when resizing

[Antoine Martin]

Hi,

This patch seems to have got lost, so I am resending it.



Pointer events are clipped to the original screen dimensions with the
dummy driver when using RandR to switch to a higher resolution at runtime.

I don't know if this is the correct fix for it (is there a more generic
solution?), but this has been used extensively over the last year
without any noticeable side effects:

--- xf86-video-dummy-0.3.6/src/dummy_driver.c2014-11-05
19:24:02.668656601 +0700
+++ xf86-video-dummy-0.3.6.new/src/dummy_driver.c2014-11-05
19:37:53.076061853 +0700
@@ -55,6 +55,9 @@
 #include 
 #endif
 +/* Needed for fixing pointer limits on resize */
+#include "inputstr.h"
+
 /* Mandatory functions */
 static const OptionInfoRec *DUMMYAvailableOptions(int chipid, int
busid);
 static void DUMMYIdentify(int flags);
@@ -713,6 +716,26 @@
 RRTellChanged(pScrn->pScreen);
 }
 #endif
+//ensure the screen dimensions are also updated:
+pScrn->pScreen->width = mode->HDisplay;
+pScrn->pScreen->height = mode->VDisplay;
+pScrn->virtualX = mode->HDisplay;
+pScrn->virtualY = mode->VDisplay;
+pScrn->frameX1 = mode->HDisplay;
+pScrn->frameY1 = mode->VDisplay;
+
+//ensure the pointer uses the new limits too:
+DeviceIntPtr pDev;
+SpritePtr pSprite;
+for (pDev = inputInfo.devices; pDev; pDev = pDev->next) {
+if (pDev->spriteInfo!=NULL && pDev->spriteInfo->sprite!=NULL) {
+pSprite = pDev->spriteInfo->sprite;
+pSprite->hotLimits.x2 = mode->HDisplay;
+pSprite->hotLimits.y2 = mode->VDisplay;
+pSprite->physLimits.x2 = mode->HDisplay;
+pSprite->physLimits.y2 = mode->VDisplay;
+}
+}
 return TRUE;
 }

Cheers
Antoine

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] xf86-video-dummy: fix pointer limits when resizing

[Antoine Martin]

Hi,

Pointer events are clipped to the original screen dimensions with the
dummy driver when using RandRto switch to a higher resolution at runtime.

I don't know if this is the correct fix for it (is there a more generic
solution?), but this has been used extensively over the last year
without any noticeable side effects:

--- xf86-video-dummy-0.3.6/src/dummy_driver.c2014-11-05
19:24:02.668656601 +0700
+++ xf86-video-dummy-0.3.6.new/src/dummy_driver.c2014-11-05
19:37:53.076061853 +0700
@@ -55,6 +55,9 @@
 #include 
 #endif
 
+/* Needed for fixing pointer limits on resize */
+#include "inputstr.h"
+
 /* Mandatory functions */
 static const OptionInfoRec *DUMMYAvailableOptions(int chipid, int
busid);
 static void DUMMYIdentify(int flags);
@@ -713,6 +716,26 @@
 RRTellChanged(pScrn->pScreen);
 }
 #endif
+//ensure the screen dimensions are also updated:
+pScrn->pScreen->width = mode->HDisplay;
+pScrn->pScreen->height = mode->VDisplay;
+pScrn->virtualX = mode->HDisplay;
+pScrn->virtualY = mode->VDisplay;
+pScrn->frameX1 = mode->HDisplay;
+pScrn->frameY1 = mode->VDisplay;
+
+//ensure the pointer uses the new limits too:
+DeviceIntPtr pDev;
+SpritePtr pSprite;
+for (pDev = inputInfo.devices; pDev; pDev = pDev->next) {
+if (pDev->spriteInfo!=NULL && pDev->spriteInfo->sprite!=NULL) {
+pSprite = pDev->spriteInfo->sprite;
+pSprite->hotLimits.x2 = mode->HDisplay;
+pSprite->hotLimits.y2 = mode->VDisplay;
+pSprite->physLimits.x2 = mode->HDisplay;
+pSprite->physLimits.y2 = mode->VDisplay;
+}
+}
 return TRUE;
 }
 

Cheers
Antoine

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xf86-video-dummy 0/6] Cleanups and RandR 1.2 support

[Antoine Martin]

Hello,

On 25/01/15 08:08, Aaron Plattner wrote:
> This series is a counterproposal to Nicolas's patch "dummy: Add support for
> custom resolutions (RandR 1.2)" [1]
>
> This version allows using xrandr's --fb option to resize the screen rather 
> than
> having to try to create fake modes that pass validation against made-up 
> hardware
> constraints.  It also allows resizing the framebuffer all the way up to
> 32767x34767 (assuming malloc succeeds).
>
> I didn't try to test the interaction of that with DGA and opted instead to 
> just
> get rid of DGA support.  Let me know if you think DGA in xf86-video-dummy is
> actually useful for something and I can try to see if that makes sense.
If I understand what this is supposed to allow us to do... I like it.

The patches still apply cleanly, but when I try to use the dummy driver,
it crashes in libpixman.
Am I missing something?

$ gdb --args /usr/libexec/Xorg  -noreset -nolisten tcp +extension GLX
+extension RANDR +extension RENDER -auth $XAUTHORITY -logfile
${HOME}/.xpra/Xorg.10.log -config /etc/xpra/xorg.conf :10
GNU gdb (GDB) Fedora 7.9.1-17.fc22
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later

This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/libexec/Xorg...Reading symbols from
/usr/lib/debug/usr/libexec/Xorg.debug...done.
done.
(gdb) r
Starting program: /usr/libexec/Xorg -noreset -nolisten tcp +extension
GLX +extension RANDR +extension RENDER -auth
/run/user/1000/gdm/Xauthority -logfile /home/antoine/.xpra/Xorg.10.log
-config /etc/xpra/xorg.conf :10
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

X.Org X Server 1.17.2
Release Date: 2015-06-16
X Protocol Version 11, Revision 0
Build Operating System:  4.0.4-202.fc21.x86_64
Current Operating System: Linux desktop 4.1.6-200.fc22.x86_64 #1 SMP Mon
Aug 17 19:54:31 UTC 2015 x86_64
Kernel command line: BOOT_IMAGE=/vmlinuz-4.1.6-200.fc22.x86_64
root=LABEL=SSDRAIDROOT ro domdadm rd.blacklist=nouveau nomodeset
rd.md.uuid=730e916f:abc3f9b9:f5b2eb84:294a44aa rootfstype=ext4
LANG=en_GB.UTF-8
Build Date: 15 July 2015  08:16:41AM
Build ID: xorg-x11-server 1.17.2-2.fc22
Current version of pixman: 0.32.6
Before reporting problems, check http://wiki.x.org
to make sure that you have the latest version.
Markers: (--) probed, (**) from config file, (==) default setting,
(++) from command line, (!!) notice, (II) informational,
(WW) warning, (EE) error, (NI) not implemented, (??) unknown.
(++) Log file: "/home/antoine/.xpra/Xorg.10.log", Time: Wed Sep 16
19:14:06 2015
(++) Using config file: "/etc/xpra/xorg.conf"
(==) Using config directory: "/etc/X11/xorg.conf.d"
(==) Using system config directory "/usr/share/X11/xorg.conf.d"

Program received signal SIGSEGV, Segmentation fault.
sse2_fill (imp=, bits=, stride=, bpp=, x=, y=,
width=8192, height=4068, filler=0) at pixman-sse2.c:3403
3403save_128_aligned ((__m128i*)(d + 16),  xmm_def);
(gdb) bt
#0  sse2_fill (imp=, bits=,
stride=, bpp=, x=,
y=, width=8192, height=4068, filler=0) at pixman-sse2.c:3403
#1  0x76aff91b in _pixman_implementation_fill (imp=0x8398c0,
bits=bits@entry=0x865070, stride=stride@entry=8192, bpp=bpp@entry=32,
x=x@entry=0, y=y@entry=0, width=8192, height=4096, filler=) at pixman-implementation.c:277
#2  0x76ab1869 in pixman_fill (bits=bits@entry=0x865070,
stride=stride@entry=8192, bpp=bpp@entry=32, x=x@entry=0, y=y@entry=0,
width=width@entry=8192, height=4096, filler=0) at pixman.c:766
#3  0x7fffef375448 in fbFill (pDrawable=pDrawable@entry=0x87ab30,
pGC=pGC@entry=0x878c90, x=x@entry=0, y=y@entry=0,
width=width@entry=8192, height=height@entry=4096) at fbfill.c:125
#4  0x7fffef375cb0 in fbPolyFillRect (pDrawable=0x87ab30,
pGC=0x878c90, nrect=, prect=0x91da58) at fbfillrect.c:72
#5  0x0051fec8 in damagePolyFillRect (pDrawable=0x87ab30,
pGC=0x878c90, nRects=1, pRects=) at damage.c:1193
#6  0x00578d12 in miPaintWindow (pWin=,
pWin@entry=0x87ab30, prgn=prgn@entry=0x7fffdc00, what=what@entry=0)
at miexpose.c:560
#7  0x00579541 in miWindowExposures (pWin=0x87ab30,
prgn=0x7fffdc00) at miexpose.c:395
#8  0x00467547 in MapWindow (pWin=0x87ab30, client=0x83bb00) at
window.c:2600
#9  0x0043e560 in dix_main (argc=17, argv=0x7fffdd98,
envp=) at main.c:263
#10 0x75996700 in __libc_start_main 

dummy driver obvious DacSpeed fix

[Antoine Martin]

Hello,

The dummy driver doesn't honour the "DacSpeed" config file option.
It sets a local "maxClock" variable, but doesn't use it.
This fixes it, and allowed me to run tests with resolutions up to
2x2:

--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -391,7 +391,7 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
 clockRanges->next = NULL;
 clockRanges->ClockMulFactor = 1;
 clockRanges->minClock = 11000;   /* guessed §§§ */
-clockRanges->maxClock = 30;
+clockRanges->maxClock = maxClock;
 clockRanges->clockIndex = -1;  /* programmable */
 clockRanges->interlaceAllowed = TRUE;
 clockRanges->doubleScanAllowed = TRUE;

Note: this would change the default from 30 to 23 (different
values hardcoded in two places).
To keep the current default, just update the (currently unused) value
defined at the top of the function to read:
int maxClock = 30;

Please apply. (I would rather avoid the git pull dance for this trivial fix)

Cheers
Antoine

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

RFC: dummy driver RandR improvements?

[Antoine Martin]

Hi,

To get xpra[1] to mirror the client's screen configuration, we use the
dummy driver as Xdummy[2].
This works well enough generally, but there are a number of niggling
issues we would like to fix:

* Limited RandR support:
http://lists.freedesktop.org/archives/xorg-devel/2011-October/026596.html
We can use XRRSetScreenConfigAndRate with some pre-defined resolutions
in xorg.conf, but we cannot use XRRSetScreenSize as this is not
supported by the dummy driver. This means that the server resolution
often does not match the client's, and also causes some problems with DPI:

* DPI: although we try to set the Xft.dpi property to a value suitable
for the client, there are many applications out there that will ignore
this value and query the X11 server directly and calculate the DPI
themselves.. (ie: Java)
Since the X11 server assumes that the virtual screen's physical
dimension is fixed (set in xorg.conf), the dpi will vary widely if you
connect from a VGA client or a 4K client... Even though most of the
time, we want 96dpi or so.

* Multi-screen setups: ideally, we want to mirror the client's layout
down to the number of screens and their exact dimensions, but again the
dummy driver only supports one screen, and using Xinerama is not a
viable option (no randr, etc). This causes problems with applications
that go fullscreen especially.


I believe the first two problems can be resolved by adding
XRRSetScreenSize support to the dummy driver. I don't think this is too
controversial, is it?
Any code pointers would be much appreciated. Is there a driver I can use
as a clean reference implementation?

The multi-screen issue is more tricky, using Xinerama on top of Xephyr
is not an option since it would not be dynamic. libfakexinerama[3] is
unreachable right now ( Bad Gateway). Anyway, LD_PRELOAD hacks are
useful but not really suitablefor use in production.
Some window managers have their own way of faking xinerama screens:
enlightenment [4] does. Some of the proprietary drivers have their own
fake xinerama options too.
Wouldn't it make sense to unify this and do it more properly somehow? Or
can you perhaps suggest a different approach we can take?

Thanks
Antoine

[1] xpra
http://xpra.org/
[2] Xdummy
http://xpra.org/trac/wiki/Xdummy
[3] libfakexinerama
http://home.kde.org/~seli/fakexinerama/
[4] enlightenment fake xinerama screens
http://git.enlightenment.org/core/enlightenment.git/commit/x-ui.sh?id=7544376cbd2a344a13d50c38f39d9e56aea02c32
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: display using unix-domain socket

[Antoine Martin]

 Now might be a good time to revive this patch which has been submitted
 before by Timo Juhani Lindfors (CCed):
 https://www.xpra.org/trac/attachment/ticket/172/X11-unix-domain-socket.patch
 
 It would be very useful for us to avoid having to find a free display
 TCP port to use with the Vfb X11 server (Xdummy), we could just allocate
 a private socket as needed.
 
 I suspect that the concerns will be security related. I don't mind
 coding and testing to ensure this new display mode cannot be abused, but
 at this point I am not entirely sure that any checks are actually needed.
Attached is an ugly minimal patch to xcb-util's xcb_parse_display and
xcb_open so that one can specify a full unix paths to the server's
socket, ie:
DISPLAY=:unix:/tmp/.X11-unix/X10 xlsclients
or if you use your own socket location (chroot or otherwise):
DISPLAY=:unix:/private/socket xlsclients

Are there any good reasons against it? (after a thorough clean up or
even a complete rewrite obviously). I can't think of a good one.
After all, the clients should be able to choose where they connect to
outside of the pre-defined /tmp/.X11-unix/ socket location without
having to resort to ugly tricks like this one (same result):
socat UNIX-LISTEN:/tmp/.X11-unix/:666 UNIX-CONNECT:/private/socket
DISPLAY=:666 xlsclients

Cheers
Antoine
--- a/src/xcb_util.c	2012-10-08 00:34:47.768142969 +0700
+++ b/src/xcb_util.c	2012-10-08 00:20:08.132927120 +0700
@@ -89,6 +86,22 @@
 if(!name)
 return 0;
 
+fprintf(stderr, _xcb_parse_display name=%s\n, name);
+if(strncmp(name, :unix:, 6) == 0) {
+//take shortcut for simplicity
+fprintf(stderr, found unix domain socket: %s\n, name+6);
+*protocol = malloc(5);
+strncpy(*protocol, unix, 5);
+*host = malloc(strlen(name)-6);
+if(!*host)
+goto error_out;
+strncpy(*host, name+6, strlen(name)-6);
+*displayp = -1;			//we don't do numbers here...
+if(screenp)
+*screenp = screen;
+return 1;
+}
+
 #ifdef HAVE_LAUNCHD
 if(strncmp(name, /tmp/launch, 11) == 0)
 slash = NULL;
@@ -207,6 +223,13 @@
 }
 #endif
 
+if (*host == '/') {
+file = malloc(strlen(host));
+strncpy(file, host, strlen(host));
+filelen = strlen(file);
+}
+else {
+
 filelen = strlen(base) + 1 + sizeof(display) * 3 + 1;
 file = malloc(filelen);
 if(file == NULL)
@@ -226,6 +249,7 @@
 }
 /* snprintf may truncate the file */
 filelen = MIN(actual_filelen, filelen - 1);
+}
 #ifdef HAVE_ABSTRACT_SOCKETS
 fd = _xcb_open_abstract(protocol, file, filelen);
 if (fd = 0 || (errno != ENOENT  errno != ECONNREFUSED))
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: display using unix-domain socket (was Re: fd passing for X)

[Antoine Martin]

On 10/07/2012 07:06 PM, Michal Suchanek wrote:
 On 6 October 2012 18:16, Antoine Martin anto...@nagafix.co.uk wrote:
 On 10/06/2012 10:42 PM, Michal Suchanek wrote:
 On 6 October 2012 12:32, Antoine Martin anto...@nagafix.co.uk wrote:
 On 10/06/2012 05:15 PM, Michal Suchanek wrote:

 If the user is choosing a port or file path the user then needs to go
 through the dances of determining which port or path is still
 available for starting a new X server. This is just moved from the X
 server to user.
 Important note: no ports are involved here, just paths.
 There is a point in letting the X11 server do this allocation for TCP
 ports (no changes here), which are in limited supply and need to be
 allocated atomically, but none of this applies to unix domain sockets.

 Paths may be also in limited supply depending on acceptable paths.
 See below.
 One of the main benefits of being able to choose your own path, is that
 you can be sure that it exists and that it is free. (but I am repeating
 myself)

 2) That socket path can be totally private, using a file path with very
 restrictive permissions if you wish to do so.

 That's definitely an advantage.


 As far as I understand the patch shifts the burden of finding a free
 port (or socket path) from the X server to the user.
 Yes, it does. If by burden you mean freedom.

 You have to somehow determine if the path you picked is free or in use
 by earlier X server or another program.
 Think about two scripts trying to start an X server at the same time, etc.
 ie:
 SOCKET=`mktemp -d`/X11-socket
 Not hard, is it? No parsing the output, no go throught the dances of
 determining which (..) path is still available...

 Indeed, if you are fine with random nonsensical socket path then
 available sockets are unlimited.
 Define nonsensical. I'm fine with mktemp names for my use case for
 example: in this context, it's not nonsensical at all, it's what you ask
 for.
 The fact that it may seem nonsensical to others in this case is
 actually a feature, not a bug.

 Rather than using nonsensical, I would much prefer using the terms
 user defined and potentially private, long but this makes it far less
 of a loaded question.

 If you wanted some numbered sockets similar to what the X server uses
 now then the sockets have the same problem ports have. It's somewhat
 hard to find a X server you started previously and to associate random
 pieces of gibberish with multiple instances of running X server but
 lsof and inspecting the X sessions might help there.
 I don't know what random pieces of gibberish you are thinking of,
 please provide more concrete objections, these purely hypothetical use
 cases do not seem very likely to me.

 You seem to be arguing that if I choose to start a private server, using
 a private socket, it cannot be found easily? Well, yes, and that is the
 point really.

 Also some planning with picking only partially random socket names might 
 help.
 If you want to do that, you are welcome to, but I don't see what this
 has to do with this patch.
 If you write a piece of software that makes use of this patch *and*
 somehow needs to cooperate with whatever else by making those sockets
 public or shared, then by all means go ahead and do that.

 Bad analogy: I don't see many pieces of software out there preventing
 you from saving your documents in certain places because you might not
 remember what you did them. This is *nix. This patch does not change
 any of the default behaviour, you have to explicitly use it.

 Also note that adding this feature of partially random socket
 (currently totally unused and imho with no valid uses cases), would
 probably increase the level of complexity of this patch tenfold if not
 more (it is only 3 lines after all).

 
 no, it only requires a better example.
 
 $ SOCKET=`mktemp -d`/X11-socket
 $ echo $SOCKET
 /tmp/tmp.Ooa6YzzSfJ/X11-socket
 $ SOCKET=`mktemp -d`/X11-socket
 $ echo $SOCKET
 /tmp/tmp.Ery2iRjLqt/X11-socket
 
 Now you have two totally nonsensical socket names and you can't tell
 which one is the first and which one is the second X server. Note that
 they are actually reversed alphabetically.
You're still using the inappropriate term nonsensical to build up your
straw man argument, so one more time: if you chose a random name, you
get a random name. And you should call it random.
Don't want it? Don't use it.
Or maybe you have an app that wants to do something clever with this?
Then please share. Until then... this is as relevant as unicorns.

 If you used SOCKET=`mktemp -d`/X11-socket-0 and then increased the
 number you would have an identification of X servers created by the
 same application but X servers created by different applications or
 stale sockets would still mix together. Anyway, if that's what you ask
 for it's probably fine if you get that. I just would not be enormously
 excited about any application using this feature to spawn Xvfb or
 whatever as opposed to grabbing first free display number

Re: display using unix-domain socket

[Antoine Martin]

On 10/08/2012 12:59 AM, Alan Coopersmith wrote:
 On 10/ 7/12 10:53 AM, Antoine Martin wrote:
 Are there any good reasons against it? (after a thorough clean up or
 even a complete rewrite obviously). I can't think of a good one.
 
 How are you going to handle authentication for this?   Purely using the
 user based authentication?   Or allowing magic cookies if you force people
 to have a different xauthority file for each connection using this so two
 servers using display :666 on different sockets don't overwrite each others
 cookies?
 
For my personal use case, user based authentication is enough but since
this is in no small part about keeping things more private, it also
makes sense to use a dedicated xauthority file (although out of the box
I think SELinux may get in the way of that somewhat)
It seems to me that if one chooses to use custom socket locations, one
should be prepared to deal with a custom xauth location to go with it?
But I am open to suggestions on how best to proceed for other potential
use cases.

Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

display using unix-domain socket (was Re: fd passing for X)

[Antoine Martin]

On 10/06/2012 03:46 AM, Keith Packard wrote:
 
 I did a bunch of experiments with fd passing in Linux to see how it
 might work in X.
 
 Here's the results:
 
 http://keithp.com/blogs/fd-passing/
 
 The bottom line is that I think it'll be easy to get fds from
 applications to the X server, and a bit harder to get fds from the X
 server back to applications.
Now might be a good time to revive this patch which has been submitted
before by Timo Juhani Lindfors (CCed):
https://www.xpra.org/trac/attachment/ticket/172/X11-unix-domain-socket.patch

It would be very useful for us to avoid having to find a free display
TCP port to use with the Vfb X11 server (Xdummy), we could just allocate
a private socket as needed.

I suspect that the concerns will be security related. I don't mind
coding and testing to ensure this new display mode cannot be abused, but
at this point I am not entirely sure that any checks are actually needed.

Cheers
Antoine

PS: sorry for hijacking thread - seems somewhat related and I had been
meant to post this question for ages.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: display using unix-domain socket (was Re: fd passing for X)

[Antoine Martin]

On 10/06/2012 04:56 PM, Michal Suchanek wrote:
 Hello,
 
 What's the advantage of that patch?
1) You can use a socket path you create in advance, so you know for
certain that it is free when you call the server.
You then know for certain that this is what will be used by the server,
not some free port you have no way of guessing correctly.

2) That socket path can be totally private, using a file path with very
restrictive permissions if you wish to do so.

 As far as I understand the patch shifts the burden of finding a free
 port (or socket path) from the X server to the user.
Yes, it does. If by burden you mean freedom.

 X is in the position to atomically find a free port and grab it
 whereas an user from outside of the X server will have much harder
 time achieving that.
These aren't ports we're talking about, but unix domain sockets, see above.

 Of course, controlling what port the X server uses directly by the
 user can have its advantages but it does not help with finding a free
 one.
OT.

Cheers
Antoine
 
 Thanks
 
 Michal
 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: display using unix-domain socket (was Re: fd passing for X)

[Antoine Martin]

On 10/06/2012 05:15 PM, Michal Suchanek wrote:
 On 6 October 2012 12:02, Antoine Martin anto...@nagafix.co.uk wrote:
 On 10/06/2012 04:56 PM, Michal Suchanek wrote:
 Hello,

 What's the advantage of that patch?
 1) You can use a socket path you create in advance, so you know for
 certain that it is free when you call the server.
 You then know for certain that this is what will be used by the server,
 not some free port you have no way of guessing correctly.
 
 There is an option for the X server to print the display name from
 which it should be possible to infer the display port.
Since I want to chose the *unix-domain-socket* that will be used, this
feature is not useful. Or are you advocating that the X11 server should
chose the unix-domain socket path to use itself? Seriously? Now, that
would be a lot more complicated, and I don't see any real benefits either.

 If the user is choosing a port or file path the user then needs to go
 through the dances of determining which port or path is still
 available for starting a new X server. This is just moved from the X
 server to user.
Important note: no ports are involved here, just paths.
There is a point in letting the X11 server do this allocation for TCP
ports (no changes here), which are in limited supply and need to be
allocated atomically, but none of this applies to unix domain sockets.

 2) That socket path can be totally private, using a file path with very
 restrictive permissions if you wish to do so.
 
 That's definitely an advantage.
 

 As far as I understand the patch shifts the burden of finding a free
 port (or socket path) from the X server to the user.
 Yes, it does. If by burden you mean freedom.
 
 You have to somehow determine if the path you picked is free or in use
 by earlier X server or another program.
 Think about two scripts trying to start an X server at the same time, etc.
ie:
SOCKET=`mktemp -d`/X11-socket
Not hard, is it? No parsing the output, no go throught the dances of
determining which (..) path is still available...

Antoine

 For display ports the X server does this for you and outputs the port
 number when allocated if asked to.
 
 Thanks
 
 Michal

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: display using unix-domain socket (was Re: fd passing for X)

[Antoine Martin]

On 10/06/2012 10:42 PM, Michal Suchanek wrote:
 On 6 October 2012 12:32, Antoine Martin anto...@nagafix.co.uk wrote:
 On 10/06/2012 05:15 PM, Michal Suchanek wrote:
 
 If the user is choosing a port or file path the user then needs to go
 through the dances of determining which port or path is still
 available for starting a new X server. This is just moved from the X
 server to user.
 Important note: no ports are involved here, just paths.
 There is a point in letting the X11 server do this allocation for TCP
 ports (no changes here), which are in limited supply and need to be
 allocated atomically, but none of this applies to unix domain sockets.
 
 Paths may be also in limited supply depending on acceptable paths.
See below.
One of the main benefits of being able to choose your own path, is that
you can be sure that it exists and that it is free. (but I am repeating
myself)

 2) That socket path can be totally private, using a file path with very
 restrictive permissions if you wish to do so.

 That's definitely an advantage.


 As far as I understand the patch shifts the burden of finding a free
 port (or socket path) from the X server to the user.
 Yes, it does. If by burden you mean freedom.

 You have to somehow determine if the path you picked is free or in use
 by earlier X server or another program.
 Think about two scripts trying to start an X server at the same time, etc.
 ie:
 SOCKET=`mktemp -d`/X11-socket
 Not hard, is it? No parsing the output, no go throught the dances of
 determining which (..) path is still available...
 
 Indeed, if you are fine with random nonsensical socket path then
 available sockets are unlimited.
Define nonsensical. I'm fine with mktemp names for my use case for
example: in this context, it's not nonsensical at all, it's what you ask
for.
The fact that it may seem nonsensical to others in this case is
actually a feature, not a bug.

Rather than using nonsensical, I would much prefer using the terms
user defined and potentially private, long but this makes it far less
of a loaded question.

 If you wanted some numbered sockets similar to what the X server uses
 now then the sockets have the same problem ports have. It's somewhat
 hard to find a X server you started previously and to associate random
 pieces of gibberish with multiple instances of running X server but
 lsof and inspecting the X sessions might help there.
I don't know what random pieces of gibberish you are thinking of,
please provide more concrete objections, these purely hypothetical use
cases do not seem very likely to me.

You seem to be arguing that if I choose to start a private server, using
a private socket, it cannot be found easily? Well, yes, and that is the
point really.

 Also some planning with picking only partially random socket names might help.
If you want to do that, you are welcome to, but I don't see what this
has to do with this patch.
If you write a piece of software that makes use of this patch *and*
somehow needs to cooperate with whatever else by making those sockets
public or shared, then by all means go ahead and do that.

Bad analogy: I don't see many pieces of software out there preventing
you from saving your documents in certain places because you might not
remember what you did them. This is *nix. This patch does not change
any of the default behaviour, you have to explicitly use it.

Also note that adding this feature of partially random socket
(currently totally unused and imho with no valid uses cases), would
probably increase the level of complexity of this patch tenfold if not
more (it is only 3 lines after all).

It seems to me that you are trying to replicate the behaviour of TCP
sockets, because that's what's there, not because it makes technical sense.

Cheers
Antoine


 
 Thanks
 
 Michal
 ___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel
 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Antoine Martin]

On 03/27/2012 06:59 AM, Sérgio Basto wrote:
 On Mon, 2012-03-26 at 16:55 -0700, Jamey Sharp wrote: 
 2012/3/26 Sérgio Basto ser...@serjux.com:
 in other day I saw /usr/bin/xvfb-run is in use on building
 miro-4.0.6-2.fc16

 how we test X in a server without displays ?

 Use xf86-video-dummy with the Xorg DDX.
 
 where is the code ? 
 I need some examples to understand what you are saying ! 
 
 start a whole X with a xf86-video-dummy instead of Xvfb :1 ? 
https://xpra.org/Xdummy.html#usage

Antoine
(+1 for removing Xvfb)


 I use
 /usr/bin/Xvfb

 Xvfb :1 -fbdir /var/tmp/
 export DISPLAY=:1
 sdl-gnash  --screenshot 0 --screenshot-file $IMG.tmp -r1 -t1 $SWF

 kind of a flash ripper .

 xf86-video-dummy doesn't have an equivalent to -fbdir, but it looks
 like you aren't actually making use of the mmap'd framebuffer in
 /var/tmp/. So this case should work just fine with xf86-video-dummy
 today as well.

 Jamey
 

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Antoine Martin]

(snip)
 At the very minimum we should ship a couple simple configs such as (guessing 
 at
 contents here, someone would need to test and refine):
(snip)
Here is the one I use often (mainly because dummy does not have proper
randr support yet - I have offered to help resolve this..):
https://xpra.org/xorg.conf

Shipping some useful default configs would not be a bad idea IMO.

As for Xorg vs suid, I suspect the distros will want to have control
over the changeover process, something along the lines of:
Xorg - Xorg.suid
Xorg.suid
Xorg.nosuid
Then, hopefully one day:
Xorg - Xorg.nosuid
(or just remove Xorg.suid at that point)

 Even better would be to ship Xephyr, Xvfb, Xnest as simple shell scripts that
 did basically Xorg -config Xorg-vfb.conf $*  (possibly with some argument
 manipulation for other arguments those supported).
+1 for shell scripts that try to do the-right-thing(tm), but from my
experience with xpra there are some tricky arguments to handle, ie:
Xvfb -screen 0 3840x2560x24+32 :10
I don't see how that can be handled with dummy at present...

Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PULL to discuss] Remove kdrive, Xnest, and Xvfb

[Antoine Martin]

On 03/28/2012 01:36 AM, Sérgio Basto wrote:
 On Tue, 2012-03-27 at 10:53 -0700, Alan Coopersmith wrote: 
 On 03/27/12 10:46 AM, Jamey Sharp wrote:
 You're probably thinking of the old X VNC servers that hacked up the
 server source tree. I believe those have been abandoned, but certainly
 this proposed change won't hurt them any more than they already have
 been.

 TigerVNC is alive and well, shipping both Xvnc standalone server and
 a vnc.so loadable module for Xorg, neither of which should be broken
 by this change.   It may be possible to eventually replace Xvnc with a
 Xorg server that loads the vnc module with the dummy video driver and
 some sort of appropriate input driver (I don't think void will cut it,
 nor running without input) - but given the GPL license on that code,
 I'm happiest keeping Xvnc as a completely separate binary.
 
 X11vnc is different of TigerVNC 
 URL : http://www.karlrunge.com/x11vnc/
 Summary : VNC server for the current X11 session

TigerVNC includes x0vncserver which does the same thing.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

adding RandR support to dummy (was adding a custom modeline with xrandr..)

[Antoine Martin]

On 10/24/2011 06:26 PM, Jamey Sharp wrote:
 On Mon, Oct 24, 2011 at 05:20:29PM +0700, Antoine Martin wrote:
 Whenever an xpra client connects to a server I want to resize the
 dummy xserver to match the client's resolution exactly. ... 
 Failed to change the screen configuration!
 
 Off-hand, I'd guess nobody has implemented RANDR support in 
 xf86-video-dummy. But since I don't know what drivers need to do
 for that, for all I know there could be some generic support in the
 core that's failing here, instead.
You are right, there is not a single line of RandR code in the dummy
driver so whatever is failing must be in core.
Interestingly this already allows the resolution to be changed (as
mentioned previously), it just does not allow adding new ones.

So I would like to add better RandR support to dummy which should be
relatively straightforward, but I cannot find any reference
implementation or documentation on which methods need to be
implemented and how.. I found that drivers include xorg/xf86RandR12.h
Calling xf86RandR12Init / PreInit only got me segfaults so far..
Are there any useful pointers to help me get started?

Thanks
Antoine



 
 Jamey
 
 
 
 ___ 
 xorg-devel@lists.x.org: X.Org development Archives:
 http://lists.x.org/archives/xorg-devel Info:
 http://lists.x.org/mailman/listinfo/xorg-devel

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH V4] xserver: check for elevated privileges not uid=0

[Antoine Martin]

On 12/17/2011 12:45 AM, Keith Packard wrote:
 On Fri, 16 Dec 2011 12:09:14 -0500, Adam Jackson a...@nwnk.net wrote:
 
 Reviewed-by: Adam Jackson a...@redhat.com
 
 Thanks for the review, Adam. Of course, with the changes to the
 function detection logic in configure.ac, the original patch no longer
 applies to master.
 
 If someone could rebase this to master, I'd like to get this merged now.
OK, will get back to you with an updated patch asap.

Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH V4] xserver: check for elevated privileges not uid=0

[Antoine Martin]

On 12/17/2011 12:45 AM, Keith Packard wrote:
 On Fri, 16 Dec 2011 12:09:14 -0500, Adam Jackson a...@nwnk.net wrote:
 
 Reviewed-by: Adam Jackson a...@redhat.com
 
 Thanks for the review, Adam. Of course, with the changes to the
 function detection logic in configure.ac, the original patch no longer
 applies to master.
 
 If someone could rebase this to master, I'd like to get this merged now.
Updated patch attached.

Antoine
From 99ade3fda128bc6363735c289eb1c877d39ddecc Mon Sep 17 00:00:00 2001
From: Antoine Martin anto...@nagafix.co.uk
Date: Sat, 17 Dec 2011 01:36:51 +0700
Subject: [PATCH xserver] xserver: check for elevated privileges not uid=0

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html

Signed-off-by: Antoine Martin anto...@nagafix.co.uk
Tested-by: Michal Suchanek hramrach at centrum.cz
Reviewed-by: Jamey Sharp jamey at minilop.net
Reviewed-by: Adam Jackson a...@redhat.com 
---
 configure.ac   |2 +-
 hw/xfree86/common/xf86Config.c |   28 +++---
 hw/xfree86/common/xf86Init.c   |   78 +++-
 hw/xfree86/common/xf86Priv.h   |1 +
 include/xorg-config.h.in   |6 +++
 5 files changed, 91 insertions(+), 24 deletions(-)

diff --git a/configure.ac b/configure.ac
index 27bf6ab..518eb06 100644
--- a/configure.ac
+++ b/configure.ac
@@ -212,7 +212,7 @@ AC_CHECK_FUNC([dlopen], [],
 AC_SUBST(DLOPEN_LIBS)
 
 dnl Checks for library functions.
-AC_CHECK_FUNCS([backtrace ffs \
+AC_CHECK_FUNCS([backtrace ffs geteuid getuid issetugid getresuid \
getdtablesize getifaddrs getpeereid getpeerucred getzoneid \
mmap shmctl64 strncasecmp vasprintf vsnprintf walkcontext])
 AC_REPLACE_FUNCS([strcasecmp strcasestr strlcat strlcpy strndup])
diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index 569695c..f51be7e 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -72,8 +72,8 @@
  * These paths define the way the config file search is done.  The escape
  * sequences are documented in parser/scan.c.
  */
-#ifndef ROOT_CONFIGPATH
-#define ROOT_CONFIGPATH%A, %R, \
+#ifndef ALL_CONFIGPATH
+#define ALL_CONFIGPATH %A, %R, \
/etc/X11/%R, %P/etc/X11/%R, \
%E, %F, \
/etc/X11/%F, %P/etc/X11/%F, \
@@ -83,8 +83,8 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef USER_CONFIGPATH
-#define USER_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
+#ifndef RESTRICTED_CONFIGPATH
+#define RESTRICTED_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
/etc/X11/%G, %P/etc/X11/%G, \
/etc/X11/%X, /etc/%X, \
%P/etc/X11/%X.%H, \
@@ -92,14 +92,14 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef ROOT_CONFIGDIRPATH
-#define ROOT_CONFIGDIRPATH %A, %R, \
+#ifndef ALL_CONFIGDIRPATH
+#define ALL_CONFIGDIRPATH  %A, %R, \
/etc/X11/%R, %C/X11/%R, \
/etc/X11/%X, %C/X11/%X
 #endif
-#ifndef USER_CONFIGDIRPATH
-#define USER_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
-   /etc/X11/%X, %C/X11/%X
+#ifndef RESTRICTED_CONFIGDIRPATH
+#define RESTRICTED_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R, \
+   /etc/X11/%X, %C/X11/%X
 #endif
 #ifndef SYS_CONFIGDIRPATH
 #define SYS_CONFIGDIRPATH  /usr/share/X11/%X, %D/X11/%X
@@ -2310,12 +2310,12 @@ xf86HandleConfigFile(Bool autoconfig)
MessageType filefrom = X_DEFAULT;
MessageType dirfrom = X_DEFAULT;
 
-   if (getuid() == 0) {
-   filesearch = ROOT_CONFIGPATH;
-   dirsearch = ROOT_CONFIGDIRPATH;
+   if (!xf86PrivsElevated()) {
+   filesearch = ALL_CONFIGPATH;
+   dirsearch = ALL_CONFIGDIRPATH;
} else {
-   filesearch = USER_CONFIGPATH;
-   dirsearch = USER_CONFIGDIRPATH;
+   filesearch = RESTRICTED_CONFIGPATH;
+   dirsearch = RESTRICTED_CONFIGDIRPATH;
}
 
if (xf86ConfigFile)
diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index c1e48ee..5263b5f 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -238,6 +238,65 @@ xf86PrintMarkers(void)
   LogPrintMarkers();
 }
 
+Bool xf86PrivsElevated(void)
+{
+  static Bool privsTested = FALSE;
+  static Bool privsElevated = TRUE;
+
+  if (!privsTested) {
+#if defined(WIN32)
+privsElevated = FALSE;
+#else
+if ((getuid

Re: [PATCH V4] xserver: check for elevated privileges not uid=0

[Antoine Martin]

On 11/03/2011 11:16 AM, Keith Packard wrote:
 On Sat, 29 Oct 2011 20:47:16 +0700, Antoine Martin anto...@nagafix.co.uk 
 wrote:
 
 Keith, is there anything else I should do to get this merged?
 
 This kind of patch could really use some careful review by someone with
 a good knowledge of Posix security, given the sensitive nature of any
 kind of privilege checking code. I know there are many pitfalls in the
 various APIs, I just don't know what they are.
For what it's worth, this code is identical to the libX11 code already
merged, almost verbatim..
http://cgit.freedesktop.org/xorg/lib/libX11/tree/src/xlibi18n/lcFile.c#n221

 It's useful to have things tested, but in this case, I don't feel like
 it's sufficient to know whether there are corner cases which aren't
 correctly handled.
OK, I can't help with the review beyond what I have already done, but
what I can do is make it easier for others to verify/look at it.

Attached is a simple shell script which builds the xf86PrivsElevated()
function completely standalone, using all the locally available
combinations of:
* issetugid
* getresuid
* seteuid-test fallback
And then tests each resulting binary with:
* owned by root or not
* suid/non-suid
* as a regular user / as root

I have tested this on a number of systems/distros, some with SELinux,
apparmor, etc..

Thanks
Antoine




PS: the script uses sudo to chmod/chown the binaries, here's a sample
output:
$ ./test_privs.sh
**
building all variants (some may fail - that's OK):
**
/tmp/ccyEXRCj.o: In function `xf86PrivsElevated':
test_privs.c:(.text+0x59): undefined reference to `issetugid'
collect2: ld returned 1 exit status
failed to build issetugid
built getresuid variant
built fallbackseteuidtest variant

Testing as current user: 500:
-rwxrwxr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_getresuid.bin
xf86PrivsElevated()=0
-rwxrwxr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_fallbackseteuidtest.bin
xf86PrivsElevated()=0

Testing via sudo:
-rwxrwxr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_getresuid.bin
xf86PrivsElevated()=0
-rwxrwxr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_fallbackseteuidtest.bin
xf86PrivsElevated()=0

Testing suid non root:
-rwsrwsr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_getresuid.bin
xf86PrivsElevated()=0
-rwsrwsr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_fallbackseteuidtest.bin
xf86PrivsElevated()=0

Testing suid non root via sudo:
-rwsrwsr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_getresuid.bin
xf86PrivsElevated()=1
-rwsrwsr-x. 1 500 500 7719 Dec 14 17:00 ./test_privs_fallbackseteuidtest.bin
xf86PrivsElevated()=1

Testing suid binary as root:
-rwsrwsr-x. 1 0 0 7719 Dec 14 17:00 ./test_privs_getresuid.bin
xf86PrivsElevated()=0
-rwsrwsr-x. 1 0 0 7719 Dec 14 17:00 ./test_privs_fallbackseteuidtest.bin
xf86PrivsElevated()=0

Testing suid binary as current user:
-rwsrwsr-x. 1 0 0 7719 Dec 14 17:00 ./test_privs_getresuid.bin
xf86PrivsElevated()=1
-rwsrwsr-x. 1 0 0 7719 Dec 14 17:00 ./test_privs_fallbackseteuidtest.bin
xf86PrivsElevated()=1

Cleaning up



test_privs.sh
Description: application/shellscript
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: moving rootless out of the server

[Antoine Martin]

On 11/11/2011 08:59 AM, Jamey Sharp wrote:
 On 11/10/11, Alan Hourihane al...@fairlite.co.uk wrote:
 On 11/10/11 23:40, Peter Hutterer wrote:
 I wonder how much effort it would be to write a xf86-video-vnc that runs
 on xfree86 ddx.
 
 That would be interesting to try, and there's some library out there
 for quickly building a VNC server into an arbitrary app. Or Alan's
 suggestion;
 
 xf4vnc already does. Just use the xf86-video-dummy driver and you have a
 standalone setup.
 
 Or use a modern implementation like x11vnc that uses COMPOSITE so it
 doesn't need any server-side code at all. :-) That's the approach
 we're trying to adopt in this new project.
This approach sounds very similar to the one used by Xpra [1]: FYI it
registers as a compositing window manager to capture the windows' pixels
which it then forwards to potentially remote clients.
Have you considered supporting this over the network? Wouldn't this be
very useful in the context of Wayland?
Are you also going to forward the bell, custom cursors, etc?
(those still fit within X11 whereas supporting things like dbus /
notifications is much more tricky)

I have added memory mapped pixel transfers to Xpra recently [2]
(obviously, this only works when the client and server are on the same
machine) which improved performance significantly, although there is
still too much memory copying going on. I suspect your solution will
target zero-copy where possible?

In any case, I am interested in your code as it may provide a
better (and probably faster too) solution to some of the problems Xpra
has to deal with. Please keep me posted.

Thanks
Antoine


[1] http://xpra.org/
[2] http://xpra.org/trac/changeset/273

 
 Jamey
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xf86-video-dummy allow up to 32767x32767 (re-send)

[Antoine Martin]

On 05/25/2011 04:57 PM, Adam Jackson wrote:
 On 5/19/11 2:17 AM, Antoine Martin wrote:
 Hi Adam,

 Can you apply this patch please?
 I have tested this up to 16384x8192, higher resolutions make xtiming
 spew out garbage (integer overflow?) so I couldn't generate modelines.
 
 Apologies for letting this sit so long.  Pushed, thanks!
Any chance of tagging xf86-video-dummy 0.3.5 so that distros can start
to pick up those changes?

At the moment I am having to rebuild debs/rpms every time a distro
updates the server abi release.

Thanks
Antoine
 
 - ajax

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH V4] xserver: check for elevated privileges not uid=0

[Antoine Martin]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/18/2011 12:41 AM, Jamey Sharp wrote:
 Reviewed-by: Jamey Sharp ja...@minilop.net
 
 Since this has gone through so many changes I tried to carefully 
 re-review the whole thing, and it still looks good to me. Thanks, 
 Antoine!
 
 We don't actually put xserver: in the commit message for a
 server patch, though. It wouldn't be wrong to tag this xfree86:
 even though it touches autoconf-ery because it's really all for the
 benefit of the xfree86 DDX, as written; but it would be OK to leave
 off the tag entirely, too.
 
 The next trick is to get Keith to merge this patch, and/or get
 more reviewed-by/tested-by tags.
There's also this one (in a reply to this same thread):
Tested-by: Michal Suchanek hramr...@centrum.cz

Keith, is there anything else I should do to get this merged?

Thanks
Antoine




 
 Jamey
 
 On Tue, Oct 11, 2011 at 03:02:50PM +0700, Antoine Martin wrote:
 This allows us to run the server as a normal user whilst still 
 being able to use the -modulepath, -logfile and -config switches 
 We define a xf86PrivsElevated which will do the checks and cache 
 the result in case it is called more than once. Also renamed the
 paths #defines to match their new meaning. Original discussion
 which led to this patch can be found here: 
 http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html


 
Signed-off-by: Antoine Martin anto...@nagafix.co.uk
 ---
 
 Changes from the previous version of this patch: * use
 FatalError() rather than exit() (thx to Julien Cristau) * remove
 comment superseded by FatalError message (thx to Tormod Volden) *
 more defensive code: print warning and assume privs are elevated 
 if getresuid or getresgid fail, ensure all codepaths set 
 privsElevated explicitly and make it default to TRUE (in case we 
 ever miss one) * email title includes patch version in the right
 location (btw, there are changes in xserver's autoconf, not just
 hw/xfree86)
 
 Thanks Antoine
 
 configure.ac   |4 ++- 
 hw/xfree86/common/xf86Config.c |   28 +++--- 
 hw/xfree86/common/xf86Init.c   |   76
 +++- 
 hw/xfree86/common/xf86Priv.h   |1 + include/xorg-config.h.in
 |6 +++ 5 files changed, 91 insertions(+), 24 deletions(-)
 
 diff --git a/configure.ac b/configure.ac index 67a6836..a79e2be
 100644 --- a/configure.ac +++ b/configure.ac @@ -208,9 +208,11 @@
 AC_SUBST(DLOPEN_LIBS)
 
 dnl Checks for library functions. AC_FUNC_VPRINTF 
 -AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp
 strchr strrchr \ +AC_CHECK_FUNCS([geteuid getuid issetugid
 getresuid \ +link memmove memset mkstemp strchr strrchr \ 
 strtol getopt getopt_long vsnprintf walkcontext backtrace \ 
 getisax getzoneid shmctl64 strcasestr ffs vasprintf]) + 
 AC_FUNC_ALLOCA dnl Old HAS_* names used in os/*.c. 
 AC_CHECK_FUNC([getdtablesize], diff --git
 a/hw/xfree86/common/xf86Config.c
 b/hw/xfree86/common/xf86Config.c index f8c1b65..8ccc52a 100644 
 --- a/hw/xfree86/common/xf86Config.c +++
 b/hw/xfree86/common/xf86Config.c @@ -72,8 +72,8 @@ * These paths
 define the way the config file search is done.  The escape *
 sequences are documented in parser/scan.c. */ -#ifndef
 ROOT_CONFIGPATH -#define ROOT_CONFIGPATH %A, %R, \ +#ifndef
 ALL_CONFIGPATH +#define ALL_CONFIGPATH   %A, %R, \ 
 /etc/X11/%R, %P/etc/X11/%R, \ %E, %F, \ /etc/X11/%F,
 %P/etc/X11/%F, \ @@ -83,8 +83,8 @@ %P/lib/X11/%X.%H, \ 
 %P/lib/X11/%X #endif -#ifndef USER_CONFIGPATH -#define
 USER_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \ +#ifndef
 RESTRICTED_CONFIGPATH +#define RESTRICTED_CONFIGPATH
 /etc/X11/%S, %P/etc/X11/%S, \ /etc/X11/%G, %P/etc/X11/%G,
 \ /etc/X11/%X, /etc/%X, \ %P/etc/X11/%X.%H, \ @@ -92,14
 +92,14 @@ %P/lib/X11/%X.%H, \ %P/lib/X11/%X #endif -#ifndef
 ROOT_CONFIGDIRPATH -#define ROOT_CONFIGDIRPATH   %A, %R, \ 
 +#ifndef ALL_CONFIGDIRPATH +#define ALL_CONFIGDIRPATH%A, %R,
 \ /etc/X11/%R, %C/X11/%R, \ /etc/X11/%X, %C/X11/%X 
 #endif -#ifndef USER_CONFIGDIRPATH -#define USER_CONFIGDIRPATH
 /etc/X11/%R, %C/X11/%R, \ -  /etc/X11/%X, 
 %C/X11/%X 
 +#ifndef RESTRICTED_CONFIGDIRPATH +#define
 RESTRICTED_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \ +
 /etc/X11/%X, %C/X11/%X #endif #ifndef SYS_CONFIGDIRPATH 
 #define SYS_CONFIGDIRPATH/usr/share/X11/%X, %D/X11/%X @@
 -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig) Bool
 implicit_layout = FALSE;
 
 if (!autoconfig) { - if (getuid() == 0) { -  filesearch =
 ROOT_CONFIGPATH; -   dirsearch = ROOT_CONFIGDIRPATH; +   if
 (!xf86PrivsElevated()) { +   filesearch = ALL_CONFIGPATH; +
 dirsearch = ALL_CONFIGDIRPATH; } else { -filesearch =
 USER_CONFIGPATH; -   dirsearch = USER_CONFIGDIRPATH; +
 filesearch = RESTRICTED_CONFIGPATH; +dirsearch =
 RESTRICTED_CONFIGDIRPATH; }
 
 if (xf86ConfigFile) diff --git a/hw/xfree86/common/xf86Init.c
 b/hw/xfree86/common/xf86Init.c index

adding a custom modeline with xrandr (dummy driver)

[Antoine Martin]

Hi,

Whenever an xpra client connects to a server I want to resize the dummy
xserver to match the client's resolution exactly.
The server is started with:
Xorg +extension RANDR +extension RENDER -config xorg-dummy.conf
I calculate a modeline using:
http://xtiming.sourceforge.net/cgi-bin/xtiming.pl
(say 1128x738@50) and add it:
xrandr --newmode 1128x738@50 54.17 1128 1160 1360 1392 738 753 759 775
Then I add it to the default output:
xrandr --addmode default 1128x738@50
Then I try to actually set it:
xrandr -s 1128x738@50
And get this error message:
Failed to change the screen configuration!

Running the same commands against a real X11 server sort-of worked in
that it did set the resolution (I think - I got a black screen because
my LCD did not support it). Interestingly, it would not let me switch
back to the original resolution from a virtual terminal (same Failed to
change... error), I had to add a sleep 5 to delay it and switch back
to the X11 vt to make it work. Could it be something similar with my
dummy instance? vt related? How do I fix it?
Or, what am I doing wrong?

I have tried doing this via the API rather than the xrandr commandline
and did not fare any better:
* XRRAllocModeInfo to allocate a new mode
* XRRCreateMode to create it with calculations similar to xtimings
* XRRAddOutputMode on the only output found using
XRRGetScreenResourcesCurrent
Strangely, the new mode is not listed by XRRSizes()
And calling XRRSetScreenSize actually crashed the whole X11 server!..

Thanks
Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: adding a custom modeline with xrandr (dummy driver)

[Antoine Martin]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/24/2011 06:26 PM, Jamey Sharp wrote:
 On Mon, Oct 24, 2011 at 05:20:29PM +0700, Antoine Martin wrote:
 Whenever an xpra client connects to a server I want to resize the
 dummy xserver to match the client's resolution exactly. ... 
 Failed to change the screen configuration!
 
 Off-hand, I'd guess nobody has implemented RANDR support in 
 xf86-video-dummy. But since I don't know what drivers need to do
 for that, for all I know there could be some generic support in the
 core that's failing here, instead.
Well there is at least some support for RANDR, since I can already
switch resolutions to any of the pre-defined ones (the ones I define
in xorg.conf).

Antoine
 
 Jamey
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6lTGMACgkQGK2zHPGK1rtAUQCfT8dAhqBtOG7X3AFJ2dFRN2FI
Hr8AnjGxr9cTdODF6pEBbsBXKP6whOwm
=nVe1
-END PGP SIGNATURE-
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH V4] xserver: check for elevated privileges not uid=0

[Antoine Martin]

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html

Signed-off-by: Antoine Martin anto...@nagafix.co.uk


Changes from the previous version of this patch:
* use FatalError() rather than exit() (thx to Julien Cristau)
* remove comment superseded by FatalError message (thx to Tormod Volden)
* more defensive code: print warning and assume privs are elevated if 
getresuid or getresgid fail, ensure all codepaths set privsElevated 
explicitly and make it default to TRUE (in case we ever miss one)

* email title includes patch version in the right location
(btw, there are changes in xserver's autoconf, not just hw/xfree86)

Thanks
Antoine

---
 configure.ac   |4 ++-
 hw/xfree86/common/xf86Config.c |   28 +++---
 hw/xfree86/common/xf86Init.c   |   76 +++-
 hw/xfree86/common/xf86Priv.h   |1 +
 include/xorg-config.h.in   |6 +++
 5 files changed, 91 insertions(+), 24 deletions(-)

diff --git a/configure.ac b/configure.ac
index 67a6836..a79e2be 100644
--- a/configure.ac
+++ b/configure.ac
@@ -208,9 +208,11 @@ AC_SUBST(DLOPEN_LIBS)
 
 dnl Checks for library functions.
 AC_FUNC_VPRINTF
-AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp strchr strrchr \
+AC_CHECK_FUNCS([geteuid getuid issetugid getresuid \
+   link memmove memset mkstemp strchr strrchr \
strtol getopt getopt_long vsnprintf walkcontext backtrace \
getisax getzoneid shmctl64 strcasestr ffs vasprintf])
+
 AC_FUNC_ALLOCA
 dnl Old HAS_* names used in os/*.c.
 AC_CHECK_FUNC([getdtablesize],
diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index f8c1b65..8ccc52a 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -72,8 +72,8 @@
  * These paths define the way the config file search is done.  The escape
  * sequences are documented in parser/scan.c.
  */
-#ifndef ROOT_CONFIGPATH
-#define ROOT_CONFIGPATH%A, %R, \
+#ifndef ALL_CONFIGPATH
+#define ALL_CONFIGPATH %A, %R, \
/etc/X11/%R, %P/etc/X11/%R, \
%E, %F, \
/etc/X11/%F, %P/etc/X11/%F, \
@@ -83,8 +83,8 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef USER_CONFIGPATH
-#define USER_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
+#ifndef RESTRICTED_CONFIGPATH
+#define RESTRICTED_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
/etc/X11/%G, %P/etc/X11/%G, \
/etc/X11/%X, /etc/%X, \
%P/etc/X11/%X.%H, \
@@ -92,14 +92,14 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef ROOT_CONFIGDIRPATH
-#define ROOT_CONFIGDIRPATH %A, %R, \
+#ifndef ALL_CONFIGDIRPATH
+#define ALL_CONFIGDIRPATH  %A, %R, \
/etc/X11/%R, %C/X11/%R, \
/etc/X11/%X, %C/X11/%X
 #endif
-#ifndef USER_CONFIGDIRPATH
-#define USER_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
-   /etc/X11/%X, %C/X11/%X
+#ifndef RESTRICTED_CONFIGDIRPATH
+#define RESTRICTED_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R, \
+   /etc/X11/%X, %C/X11/%X
 #endif
 #ifndef SYS_CONFIGDIRPATH
 #define SYS_CONFIGDIRPATH  /usr/share/X11/%X, %D/X11/%X
@@ -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig)
 Bool implicit_layout = FALSE;
 
 if (!autoconfig) {
-   if (getuid() == 0) {
-   filesearch = ROOT_CONFIGPATH;
-   dirsearch = ROOT_CONFIGDIRPATH;
+   if (!xf86PrivsElevated()) {
+   filesearch = ALL_CONFIGPATH;
+   dirsearch = ALL_CONFIGDIRPATH;
} else {
-   filesearch = USER_CONFIGPATH;
-   dirsearch = USER_CONFIGDIRPATH;
+   filesearch = RESTRICTED_CONFIGPATH;
+   dirsearch = RESTRICTED_CONFIGDIRPATH;
}
 
if (xf86ConfigFile)
diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index 350918d..3fead6f 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -237,6 +237,63 @@ xf86PrintMarkers(void)
   LogPrintMarkers();
 }
 
+Bool xf86PrivsElevated(void)
+{
+  static Bool privsTested = FALSE;
+  static Bool privsElevated = TRUE;
+
+  if (!privsTested) {
+#if !defined(WIN32)
+if ((getuid() != geteuid()) || (getgid() != getegid())) {
+  privsElevated = TRUE;
+} else {
+#if defined(HAVE_ISSETUGID)
+  privsElevated = issetugid();
+#elif defined(HAVE_GETRESUID)
+  uid_t ruid, euid

[PATCH xserver] check for elevated privileges not uid=0 (V2)

[Antoine Martin]

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html


Changes from the previous version of this patch:
* moved variables inside the function (thx to Tormod Volden)
* fallback code (when both HASSETUGID and HASGETRESUID are unset) now 
correctly returns FALSE if uid is already 0. (thx to Michal Suchanek)

* consistently use TRUE constant rather than 1 value

You can find some ready made Fedora 15 test RPMs here:
http://xpra.org/src/Xdummy/
I have been using this patch for days without any visible side effects.
Can I please get some reviewed-by / acks?

Thanks
Antoine

---
 hw/xfree86/common/xf86Config.c |   28 
 hw/xfree86/common/xf86Init.c   |   65 ++-
 hw/xfree86/common/xf86Priv.h   |1 +
 3 files changed, 71 insertions(+), 23 deletions(-)

diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index f8c1b65..8ccc52a 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -72,8 +72,8 @@
  * These paths define the way the config file search is done.  The escape
  * sequences are documented in parser/scan.c.
  */
-#ifndef ROOT_CONFIGPATH
-#define ROOT_CONFIGPATH%A, %R, \
+#ifndef ALL_CONFIGPATH
+#define ALL_CONFIGPATH %A, %R, \
/etc/X11/%R, %P/etc/X11/%R, \
%E, %F, \
/etc/X11/%F, %P/etc/X11/%F, \
@@ -83,8 +83,8 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef USER_CONFIGPATH
-#define USER_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
+#ifndef RESTRICTED_CONFIGPATH
+#define RESTRICTED_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
/etc/X11/%G, %P/etc/X11/%G, \
/etc/X11/%X, /etc/%X, \
%P/etc/X11/%X.%H, \
@@ -92,14 +92,14 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef ROOT_CONFIGDIRPATH
-#define ROOT_CONFIGDIRPATH %A, %R, \
+#ifndef ALL_CONFIGDIRPATH
+#define ALL_CONFIGDIRPATH  %A, %R, \
/etc/X11/%R, %C/X11/%R, \
/etc/X11/%X, %C/X11/%X
 #endif
-#ifndef USER_CONFIGDIRPATH
-#define USER_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
-   /etc/X11/%X, %C/X11/%X
+#ifndef RESTRICTED_CONFIGDIRPATH
+#define RESTRICTED_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R, \
+   /etc/X11/%X, %C/X11/%X
 #endif
 #ifndef SYS_CONFIGDIRPATH
 #define SYS_CONFIGDIRPATH  /usr/share/X11/%X, %D/X11/%X
@@ -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig)
 Bool implicit_layout = FALSE;
 
 if (!autoconfig) {
-   if (getuid() == 0) {
-   filesearch = ROOT_CONFIGPATH;
-   dirsearch = ROOT_CONFIGDIRPATH;
+   if (!xf86PrivsElevated()) {
+   filesearch = ALL_CONFIGPATH;
+   dirsearch = ALL_CONFIGDIRPATH;
} else {
-   filesearch = USER_CONFIGPATH;
-   dirsearch = USER_CONFIGDIRPATH;
+   filesearch = RESTRICTED_CONFIGPATH;
+   dirsearch = RESTRICTED_CONFIGDIRPATH;
}
 
if (xf86ConfigFile)
diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index 350918d..7386177 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -237,6 +237,52 @@ xf86PrintMarkers(void)
   LogPrintMarkers();
 }
 
+Bool xf86PrivsElevated(void)
+{
+  static Bool privsTested = FALSE;
+  static Bool privsElevated = FALSE;
+  if (!privsTested) {
+#if !defined(WIN32)
+if ((getuid() != geteuid()) || (getgid() != getegid())) {
+  privsElevated = TRUE;
+} else {
+#if defined(HASSETUGID)
+  privsElevated = issetugid();
+#elif defined(HASGETRESUID)
+  uid_t ruid, euid, suid;
+  gid_t rgid, egid, sgid;
+  if ((getresuid(ruid, euid, suid) == 0) 
+  (getresgid(rgid, egid, sgid) == 0))
+privsElevated = (euid != suid) || (egid != sgid);
+#else
+  if (getuid()!=0) {
+/*
+ * If there are saved ID's the process might still be priviledged
+ * even though the above test succeeded.  If issetugid() and
+ * getresgid() aren't available, test this by trying to set
+ * euid to 0.
+ */
+unsigned int oldeuid;
+oldeuid = geteuid();
+if (seteuid(0) != 0) {
+  privsElevated = FALSE;
+} else {
+  if (seteuid(oldeuid) == -1) {
+/* XXX ouch, coudn't get back to original uid
+   what can we do ??? */
+

Re: [PATCH xserver] check for elevated privileges not uid=0 (V2)

[Antoine Martin]

Please ignore the patch sent in the previous email, it was missing the 
autoconf bits. The correct patch is now attached to this email.
I have copied the macros from the libX11 example and I have verified 
that the HASGETRESUID/(HASSETUGID) are defined during the build on F15 - 
still, I would prefer if someone could double check it..


The good thing is that this explains why Michal was using the fallback 
code when testing, bonus is that this got tested properly, otherwise it 
might not have.


Sorry it took so long to get it into shape..

Antoine


On 10/10/2011 09:07 PM, Antoine Martin wrote:

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html


Changes from the previous version of this patch:
* moved variables inside the function (thx to Tormod Volden)
* fallback code (when both HASSETUGID and HASGETRESUID are unset) now
correctly returns FALSE if uid is already 0. (thx to Michal Suchanek)
* consistently use TRUE constant rather than 1 value

You can find some ready made Fedora 15 test RPMs here:
http://xpra.org/src/Xdummy/
I have been using this patch for days without any visible side effects.
Can I please get some reviewed-by / acks?

Thanks
Antoine


From 7412ccc6c837d36956dc00a7a44e595496f652f9 Mon Sep 17 00:00:00 2001
From: Antoine Martin anto...@nagafix.co.uk
Date: Mon, 10 Oct 2011 23:27:29 +0700
Subject: [PATCH xserver] check for elevated privileges not uid=0 (V3)

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html

Signed-off-by: Antoine Martin anto...@nagafix.co.uk
---
 configure.ac   |7 
 hw/xfree86/common/xf86Config.c |   28 +-
 hw/xfree86/common/xf86Init.c   |   63 ++--
 hw/xfree86/common/xf86Priv.h   |1 +
 include/xorg-config.h.in   |6 
 5 files changed, 82 insertions(+), 23 deletions(-)

diff --git a/configure.ac b/configure.ac
index 67a6836..fe836cc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -211,6 +211,13 @@ AC_FUNC_VPRINTF
 AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp strchr strrchr \
strtol getopt getopt_long vsnprintf walkcontext backtrace \
getisax getzoneid shmctl64 strcasestr ffs vasprintf])
+
+dnl Checks for uid/gid functions used for testing elevated privileges
+AC_CHECK_FUNC([issetugid],
+AC_DEFINE(HASSETUGID,1,[Has issetugid() function]))
+AC_CHECK_FUNC([getresuid],
+AC_DEFINE(HASGETRESUID,1,[Has getresuid()  getresgid() functions]))
+
 AC_FUNC_ALLOCA
 dnl Old HAS_* names used in os/*.c.
 AC_CHECK_FUNC([getdtablesize],
diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index f8c1b65..8ccc52a 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -72,8 +72,8 @@
  * These paths define the way the config file search is done.  The escape
  * sequences are documented in parser/scan.c.
  */
-#ifndef ROOT_CONFIGPATH
-#define ROOT_CONFIGPATH%A, %R, \
+#ifndef ALL_CONFIGPATH
+#define ALL_CONFIGPATH %A, %R, \
/etc/X11/%R, %P/etc/X11/%R, \
%E, %F, \
/etc/X11/%F, %P/etc/X11/%F, \
@@ -83,8 +83,8 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef USER_CONFIGPATH
-#define USER_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
+#ifndef RESTRICTED_CONFIGPATH
+#define RESTRICTED_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
/etc/X11/%G, %P/etc/X11/%G, \
/etc/X11/%X, /etc/%X, \
%P/etc/X11/%X.%H, \
@@ -92,14 +92,14 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef ROOT_CONFIGDIRPATH
-#define ROOT_CONFIGDIRPATH %A, %R, \
+#ifndef ALL_CONFIGDIRPATH
+#define ALL_CONFIGDIRPATH  %A, %R, \
/etc/X11/%R, %C/X11/%R, \
/etc/X11/%X, %C/X11/%X
 #endif
-#ifndef USER_CONFIGDIRPATH
-#define USER_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
-   /etc/X11/%X, %C/X11/%X
+#ifndef RESTRICTED_CONFIGDIRPATH
+#define RESTRICTED_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R

[PATCH xserver] check for elevated privileges not uid=0

[Antoine Martin]

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html

Signed-off-by: Antoine Martin anto...@nagafix.co.uk


Changes from the previous version of this patch:
* static var location (again!), formatting, typos, error message, exit() 
(thx to Tormod Volden)

* use more generic AC_CHECK_FUNCS macro (thx to Alan Coopersmith)

Thanks
Antoine
---
 configure.ac   |4 ++-
 hw/xfree86/common/xf86Config.c |   28 
 hw/xfree86/common/xf86Init.c   |   68 ++-
 hw/xfree86/common/xf86Priv.h   |1 +
 include/xorg-config.h.in   |6 +++
 5 files changed, 83 insertions(+), 24 deletions(-)

diff --git a/configure.ac b/configure.ac
index 67a6836..a79e2be 100644
--- a/configure.ac
+++ b/configure.ac
@@ -208,9 +208,11 @@ AC_SUBST(DLOPEN_LIBS)
 
 dnl Checks for library functions.
 AC_FUNC_VPRINTF
-AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp strchr strrchr \
+AC_CHECK_FUNCS([geteuid getuid issetugid getresuid \
+   link memmove memset mkstemp strchr strrchr \
strtol getopt getopt_long vsnprintf walkcontext backtrace \
getisax getzoneid shmctl64 strcasestr ffs vasprintf])
+
 AC_FUNC_ALLOCA
 dnl Old HAS_* names used in os/*.c.
 AC_CHECK_FUNC([getdtablesize],
diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index f8c1b65..8ccc52a 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -72,8 +72,8 @@
  * These paths define the way the config file search is done.  The escape
  * sequences are documented in parser/scan.c.
  */
-#ifndef ROOT_CONFIGPATH
-#define ROOT_CONFIGPATH%A, %R, \
+#ifndef ALL_CONFIGPATH
+#define ALL_CONFIGPATH %A, %R, \
/etc/X11/%R, %P/etc/X11/%R, \
%E, %F, \
/etc/X11/%F, %P/etc/X11/%F, \
@@ -83,8 +83,8 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef USER_CONFIGPATH
-#define USER_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
+#ifndef RESTRICTED_CONFIGPATH
+#define RESTRICTED_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
/etc/X11/%G, %P/etc/X11/%G, \
/etc/X11/%X, /etc/%X, \
%P/etc/X11/%X.%H, \
@@ -92,14 +92,14 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef ROOT_CONFIGDIRPATH
-#define ROOT_CONFIGDIRPATH %A, %R, \
+#ifndef ALL_CONFIGDIRPATH
+#define ALL_CONFIGDIRPATH  %A, %R, \
/etc/X11/%R, %C/X11/%R, \
/etc/X11/%X, %C/X11/%X
 #endif
-#ifndef USER_CONFIGDIRPATH
-#define USER_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
-   /etc/X11/%X, %C/X11/%X
+#ifndef RESTRICTED_CONFIGDIRPATH
+#define RESTRICTED_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R, \
+   /etc/X11/%X, %C/X11/%X
 #endif
 #ifndef SYS_CONFIGDIRPATH
 #define SYS_CONFIGDIRPATH  /usr/share/X11/%X, %D/X11/%X
@@ -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig)
 Bool implicit_layout = FALSE;
 
 if (!autoconfig) {
-   if (getuid() == 0) {
-   filesearch = ROOT_CONFIGPATH;
-   dirsearch = ROOT_CONFIGDIRPATH;
+   if (!xf86PrivsElevated()) {
+   filesearch = ALL_CONFIGPATH;
+   dirsearch = ALL_CONFIGDIRPATH;
} else {
-   filesearch = USER_CONFIGPATH;
-   dirsearch = USER_CONFIGDIRPATH;
+   filesearch = RESTRICTED_CONFIGPATH;
+   dirsearch = RESTRICTED_CONFIGDIRPATH;
}
 
if (xf86ConfigFile)
diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index 350918d..063a2d8 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -237,6 +237,55 @@ xf86PrintMarkers(void)
   LogPrintMarkers();
 }
 
+Bool xf86PrivsElevated(void)
+{
+  static Bool privsTested = FALSE;
+  static Bool privsElevated = FALSE;
+
+  if (!privsTested) {
+#if !defined(WIN32)
+if ((getuid() != geteuid()) || (getgid() != getegid())) {
+  privsElevated = TRUE;
+} else {
+#if defined(HAVE_ISSETUGID)
+  privsElevated = issetugid();
+#elif defined(HAVE_GETRESUID)
+  uid_t ruid, euid, suid;
+  gid_t rgid, egid, sgid;
+
+  if ((getresuid(ruid, euid, suid) == 0) 
+  (getresgid(rgid, egid, sgid) == 0))
+privsElevated = (euid != suid) || (egid != sgid);
+#else
+  /*
+   * If there are saved ID's the process might still be privileged
+   * even though

Re: [PATCH xserver] check for elevated privileges not uid=0 (V2)

[Antoine Martin]

[snip]

Subject: [PATCH xserver] check for elevated privileges not uid=0 (V3)

Couldn't find any reference to it, so I just removed it.


You must have messed up from V2 to V3 because the variables ended
outside the function again.

Oops, (re)fixed.


Note that in V2 there was missing a blank
line after the variable declarations.

all instances fixed

[snip]

+   * If there are saved ID's the process might still be priviledged
privileged

thx!


+   * even though the above test succeeded.  If issetugid() and
I also consider double space after period a typographical mistake, but
I am not gonna run a crusade here :)

Indeed it was.


+if (seteuid(0) != 0) {
+  privsElevated = FALSE;
You use != 0 to check for failure here...

+} else {
+  if (seteuid(oldeuid) == -1) {

... but == -1 here. Is there any reason for not being consistent?

That's from the libX11 code I duplicated...
(fixed)


+/* XXX ouch, coudn't get back to original uid
+   what can we do ??? */

If we get to this point, the code in the patch has failed. It should
apologize deeply and offer your phone number for technical support :)
Seriously, it should print an internal error message of some kind,
maybe including __function__. Just quitting silently and leave a
cryptic 127 that might or not propagate to the user does not cut it.

Added a more helpful message. Notes:
* most platforms I can get hold of now have getresuid or issetugid, 
so this is unlikely to fire in the real world
* when it does fire, the chances that setuid(0) does not fail but 
setuid(!=0) does is slim.




+_exit(127);

Changed to plain exit(127)


Why do you use _exit() and not exit() here? This is not e.g. a forked
child that should escape normal clean-up?

Are there are risks in calling the exit hooks as root? I can't see any.

See new patch in separate email.

Thanks!
Antoine



Best regards,
Tormod


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] check for elevated privileges not uid=0

[Antoine Martin]

And another blooper, sorry for the spam. Correct patch attached.
Need sleep, will pick this up tomorrow.

Antoine



On 10/11/2011 02:00 AM, Antoine Martin wrote:

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html

Signed-off-by: Antoine Martin anto...@nagafix.co.uk


Changes from the previous version of this patch:
* static var location (again!), formatting, typos, error message, exit()
(thx to Tormod Volden)
* use more generic AC_CHECK_FUNCS macro (thx to Alan Coopersmith)

Thanks
Antoine


---
 configure.ac   |4 ++-
 hw/xfree86/common/xf86Config.c |   28 
 hw/xfree86/common/xf86Init.c   |   68 ++-
 hw/xfree86/common/xf86Priv.h   |1 +
 include/xorg-config.h.in   |6 +++
 5 files changed, 83 insertions(+), 24 deletions(-)

diff --git a/configure.ac b/configure.ac
index 67a6836..a79e2be 100644
--- a/configure.ac
+++ b/configure.ac
@@ -208,9 +208,11 @@ AC_SUBST(DLOPEN_LIBS)
 
 dnl Checks for library functions.
 AC_FUNC_VPRINTF
-AC_CHECK_FUNCS([geteuid getuid link memmove memset mkstemp strchr strrchr \
+AC_CHECK_FUNCS([geteuid getuid issetugid getresuid \
+   link memmove memset mkstemp strchr strrchr \
strtol getopt getopt_long vsnprintf walkcontext backtrace \
getisax getzoneid shmctl64 strcasestr ffs vasprintf])
+
 AC_FUNC_ALLOCA
 dnl Old HAS_* names used in os/*.c.
 AC_CHECK_FUNC([getdtablesize],
diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index f8c1b65..8ccc52a 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -72,8 +72,8 @@
  * These paths define the way the config file search is done.  The escape
  * sequences are documented in parser/scan.c.
  */
-#ifndef ROOT_CONFIGPATH
-#define ROOT_CONFIGPATH%A, %R, \
+#ifndef ALL_CONFIGPATH
+#define ALL_CONFIGPATH %A, %R, \
/etc/X11/%R, %P/etc/X11/%R, \
%E, %F, \
/etc/X11/%F, %P/etc/X11/%F, \
@@ -83,8 +83,8 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef USER_CONFIGPATH
-#define USER_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
+#ifndef RESTRICTED_CONFIGPATH
+#define RESTRICTED_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
/etc/X11/%G, %P/etc/X11/%G, \
/etc/X11/%X, /etc/%X, \
%P/etc/X11/%X.%H, \
@@ -92,14 +92,14 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef ROOT_CONFIGDIRPATH
-#define ROOT_CONFIGDIRPATH %A, %R, \
+#ifndef ALL_CONFIGDIRPATH
+#define ALL_CONFIGDIRPATH  %A, %R, \
/etc/X11/%R, %C/X11/%R, \
/etc/X11/%X, %C/X11/%X
 #endif
-#ifndef USER_CONFIGDIRPATH
-#define USER_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
-   /etc/X11/%X, %C/X11/%X
+#ifndef RESTRICTED_CONFIGDIRPATH
+#define RESTRICTED_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R, \
+   /etc/X11/%X, %C/X11/%X
 #endif
 #ifndef SYS_CONFIGDIRPATH
 #define SYS_CONFIGDIRPATH  /usr/share/X11/%X, %D/X11/%X
@@ -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig)
 Bool implicit_layout = FALSE;
 
 if (!autoconfig) {
-   if (getuid() == 0) {
-   filesearch = ROOT_CONFIGPATH;
-   dirsearch = ROOT_CONFIGDIRPATH;
+   if (!xf86PrivsElevated()) {
+   filesearch = ALL_CONFIGPATH;
+   dirsearch = ALL_CONFIGDIRPATH;
} else {
-   filesearch = USER_CONFIGPATH;
-   dirsearch = USER_CONFIGDIRPATH;
+   filesearch = RESTRICTED_CONFIGPATH;
+   dirsearch = RESTRICTED_CONFIGDIRPATH;
}
 
if (xf86ConfigFile)
diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index 350918d..063a2d8 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -237,6 +237,55 @@ xf86PrintMarkers(void)
   LogPrintMarkers();
 }
 
+Bool xf86PrivsElevated(void)
+{
+  static Bool privsTested = FALSE;
+  static Bool privsElevated = FALSE;
+
+  if (!privsTested) {
+#if !defined(WIN32)
+if ((getuid() != geteuid()) || (getgid() != getegid())) {
+  privsElevated = TRUE;
+} else {
+#if defined(HAVE_ISSETUGID)
+  privsElevated = issetugid();
+#elif defined(HAVE_GETRESUID)
+  uid_t ruid, euid, suid;
+  gid_t rgid, egid, sgid;
+
+  if ((getresuid(ruid, euid, suid) == 0) 
+  (getresgid(rgid, egid, sgid) == 0

Re: [PATCH xserver] check for elevated privileges rather than just euid=0

[Antoine Martin]

On 07/10/11 02:00, Jeremy Huddleston wrote:
 I don't know if xf86PrivsElevated is the right name for this API.  Users 
 might have access even without *elevated* privs.  What we really want to know 
 is if the user is privileged, and I can see us eventually updating the 
 implementation of this call to reflect changes in access controls.  Perhaps 
 something like xf86IsPrivileged()
I don't think xf86IsPrivileged() is the right name / check.
Quick recap for all: we want to ensure that the command line switches
protected by this check cannot be used to:
1) load modules as-root from user controlled locations with -modulepath
2) write to a user specified file as-root with -logfile
3) load config files from absolute paths with -config
4) I had also changed the code around:
fcntl(fileno(stderr), F_SETFL, status | O_NONBLOCK);
And now I don't think this was the right thing to do, anyone?

We want to allow these options:
1) if the user *is not* privileged (ie: non-suid Xorg binary)
2) if the user *is* privileged (ie: running Xorg as root directly)
Just not when the user is running with elevated privileges (ie: suid Xorg)
The name xf86IsPrivileged() would not make this clear to the caller.
(makes it sound more like a glorified is-root check to me)

Anyway, I will already have to re-submit the patch as I need to
incorporate Tormod's comments on the location of the static fields.
So if we can just agree on a meaningful name...

Thanks
Antoine


 --Jeremy


 On Oct 6, 2011, at 6:05 AM, Antoine Martin wrote:

 This allows us to run the server as a normal user whilst still
 being able to use the -modulepath, -logfile and -config switches
 We define a xf86PrivsElevated which will do the checks and cache
 the result in case it is called more than once.
 Also renamed the paths #defines to match their new meaning.
 Original discussion which led to this patch can be found here:
 http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html

 Signed-off-by: antoine anto...@nagafix.co.uk
 0001-xserver-check-for-elevated-privileges-rather-than-ju.patch___
 xorg-devel@lists.x.org: X.Org development
 Archives: http://lists.x.org/archives/xorg-devel
 Info: http://lists.x.org/mailman/listinfo/xorg-devel


 ---
 Jeremy Huddleston

 Rebuild Sudan
  - Board of Directors
  - http://www.rebuildsudan.org

 Berkeley Foundation for Opportunities in Information Technology
  - Advisory Board
  - http://www.bfoit.org


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] check for elevated privileges rather than just euid=0

[Antoine Martin]

On 07/10/11 04:25, Michal Suchanek wrote:
 On 6 October 2011 17:30, Antoine Martin anto...@nagafix.co.uk wrote:
 On 06/10/11 20:39, Michal Suchanek wrote:

 Hello,

 I would like to check this out but how do I tell this actually works?

 I use this patch for using Xorg as an Xdummy server, like so:
 /usr/local/bin/Xorg +extension GLX +extension RandR +extension Render
 -logfile $HOME/log -config $HOME/xorg.conf'
 My Xdummy xorg.conf can be found here:
 http://xpra.org/src/Xdummy/xorg.conf

 I tried to build a Debian X server package with this patch.

 I can run Xorg directly with these arguments but not through the X
 suid wrapper Debian uses.
That's the idea.. It is meant to continue to prevent non-root users from
using the suid wrapper to load arbitrary modules, config files or write
to user-specified log files.
 Still I cannot run X server with these arguments when I use su to log
 in as root.
Well, then this is an unintended problem.
I suspect this is a consequence of using the euid/guid/ruid checks that
Alan suggested here:
http://www.mail-archive.com/xorg-devel@lists.x.org/msg25259.html
Maybe those checks are a little too stringent for sudo/su vs suid wrappers?
 Since Debian and Ubuntu ship with root login disabled it disables
 these arguments for root entirely which does not sound desirable.
Definitely - I'm looking into it now, thanks for pointing that out!

Antoine

 Thanks
 Michal
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] check for elevated privileges rather than just euid=0

[Antoine Martin]

On 07/10/11 13:20, Antoine Martin wrote:
 On 07/10/11 04:25, Michal Suchanek wrote:
 On 6 October 2011 17:30, Antoine Martin anto...@nagafix.co.uk wrote:
 On 06/10/11 20:39, Michal Suchanek wrote:

 Hello,

 I would like to check this out but how do I tell this actually works?

 I use this patch for using Xorg as an Xdummy server, like so:
 /usr/local/bin/Xorg +extension GLX +extension RandR +extension Render
 -logfile $HOME/log -config $HOME/xorg.conf'
 My Xdummy xorg.conf can be found here:
 http://xpra.org/src/Xdummy/xorg.conf

 I tried to build a Debian X server package with this patch.

 I can run Xorg directly with these arguments but not through the X
 suid wrapper Debian uses.
 That's the idea.. It is meant to continue to prevent non-root users from
 using the suid wrapper to load arbitrary modules, config files or write
 to user-specified log files.
 Still I cannot run X server with these arguments when I use su to log
 in as root.
 Well, then this is an unintended problem.
 I suspect this is a consequence of using the euid/guid/ruid checks that
 Alan suggested here:
 http://www.mail-archive.com/xorg-devel@lists.x.org/msg25259.html
 Maybe those checks are a little too stringent for sudo/su vs suid wrappers?
Are you sure you can't run the X server after suing to root?
This is what I get on an Ubuntu Lucid box when calling via the X wrapper:
$ su -
Password:
# X -v
ruid=0, euid=0, suid=0
rgid=0, egid=0, sgid=0

Looks ok to me, ruid==euid==suid so xf86PrivsElevated() returns FALSE.
The behaviour should be unchanged from before when using sudo or su.
What's the error message you are getting in this case?
The full command line and error would be nice, as well as distro and
versions.
 Since Debian and Ubuntu ship with root login disabled it disables
 these arguments for root entirely which does not sound desirable.
 Definitely - I'm looking into it now, thanks for pointing that out!
If anyone is interested, I am attaching the wrapper that debian uses,
extracted from:
xorg-7.5+8/debian/local/xserver-wrapper.c
It already does some funny stuff with -config in there.

Running this wrapper as a normal user I get:
# X -version
ruid=1000, euid=0, suid=0
rgid=1000, egid=0, sgid=0

Which correctly makes xf86PrivsElevated() return TRUE.
I don't see what I could have broken with the patch!?

Antoine
 Antoine
 Thanks
 Michal

/* xserver-wrapper.c - a simple wrapper for X servers that decides whether to
 * let them be run.
 *
 * By Stephen Early
 *
 * Stephen Early: modified to use /etc/X11/X symlink with security level of
 *'Console' if /etc/X11/Xserver does not exist
 * Mark W. Eichin: permit non-privileged -showconfig (6 May 1997)
 * Mark W. Eichin: fix sense of error check for -showconfig (11 May 1997)
 * Mark W. Eichin: drop privileges on alternate -config, even if we do pass the
 * security check, to prevent using the error handling to read
 * the first line of any protected file (19 Sep 1997)
 * Erik Troan: prevent buffer overruns (25 Mar 1998)
 * Topi Miettinen: plug file descriptor leak (26 Apr 1998)
 * Branden Robinson: only fclose() if file was opened (3 May 1999)
 * Colin Phipps: minor device number check should be  64, not  128, or we
 *   catch serial terminals (26 Feb 2000)
 * Branden Robinson: ensure sanity of X server socket directory (13 Jun 2000)
 * Branden Robinson: make all paths #defines
 *   more helpful socket dir error messages (29 Jun 2000)
 * Branden Robinson: bail out if the config file contains only the silly
 *   default X server name (XF86_NONE) (30 Jul 2000)
 * Branden Robinson: increase verbosity when wrapper config not found
 *   (2 Oct 2000)
 * Branden Robinson:
 *   - new configuration file, Xwrapper.config, with different format
 * (name=value)
 *   - now just exec's /etc/X11/X; whatever this symlink points
 * to will be used as the X server
 *   - config file specifies allowed user types as before (root only,
 * console users, anyone)
 *   - config file specifies nice value to use for server
 *   (17 Nov 2000)
 * Branden Robinson: now accepts hyphens in variable contents (24 Nov 2000)
 * Branden Robinson: fix dumb errors left over from debugging (3 Dec 2000)
 * Branden Robinson: let root start the server even if he isn't on a
 *console, and the security level is console (11 Dec 2000)
 * Branden Robinson: check out the X server symlink with readlink; abort if
 *   it's not a symlink, or if it points back to this wrapper
 *   (24 Feb 2001)
 * Branden Robinson: whoops; readlink() doesn't null-terminate the target
 *   string (27 Feb 20001)
 * Branden Robinson: add more info to suspicious error messages (16 Mar 2001)
 * Branden Robinson: also allow unprivileged use of -version option
 *   (13 Jul 2001)
 * Branden Robinson: check mode of DRI device directory, if it exists, and warn

Re: [PATCH xserver] check for elevated privileges rather than just euid=0

[Antoine Martin]

[snip]
 Still I cannot run X server with these arguments when I use su to log
 in as root.
 Well, then this is an unintended problem.
 I suspect this is a consequence of using the euid/guid/ruid checks that
 Alan suggested here:
 http://www.mail-archive.com/xorg-devel@lists.x.org/msg25259.html
 Maybe those checks are a little too stringent for sudo/su vs suid wrappers?
 Are you sure you can't run the X server after suing to root?
 This is what I get on an Ubuntu Lucid box when calling via the X wrapper:
 $ su -
 Password:
 # X -v
 ruid=0, euid=0, suid=0
 rgid=0, egid=0, sgid=0
 I don't know where you get this. My X wrapper does not provide this option:
That's because I cheated, sorry for misleading you.
I didn't have time to install all the debian dev environment on that
test box so I just replaced /usr/bin/Xorg with the program attached to
see what happens.
It just prints out the uids it finds, which is what xf86PrivsElevated()
uses. I don't see why it would behave any different from the patch..
 Looks ok to me, ruid==euid==suid so xf86PrivsElevated() returns FALSE.
 The behaviour should be unchanged from before when using sudo or su.
 What's the error message you are getting in this case?
 The full command line and error would be nice, as well as distro and
 versions.
  $ su -
 Password:
 OptiPlex960:~# Xorg +extension GLX +extension RANDR +extension RENDER
 -logfile /scratch/xdummy.log :1

 Fatal server error:
 The '-logfile' option cannot be used with elevated privileges.


 Please consult the The X.Org Foundation support
  at http://wiki.x.org
  for help.

 Obviously, I am running with eleveated privileges which is technically true.
The application only sees that you are logged in as root at this point
and it should have no knowledge of how you got there (su).
So although you have elevated privileges in some sense, the check
should return FALSE here.

That's odd because as I posted earlier, running the attached test
program after suing gives me the correct result for ruid/euid/suid -
which is 0.
Maybe compiling on Debian will give a different result, is HASSETUGID
set in your build?
 Note that I normally use 'su' without any options which gives the same error.
  dpkg -S `which su`
 login: /bin/su

 ii  login 1:4.1.4.2+svn3283-2+squeeze1  system
 login tools
 ii  libc6 2.13-21
 Embedded GNU C Library: Shared libraries

 I hope this provides the required info.
It does, thank you very much for that. (I assume that xorg-server is up
to date too since you rebuilt it from source)
I will take a look at this using a proper debian build environment ASAP.

Antoine

 Thanks

 Michal
#include stdlib.h
#include stdio.h
#include errno.h

//#define HASSETUGID 0
#define HASGETRESUID 1

main()
{
  printf (uid=%d, euid=%d, gid=%d, egid=%d\n, getuid(), geteuid(), getgid(), 
getegid());
#if defined(HASSETUGID)
  printf (issetugid()=%d % issetugid());
#endif

#if defined(HASGETRESUID)
  uid_t ruid, euid, suid;
  gid_t rgid, egid, sgid;
  if ((getresuid(ruid, euid, suid) == 0) 
  (getresgid(rgid, egid, sgid) == 0)) {
printf (ruid=%d, euid=%d, suid=%d\n, ruid, euid, suid);
printf (rgid=%d, egid=%d, sgid=%d\n, rgid, egid, sgid);
  }
#endif

  unsigned int oldeuid;
  oldeuid = geteuid();
  if (seteuid(0) != 0) {
printf (cannot setuid(0)\n);
  } else {
printf (can setuid(0)\n);
  }
}
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] check for elevated privileges rather than just euid=0

[Antoine Martin]

This allows us to run the server as a normal user whilst still
being able to use the -modulepath, -logfile and -config switches
We define a xf86PrivsElevated which will do the checks and cache
the result in case it is called more than once.
Also renamed the paths #defines to match their new meaning.
Original discussion which led to this patch can be found here:
http://lists.freedesktop.org/archives/xorg-devel/2011-September/025853.html

Signed-off-by: antoine anto...@nagafix.co.uk
---
 hw/xfree86/common/xf86Config.c |   28 +-
 hw/xfree86/common/xf86Init.c   |   63 ++--
 hw/xfree86/common/xf86Priv.h   |1 +
 3 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
index f8c1b65..8ccc52a 100644
--- a/hw/xfree86/common/xf86Config.c
+++ b/hw/xfree86/common/xf86Config.c
@@ -72,8 +72,8 @@
  * These paths define the way the config file search is done.  The escape
  * sequences are documented in parser/scan.c.
  */
-#ifndef ROOT_CONFIGPATH
-#define ROOT_CONFIGPATH%A, %R, \
+#ifndef ALL_CONFIGPATH
+#define ALL_CONFIGPATH %A, %R, \
/etc/X11/%R, %P/etc/X11/%R, \
%E, %F, \
/etc/X11/%F, %P/etc/X11/%F, \
@@ -83,8 +83,8 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef USER_CONFIGPATH
-#define USER_CONFIGPATH/etc/X11/%S, %P/etc/X11/%S, \
+#ifndef RESTRICTED_CONFIGPATH
+#define RESTRICTED_CONFIGPATH  /etc/X11/%S, %P/etc/X11/%S, \
/etc/X11/%G, %P/etc/X11/%G, \
/etc/X11/%X, /etc/%X, \
%P/etc/X11/%X.%H, \
@@ -92,14 +92,14 @@
%P/lib/X11/%X.%H, \
%P/lib/X11/%X
 #endif
-#ifndef ROOT_CONFIGDIRPATH
-#define ROOT_CONFIGDIRPATH %A, %R, \
+#ifndef ALL_CONFIGDIRPATH
+#define ALL_CONFIGDIRPATH  %A, %R, \
/etc/X11/%R, %C/X11/%R, \
/etc/X11/%X, %C/X11/%X
 #endif
-#ifndef USER_CONFIGDIRPATH
-#define USER_CONFIGDIRPATH /etc/X11/%R, %C/X11/%R, \
-   /etc/X11/%X, %C/X11/%X
+#ifndef RESTRICTED_CONFIGDIRPATH
+#define RESTRICTED_CONFIGDIRPATH   /etc/X11/%R, %C/X11/%R, \
+   /etc/X11/%X, %C/X11/%X
 #endif
 #ifndef SYS_CONFIGDIRPATH
 #define SYS_CONFIGDIRPATH  /usr/share/X11/%X, %D/X11/%X
@@ -2302,12 +2302,12 @@ xf86HandleConfigFile(Bool autoconfig)
 Bool implicit_layout = FALSE;
 
 if (!autoconfig) {
-   if (getuid() == 0) {
-   filesearch = ROOT_CONFIGPATH;
-   dirsearch = ROOT_CONFIGDIRPATH;
+   if (!xf86PrivsElevated()) {
+   filesearch = ALL_CONFIGPATH;
+   dirsearch = ALL_CONFIGDIRPATH;
} else {
-   filesearch = USER_CONFIGPATH;
-   dirsearch = USER_CONFIGDIRPATH;
+   filesearch = RESTRICTED_CONFIGPATH;
+   dirsearch = RESTRICTED_CONFIGDIRPATH;
}
 
if (xf86ConfigFile)
diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index 350918d..29f7638 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -237,6 +237,50 @@ xf86PrintMarkers(void)
   LogPrintMarkers();
 }
 
+static Bool privsTested = FALSE;
+static Bool privsElevated = FALSE;
+Bool xf86PrivsElevated(void)
+{
+  if (!privsTested) {
+#if !defined(WIN32)
+if ((getuid() != geteuid()) || (getgid() != getegid())) {
+  privsElevated = TRUE;
+} else {
+#if defined(HASSETUGID)
+  privsElevated = issetugid();
+#elif defined(HASGETRESUID)
+  uid_t ruid, euid, suid;
+  gid_t rgid, egid, sgid;
+  if ((getresuid(ruid, euid, suid) == 0) 
+  (getresgid(rgid, egid, sgid) == 0))
+privsElevated = (euid != suid) || (egid != sgid);
+#else
+  /*
+   * If there are saved ID's the process might still be priviledged
+   * even though the above test succeeded.  If issetugid() and
+   * getresgid() aren't available, test this by trying to set
+   * euid to 0.
+   */
+  unsigned int oldeuid;
+  oldeuid = geteuid();
+  if (seteuid(0) != 0) {
+privsElevated = FALSE;
+  } else {
+if (seteuid(oldeuid) == -1) {
+ /* XXX ouch, coudn't get back to original uid
+  what can we do ??? */
+  _exit(127);
+}
+   privsElevated = 1;
+  }
+#endif
+}
+#endif
+privsTested = TRUE;
+  }
+  return privsElevated;
+}
+
 static Bool
 xf86CreateRootWindow(WindowPtr pWin)
 {
@@ -896,7 +940,7 @@ OsVendorInit(void)
 
 #ifdef O_NONBLOCK
   if (!beenHere) {
-if (geteuid() == 0  getuid() != geteuid())
+if (xf86PrivsElevated())
 {
   int status;
 
@@ -1067,10 +,11 @@ ddxProcessArgument(int argc, char **argv, int i)
   FatalError(Required argument to %s not specified\n, argv[i]);  \
 }
 
-  /* First 

Re: [PATCH xserver] check for elevated privileges rather than just euid=0

[Antoine Martin]

On 06/10/11 20:39, Michal Suchanek wrote:
 Hello,

 I would like to check this out but how do I tell this actually works?
I use this patch for using Xorg as an Xdummy server, like so:
/usr/local/bin/Xorg +extension GLX +extension RandR +extension Render
-logfile $HOME/log -config $HOME/xorg.conf'
My Xdummy xorg.conf can be found here:
http://xpra.org/src/Xdummy/xorg.conf

This will fail without the patch since you cannot use your own config or
log file location.
You can then chmod +s Xorg and it will then refuse to honour those flags.
 Xephyr and Xvfb already run as non-root, Xorg needs device access to
 run which will likely fail as non-root.
The particular use case which led me to write this patch does not
require any hardware access (dummy driver), for more information on the
changes and the reason why checking for root is always the wrong check
please see this discussion:
http://www.mail-archive.com/xorg-devel@lists.x.org/msg25230.html

AFAIK there are a number of things that still need to be addressed to
allow X to run as root against real hardware. This patch does not try to
address any of it.

Cheers
Antoine
 Thanks
 Michal

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC] Xdummy standalone binary or -dummy switch

[Antoine Martin]

[snip]
 Others will have an easier time
 reviewing and merging your patch if you follow these guidelines:

 http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches
Will do, I just thought this would be ok for the RFC stage.
 The only other patch required to make this useful for non-root users is
 the one that allows for absolute config files I had posted earlier,
 otherwise I don't see how one can launch using custom config files as
 the -config option does not seem to honour the -configdir option.
 Obviously this would now need an extra euid!=uid check.
 Or am I missing something again?
 Looks to me like all you need for this bit is to fix the getuid check
 in xf86HandleConfigFile. The USER_CONFIGPATH lists both absolute and
 relative paths as the first things it checks, and this also takes care
 of -configdir. (But -configdir doesn't do what you think it does.)
Hah, so I guess you are suggesting that USER_CONFIGPATH is used when not
running with elevated privileges?
I'll have to check which paths are used carefully as this could cause a
change in behaviour...
 If that's all the issues you've had, I'm hoping that just fixing the
 stupid am I root? checks will take care of everything you need.
 Anything missing?
 I don't think so - works-for-me(tm)
 Hooray!

 I guess distributors will then have to ship two copies of /usr/bin/Xorg,
 one that is suid-root and one that isn't? (until we can get rid of the
 suid one entirely, one day)
 Amusingly, any user can get a non-suid copy of a suid binary. cp
 will do.
On Fedora, /usr/bin/Xorg is not world readable, so you can't do that.
I've never understood the reasoning behind this decision since anyone
can easily get hold of the exact same binary file from a CD or rpm mirror.

Antoine
  But I think adding a -dropprivs flag wouldn't be a terrible
 plan. It should wait until we find out if distros actually have this
 problem, though, especially since apparently it's hard to fully drop
 privileges correctly and portably.

 Jamey

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC] Xdummy standalone binary or -dummy switch

[Antoine Martin]

On 01/10/11 04:12, Alan Coopersmith wrote:
  if ( (geteuid() != getuid()) ) {

 It's not that simple to check for elevated privs in a world with
 saved setids and other forms of privilege manipulation - issetugid() is
 useful if it's available - for instance see:
 http://cgit.freedesktop.org/xorg/lib/libX11/tree/src/xlibi18n/lcFile.c#n234


There are only 2 or 3 places that will require this check, but seeing
the level of complexity of the code you posted above it probably makes
sense to define a static helper function somewhere in /common/ ?
It doesn't look like there is a need for non-posix/os-specific code, but
I am not familiar enough with all the OSes supported by Xorg to be certain.
If that approach sounds ok, let me know and I'll post a proper patch
(git and everything)

Antoine
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC] Xdummy standalone binary or -dummy switch

[Antoine Martin]

[snip]
I meant to reply to Jamey, but couldn't find his reply in my mailbox..
 What are those limitations?
 1) root-only limitations on the configuration options for the location
 of the config and log files.
 This makes sense when the server runs as root and gets (almost?) full
 access to all system memory, but not when the only device driver loaded
 is the dummy one.
 Any user should be able to launch an Xdummy instance without needing
 root or special permissions.
 (I've tested this with a quick and dirty patch [4])
 The same goes for the video-nested driver. It might make sense for fbdev
 or KMS drivers too: if you have permission for the device node, you
 should be able to run an X server on it any way you like.

 Further, I think checking whether the server is running as root is
 always the wrong check. I think the *right* check is to test whether the
 server's effective UID is the same as its real UID. Then the server can
 be paranoid about config files and options only when it's running with
 different permissions than its caller otherwise would have.

 For example, there's nothing wrong with a non-root user setting a custom
 module path. The only problem is if a non-root user, running a SUID X
 server, can use a custom module path to execute harmful code with
 elevated permissions. Similarly, setting a log-file path is only a
 problem if you can overwrite a file you couldn't otherwise overwrite.

 I don't think this calls for an unbreak me please flag; I think the
 normal behavior of the server should be fixed.
OK, I've looked for at all the places that use getuid or getuid, and found:
* os-support/* some checks for euid!=0 for console stuff / KeepTTY,
which I have left alone
* parser/write.c also left alone for now - I can look into it too if you
want.
* common/xf86Init.c: ddxProcessArgument and ddxUseMsg patched, see
attachment.

With just this small patch and the correct xorg.conf as per below I can
run dummy non-root without problems. Does it look acceptable?

The only other patch required to make this useful for non-root users is
the one that allows for absolute config files I had posted earlier,
otherwise I don't see how one can launch using custom config files as
the -config option does not seem to honour the -configdir option.
Obviously this would now need an extra euid!=uid check.
Or am I missing something again?

--- xfree86/parser/scan.c2011-08-05 12:59:02.0 +0700
+++ xfree86.euid/parser/scan.c2011-09-30 12:59:44.935338707 +0700
@@ -739,6 +739,17 @@
 int cmdlineUsed = 0;
 FILE *file = NULL;
 
+if (cmdline  cmdline[0]=='/') {
+filepath = strdup(cmdline);
+file = fopen(filepath, r);
+if (file) {
+configFiles[numFiles].file = file;
+configFiles[numFiles].path = filepath;
+numFiles++;
+return strdup(cmdline);
+}
+}
+
 pathcopy = strdup(path);
 for (template = strtok(pathcopy, ,); template  !file;
  template = strtok(NULL, ,)) {
 2) Xorg server probing devices.
 Now this one I am less sure about, maybe I just don't know the proper
 incantations. I've tried all the options I could find to try to prevent
 all the probing that goes on (explicitly specifying the void driver
 for keyboard and mouse, etc)
 In the end, I just hacked the code to ignore them all for now [5]
 I use these settings in my nested/dummy testing, which seem to suffice
 for me:

 Section ServerFlags
 Option AutoEnableDevices false
 Option AutoAddDevices false
 EndSection

 I'm not sure even AutoEnableDevices is relevant; I think AutoAddDevices
 is the important flag. You shouldn't need to add any input devices, not
 even void ones.
That did the trick!
 If that's all the issues you've had, I'm hoping that just fixing the
 stupid am I root? checks will take care of everything you need.
 Anything missing?
I don't think so - works-for-me(tm)
I guess distributors will then have to ship two copies of /usr/bin/Xorg,
one that is suid-root and one that isn't? (until we can get rid of the
suid one entirely, one day)

Thanks
Antoine
 Jamey
--- xfree86/common/xf86Init.c   2011-08-05 13:00:12.0 +0700
+++ xfree86.euid/common/xf86Init.c  2011-09-30 12:11:37.028783141 +0700
@@ -1052,8 +1052,8 @@
 
   /* First the options that are only allowed for root */
   if (!strcmp(argv[i], -modulepath) || !strcmp(argv[i], -logfile)) {
-if ( (geteuid() == 0)  (getuid() != 0) ) {
-  FatalError(The '%s' option can only be used by root.\n, argv[i]);
+if ( (geteuid() != getuid()) ) {
+  FatalError(The '%s' option cannot be used when running with elevated 
privileges.\n, argv[i]);
 }
 else if (!strcmp(argv[i], -modulepath))
 {
@@ -1081,9 +1081,9 @@
   if (!strcmp(argv[i], -config) || !strcmp(argv[i], -xf86config))
   {
 CHECK_FOR_REQUIRED_ARGUMENT();
-if (getuid() != 0  !xf86PathIsSafe(argv[i + 1])) {
+if (getuid() != geteuid()  !xf86PathIsSafe(argv[i + 1])) {
   

[RFC] Xdummy standalone binary or -dummy switch

[Antoine Martin]

Hi,

As discussed a while back [1], there are a number of issues which can
only be resolved by using an Xdummy binary server in order to replace
Xvfb with a more modern equivalent. The only alternative I can think of
would be to add a -dummy command line option to Xorg, this feels
somewhat more intrusive/dangerous. Or maybe there is another way I
haven't thought about?

First, what is wrong with Xvfb?
It does not support modern extensions like RandrR which I need for my
Xpra fork [2]

Why can't people just use Xorg as it is with the dummy driver?
Well, you can but this involves using Xdummy's [3] dodgy LD_PRELOAD
hacks to workaround some of the limitations built into the Xorg server.

What are those limitations?
1) root-only limitations on the configuration options for the location
of the config and log files.
This makes sense when the server runs as root and gets (almost?) full
access to all system memory, but not when the only device driver loaded
is the dummy one.
Any user should be able to launch an Xdummy instance without needing
root or special permissions.
(I've tested this with a quick and dirty patch [4])
2) Xorg server probing devices.
Now this one I am less sure about, maybe I just don't know the proper
incantations. I've tried all the options I could find to try to prevent
all the probing that goes on (explicitly specifying the void driver
for keyboard and mouse, etc)
In the end, I just hacked the code to ignore them all for now [5]


It feels cleaner to just make a new Xdummy binary rather than
overloading Xorg with a new -dummy switch.
pros:
* drop almost all command line switches, make -novtswitch the default, etc
* easier to whitelist/blacklist what gets loaded
* no need to link against libraries which aren't needed, no VGAHW,
INT10, etc
* easier to use: can be made to look more like Xvfb
cons:
* higher initial development cost and maintenance

The alternative is the Xorg -dummy switch:
pros:
* much easier to implement? (new option and a few switches here and there?)
cons:
* potentially dangerous if somehow hardware device drivers can still be
loaded when the switch is on (which would make this type of bug
potentially quite dangerous)


Should you want to try it out with Xpra, simply apply the 3 patches
below to /xfree86/ (the third one [6] allows for absolute config file
paths - not sure why this does not work for root at present?), make and
copy the resulting Xorg as Xdummy, then you can test with:
xpra start :100 --xvfb=/usr/local/bin/Xdummy +extension RandR
+extension Render -logfile ./log -config /path/to/xorg.conf
DISPLAY=:100 xterm
xpra attach :100
Or if attaching from somewhere else:
xpra attach ssh:hostname:100

Your comments would be much appreciated.

Thanks
Antoine


[1] xorg discussion and patch for dummy driver:
http://lists.freedesktop.org/archives/xorg/2011-April/052812.html
[2] Xpra fork:
http://xpra.org/
[3] Xdummy by Karl Runge:
http://www.karlrunge.com/x11vnc/Xdummy
[4] Allow non-root users to specify log and config locations:
http://xpra.org/src/Xdummy/xdummy-allow-nonrootoptions.patch
[5] Ignore all input devices:
http://xpra.org/src/Xdummy/xdummy-ignoreallinputdevices.patch
[6] Allow absolute config filenames:
http://xpra.org/src/Xdummy/xdummy-absoluteconfigs.patch
[7] Sample xorg.conf I have used for testing dummy:
http://xpra.org/src/Xdummy/xorg.conf

PS: these patches were generated in reverse so you need to apply them
with -R, sorry about that!

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] xf86-video-dummy allow up to 32767x32767 (re-send)

[Antoine Martin]

Hi Adam,

Can you apply this patch please?
I have tested this up to 16384x8192, higher resolutions make xtiming
spew out garbage (integer overflow?) so I couldn't generate modelines.

Thanks
Antoine

From 24ba77d173273159d6295f8bcd8d15a26eab Mon Sep 17 00:00:00 2001
From: Antoine Martin anto...@devloop.org.uk
Date: Wed, 13 Apr 2011 19:30:26 +0700
Subject: [PATCH] Constifies and increases the maximum size allowed for dummy
 screens to 32767x32767

Signed-off-by: Antoine Martin anto...@devloop.org.uk
Tested-by: Antoine Martin anto...@devloop.org.uk
---
 src/dummy_driver.c |7 +--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index 804e41e..305bd3c 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -85,6 +85,9 @@ static Bool   dummyDriverFunc(ScrnInfoPtr pScrn, 
xorgDriverFuncOp op,
 #define DUMMY_MINOR_VERSION PACKAGE_VERSION_MINOR
 #define DUMMY_PATCHLEVEL PACKAGE_VERSION_PATCHLEVEL
 
+#define DUMMY_MAX_WIDTH 32767
+#define DUMMY_MAX_HEIGHT 32767
+
 /*
  * This is intentionally screen-independent.  It indicates the binding
  * choice made in the first PreInit.
@@ -402,8 +405,8 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
int apertureSize = (pScrn-videoRam * 1024);
i = xf86ValidateModes(pScrn, pScrn-monitor-Modes,
  pScrn-display-modes, clockRanges,
- NULL, 256, 2048,(8 * pScrn-bitsPerPixel),
- 128, 2048, pScrn-display-virtualX,
+ NULL, 256, DUMMY_MAX_WIDTH,(8 * 
pScrn-bitsPerPixel),
+ 128, DUMMY_MAX_HEIGHT, pScrn-display-virtualX,
  pScrn-display-virtualY, apertureSize,
  LOOKUP_BEST_REFRESH);
 
-- 
1.7.4.2


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Fwd: [PATCH] xf86-video-dummy allow up to 32767x32767

[Antoine Martin]

Re-sending to devel list.

 Original Message 
Subject: [PATCH] xf86-video-dummy allow up to 32767x32767
Date: Wed, 13 Apr 2011 19:41:23 +0700
From: Antoine Martin anto...@nagafix.co.uk
To: Adam Jackson a...@nwnk.net,  x...@lists.freedesktop.org 
x...@lists.freedesktop.org


Hi Adam,

Can you please apply this patch please?
Re-sending as a separate thread with patch in git format-patch.
I have tested this up to 16384x8192, higher resolutions make xtiming
spew out garbage (integer overflow?).

Thanks
Antoine

From 24ba77d173273159d6295f8bcd8d15a26eab Mon Sep 17 00:00:00 2001
From: Antoine Martin anto...@devloop.org.uk
Date: Wed, 13 Apr 2011 19:30:26 +0700
Subject: [PATCH] Constifies and increases the maximum size allowed for dummy
 screens to 32767x32767

Signed-off-by: Antoine Martin anto...@devloop.org.uk
Tested-by: Antoine Martin anto...@devloop.org.uk
---
 src/dummy_driver.c |7 +--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/dummy_driver.c b/src/dummy_driver.c
index 804e41e..305bd3c 100644
--- a/src/dummy_driver.c
+++ b/src/dummy_driver.c
@@ -85,6 +85,9 @@ static Bool   dummyDriverFunc(ScrnInfoPtr pScrn, 
xorgDriverFuncOp op,
 #define DUMMY_MINOR_VERSION PACKAGE_VERSION_MINOR
 #define DUMMY_PATCHLEVEL PACKAGE_VERSION_PATCHLEVEL
 
+#define DUMMY_MAX_WIDTH 32767
+#define DUMMY_MAX_HEIGHT 32767
+
 /*
  * This is intentionally screen-independent.  It indicates the binding
  * choice made in the first PreInit.
@@ -402,8 +405,8 @@ DUMMYPreInit(ScrnInfoPtr pScrn, int flags)
int apertureSize = (pScrn-videoRam * 1024);
i = xf86ValidateModes(pScrn, pScrn-monitor-Modes,
  pScrn-display-modes, clockRanges,
- NULL, 256, 2048,(8 * pScrn-bitsPerPixel),
- 128, 2048, pScrn-display-virtualX,
+ NULL, 256, DUMMY_MAX_WIDTH,(8 * 
pScrn-bitsPerPixel),
+ 128, DUMMY_MAX_HEIGHT, pScrn-display-virtualX,
  pScrn-display-virtualY, apertureSize,
  LOOKUP_BEST_REFRESH);
 
-- 
1.7.4.2


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel