Re: [PATCH xserver v2] present: Only call present_flip_notify if vblank->queued == FALSE

2017-01-07 Thread Alex Deucher 
On Sat, Jan 7, 2017 at 3:36 AM, Michel Dänzer  wrote:
>
> Any takers?
>
> In the absence of a negative review, I'll push this next week, since
> more people are running into this.


Reviewed-by: Alex Deucher 

>
>
> On 28/11/16 10:54 AM, Michel Dänzer wrote:
>> From: Michel Dänzer 
>>
>> We are no longer using the present_flip_queue list only for presents
>> which have already been submitted to the driver for page flipping, but
>> also for those which we are queueing up to be flipped later, marked
>> with vblank->queued == TRUE. We were incorrectly calling
>> present_flip_notify for such entries, failing the assertion in
>> present_flip_notify (or presumably resulting in other undesirable
>> behaviour with assertions disabled).
>>
>> Reproduction recipe: Run the JavaFX test case referenced by
>> https://bugs.freedesktop.org/show_bug.cgi?id=98831#c6 and alt-tab out
>> of it while it's fullscreen. May take a few attempts to hit the
>> assertion failure.
>>
>> Fixes: bab0f450a719 ("present: Fix presentation of flips out of order")
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98854
>> Signed-off-by: Michel Dänzer 
>> ---
>>
>> v2: Add bugzilla reference
>>
>>  present/present.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/present/present.c b/present/present.c
>> index a7ca06e..ef89045 100644
>> --- a/present/present.c
>> +++ b/present/present.c
>> @@ -536,7 +536,10 @@ present_event_notify(uint64_t event_id, uint64_t ust, 
>> uint64_t msc)
>>  }
>>  xorg_list_for_each_entry(vblank, _flip_queue, event_queue) {
>>  if (vblank->event_id == event_id) {
>> -present_flip_notify(vblank, ust, msc);
>> +if (vblank->queued)
>> +present_execute(vblank, ust, msc);
>> +else
>> +present_flip_notify(vblank, ust, msc);
>>  return;
>>  }
>>  }
>>
>
>
> --
> Earthling Michel Dänzer   |   http://www.amd.com
> Libre software enthusiast | Mesa and X developer
> ___
> 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 libXrandr] Avoid out of boundary accesses on illegal responses

2017-01-07 Thread Tobias Stoeckmann 
Hi Julien,

On Sat, Jan 07, 2017 at 07:03:17PM +0100, Julien Cristau wrote:
> It looks like we're leaking 'attr' on these error paths?

confirmed. That is what I get for copying the error handling of the
attr == NULL case...


diff --git a/src/XrrCrtc.c b/src/XrrCrtc.c
index 6665092..8316b78 100644
--- a/src/XrrCrtc.c
+++ b/src/XrrCrtc.c
@@ -459,6 +459,7 @@ XRRGetCrtcTransform (Display*dpy,
 e = extra;
 
 if (e + rep.pendingNbytesFilter > end) {
+   XFree (attr);
XFree (extra);
return False;
 }
@@ -468,6 +469,7 @@ XRRGetCrtcTransform (Display*dpy,
 for (p = 0; p < rep.pendingNparamsFilter; p++) {
INT32   f;
if (e + 4 > end) {
+   XFree (attr);
XFree (extra);
return False;
}
@@ -478,6 +480,7 @@ XRRGetCrtcTransform (Display*dpy,
 attr->pendingNparams = rep.pendingNparamsFilter;
 
 if (e + rep.currentNbytesFilter > end) {
+   XFree (attr);
XFree (extra);
return False;
 }
@@ -487,6 +490,7 @@ XRRGetCrtcTransform (Display*dpy,
 for (p = 0; p < rep.currentNparamsFilter; p++) {
INT32   f;
if (e + 4 > end) {
+   XFree (attr);
XFree (extra);
return False;
}
___
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 libXrandr] Avoid out of boundary accesses on illegal responses

2017-01-07 Thread Julien Cristau 
[resending with xorg-devel cc added which I forgot the first time around]

On Sun, Sep 25, 2016 at 22:50:44 +0200, Matthieu Herrb wrote:

> diff --git a/src/XrrCrtc.c b/src/XrrCrtc.c
> index 5ae35c5..6665092 100644
> --- a/src/XrrCrtc.c
> +++ b/src/XrrCrtc.c
[...]
> @@ -357,7 +378,7 @@ XRRGetCrtcTransform (Display  *dpy,
>  xRRGetCrtcTransformReq   *req;
>  int  major_version, minor_version;
>  XRRCrtcTransformAttributes   *attr;
> -char *extra = NULL, *e;
> +char *extra = NULL, *end = NULL, *e;
>  int  p;
>  
>  *attributes = NULL;
> @@ -395,9 +416,17 @@ XRRGetCrtcTransform (Display *dpy,
>   else
>   {
>   int extraBytes = rep.length * 4 - CrtcTransformExtra;
> - extra = Xmalloc (extraBytes);
> + if (rep.length < INT_MAX / 4 &&
> + rep.length * 4 >= CrtcTransformExtra) {
> + extra = Xmalloc (extraBytes);
> + end = extra + extraBytes;
> + } else
> + extra = NULL;
>   if (!extra) {
> - _XEatDataWords (dpy, rep.length - (CrtcTransformExtra >> 2));
> + if (rep.length > (CrtcTransformExtra >> 2))
> + _XEatDataWords (dpy, rep.length - (CrtcTransformExtra >> 
> 2));
> + else
> + _XEatDataWords (dpy, rep.length);
>   UnlockDisplay (dpy);
>   SyncHandle ();
>   return False;
> @@ -429,22 +458,38 @@ XRRGetCrtcTransform (Display*dpy,
>  
>  e = extra;
>  
> +if (e + rep.pendingNbytesFilter > end) {
> + XFree (extra);
> + return False;
> +}
>  memcpy (attr->pendingFilter, e, rep.pendingNbytesFilter);
>  attr->pendingFilter[rep.pendingNbytesFilter] = '\0';
>  e += (rep.pendingNbytesFilter + 3) & ~3;
>  for (p = 0; p < rep.pendingNparamsFilter; p++) {
>   INT32   f;
> + if (e + 4 > end) {
> + XFree (extra);
> + return False;
> + }
>   memcpy (, e, 4);
>   e += 4;
>   attr->pendingParams[p] = (XFixed) f;
>  }
>  attr->pendingNparams = rep.pendingNparamsFilter;
>  
> +if (e + rep.currentNbytesFilter > end) {
> + XFree (extra);
> + return False;
> +}
>  memcpy (attr->currentFilter, e, rep.currentNbytesFilter);
>  attr->currentFilter[rep.currentNbytesFilter] = '\0';
>  e += (rep.currentNbytesFilter + 3) & ~3;
>  for (p = 0; p < rep.currentNparamsFilter; p++) {
>   INT32   f;
> + if (e + 4 > end) {
> + XFree (extra);
> + return False;
> + }
>   memcpy (, e, 4);
>   e += 4;
>   attr->currentParams[p] = (XFixed) f;

It looks like we're leaking 'attr' on these error paths?

Cheers,
Julien
___
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 libX11] Fix wrong Xfree in XListFonts failure path

2017-01-07 Thread Alan Coopersmith 

Reviewed-by: Alan Coopersmith 

Looks like this bug was introduced just after the 1.6.4 release and hasn't
made it out into a libX11 release yet - thanks for catching it in time.

-alan-

On 01/ 7/17 07:20 AM, Julien Cristau wrote:

'ch' gets moved inside the allocated buffer as we're looping through
fonts, so keep a reference to the start of the buffer so we can pass
that to Xfree in the failure case.

Fixes: commit 20a3f99eba5001925b8b313da3accb7900eb1927 "Plug a memory leak"

Signed-off-by: Julien Cristau 
---
 src/FontNames.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/FontNames.c b/src/FontNames.c
index 3e23b5f4..9ffdfd29 100644
--- a/src/FontNames.c
+++ b/src/FontNames.c
@@ -43,6 +43,7 @@ int *actualCount) /* RETURN */
 register int length;
 char **flist = NULL;
 char *ch = NULL;
+char *chstart;
 char *chend;
 int count = 0;
 xListFontsReply rep;
@@ -86,6 +87,7 @@ int *actualCount) /* RETURN */
/*
 * unpack into null terminated strings.
 */
+   chstart = ch;
chend = ch + (rlen + 1);
length = *(unsigned char *)ch;
*ch = 1; /* make sure it is non-zero for XFreeFontNames */
@@ -98,14 +100,14 @@ int *actualCount)  /* RETURN */
*ch = '\0';  /* and replace with null-termination */
count++;
} else {
-Xfree(ch);
+Xfree(chstart);
 Xfree(flist);
 flist = NULL;
 count = 0;
 break;
}
} else {
-Xfree(ch);
+Xfree(chstart);
 Xfree(flist);
 flist = NULL;
 count = 0;




--
-Alan Coopersmith-  alan.coopersm...@oracle.com
 Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
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 libXrender 1/2] Avoid OOB write in XRenderQueryFilters

2017-01-07 Thread Julien Cristau 
On Sun, Sep 25, 2016 at 22:50:45 +0200, Matthieu Herrb wrote:

> From: Tobias Stoeckmann 
> 
> The memory for filter names is reserved right after receiving the reply.
> After that, filters are iterated and each individual filter name is
> stored in that reserved memory.
> 
> The individual name lengths are not checked for validity, which means
> that a malicious server can reserve less memory than it will write to
> during each iteration.
> 
> v2: consume remaining bytes in reply buffer on error.
> 
> Signed-off-by: Tobias Stoeckmann 
> Reviewed-by: Matthieu Herrb 
> ---
>  src/Filter.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/Filter.c b/src/Filter.c
> index edfa572..8d701eb 100644
> --- a/src/Filter.c
> +++ b/src/Filter.c
> @@ -38,7 +38,7 @@ XRenderQueryFilters (Display *dpy, Drawable drawable)
>  char *name;
>  char len;
>  int  i;
> -unsigned longnbytes, nbytesAlias, nbytesName;
> +unsigned longnbytes, nbytesAlias, nbytesName, reply_left;
>  
>  if (!RenderHasExtension (info))
>   return NULL;
> @@ -114,6 +114,7 @@ XRenderQueryFilters (Display *dpy, Drawable drawable)
>   * Read the filter aliases
>   */
>  _XRead16Pad (dpy, filters->alias, 2 * rep.numAliases);
> +reply_left = 8 + rep.length - 2 * rep.numAliases;;
>  
reply_left looks like a byte count, in which case shouldn't rep.length
be multiplied by 4?  I don't get where that 8 comes from, either, any
chance you could explain?  In fact I wonder if this couldn't use
nbytesName instead?

Cheers,
Julien
___
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 libX11] Fix wrong Xfree in XListFonts failure path

2017-01-07 Thread Julien Cristau 
'ch' gets moved inside the allocated buffer as we're looping through
fonts, so keep a reference to the start of the buffer so we can pass
that to Xfree in the failure case.

Fixes: commit 20a3f99eba5001925b8b313da3accb7900eb1927 "Plug a memory leak"

Signed-off-by: Julien Cristau 
---
 src/FontNames.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/FontNames.c b/src/FontNames.c
index 3e23b5f4..9ffdfd29 100644
--- a/src/FontNames.c
+++ b/src/FontNames.c
@@ -43,6 +43,7 @@ int *actualCount) /* RETURN */
 register int length;
 char **flist = NULL;
 char *ch = NULL;
+char *chstart;
 char *chend;
 int count = 0;
 xListFontsReply rep;
@@ -86,6 +87,7 @@ int *actualCount) /* RETURN */
/*
 * unpack into null terminated strings.
 */
+   chstart = ch;
chend = ch + (rlen + 1);
length = *(unsigned char *)ch;
*ch = 1; /* make sure it is non-zero for XFreeFontNames */
@@ -98,14 +100,14 @@ int *actualCount)  /* RETURN */
*ch = '\0';  /* and replace with null-termination */
count++;
} else {
-Xfree(ch);
+Xfree(chstart);
 Xfree(flist);
 flist = NULL;
 count = 0;
 break;
}
} else {
-Xfree(ch);
+Xfree(chstart);
 Xfree(flist);
 flist = NULL;
 count = 0;
-- 
2.11.0

___
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] Add "s query" to print screen saver state and timing.

2017-01-07 Thread Nicolas George 
The output looks like:
state = 0 (off)
kind = 1 (internal)
until = 557900 (0:09:17.900)
idle = 42100 (0:00:42.100)
event_mask = 0

Signed-off-by: Nicolas George 
---
 configure.ac |  2 ++
 man/xset.man |  2 +-
 xset.c   | 42 +-
 3 files changed, 44 insertions(+), 2 deletions(-)


Hi. This is a feature that I frequently use in script from a personal
standalone program. I think it may be useful for many other people.

I have tried to follow the instructions on the wiki page
https://www.x.org/wiki/Development/Documentation/SubmittingPatches/
and the surrounding coding style, but I am not very familiar with the
autotools features.

Regards,

-- 
  Nicolas George


diff --git a/configure.ac b/configure.ac
index 77de8f4..8d77ba3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -58,6 +58,8 @@ PKG_CHECK_MODULES(SET_XKB, x11,
AC_CHECK_HEADERS([X11/XKBlib.h],,,[#include ])
CPPFLAGS="$SAVE_CPPFLAGS"],[echo "not found"])
 
+AC_CHECK_LIB(Xss, XScreenSaverQueryInfo, [], [], [-lX11])
+
 AC_ARG_WITH(xf86misc, AS_HELP_STRING([--without-xf86misc],[Disable xf86misc 
support.]),
 [USE_XF86MISC="$withval"], [USE_XF86MISC="yes"])
 if test "x$USE_XF86MISC" != "xno" ; then
diff --git a/man/xset.man b/man/xset.man
index e9f1c5e..48544be 100644
--- a/man/xset.man
+++ b/man/xset.man
@@ -61,7 +61,7 @@ xset - user preference utility for X
 [r {on|off}] [r rate \fIdelay\fP [\fIrate\fP]]
 .br
 [s [\fIlength\fP [\fIperiod\fP]]] [s {blank|noblank}]
-[s {expose|noexpose}] [s {on|off}] [s default] [s activate] [s reset]
+[s {expose|noexpose}] [s {on|off}] [s default] [s activate] [s reset] [s query]
 .br
 [q]
 .br
diff --git a/xset.c b/xset.c
index 95da41b..3d1a136 100644
--- a/xset.c
+++ b/xset.c
@@ -37,6 +37,10 @@ in this Software without prior written authorization from 
The Open Group.
 # define MITMISC
 #endif
 
+#ifdef HAVE_LIBXSS
+# define XSS
+#endif
+
 #ifdef HAVE_X11_XKBLIB_H
 # define XKB
 #endif
@@ -69,6 +73,9 @@ in this Software without prior written authorization from The 
Open Group.
 #ifdef DPMSExtension
 # include 
 #endif /* DPMSExtension */
+#ifdef XSS
+# include 
+#endif
 
 #ifdef XF86MISC
 # include 
@@ -635,6 +642,39 @@ main(int argc, char *argv[])
} else if (strcmp(arg, "reset") == 0) { /* force it inactive */
XResetScreenSaver(dpy);
i++;
+} else if (strcmp(arg, "query") == 0) {
+#ifdef XSS
+XScreenSaverInfo *info;
+info = XScreenSaverAllocInfo();
+if (!info)
+error("out of memory for font screen saver query");
+XScreenSaverQueryInfo(dpy, DefaultRootWindow(dpy), info);
+printf("state = %d (%s)\n",
+   info->state,
+   info->state == ScreenSaverOff ? "off" :
+   info->state == ScreenSaverOn ? "on" :
+   info->state == ScreenSaverDisabled ? "disabled" : 
"unknown");
+printf("kind = %d (%s)\n",
+   info->kind,
+   info->kind == ScreenSaverBlanked ? "blanked" :
+   info->kind == ScreenSaverInternal ? "internal" :
+   info->kind == ScreenSaverExternal ? "external" : 
"unknown");
+printf("until = %ld (%ld:%02ld:%02ld.%03ld)\n", 
info->til_or_since,
+   info->til_or_since / 360,
+   (info->til_or_since / 6) % 60,
+   (info->til_or_since / 1000) % 60,
+   info->til_or_since % 1000);
+printf("idle = %ld (%ld:%02ld:%02ld.%03ld)\n", info->idle,
+   info->idle / 360,
+   (info->idle / 6) % 60,
+   (info->idle / 1000) % 60,
+   info->idle % 1000);
+printf("event_mask = %lX\n", info->eventMask);
+XFree(info);
+#else
+error("  xset was not built with MIT-SCREEN-SAVER Extension 
support\n");
+#endif
+i++;
} else if (*arg >= '0' && *arg <= '9') {/* Set as user wishes. 
*/
set_saver(dpy, TIMEOUT, atoi(arg));
i++;
@@ -1653,7 +1693,7 @@ usage(const char *fmt, ...)
 "For screen-saver control:\n"
 "\t s [timeout [cycle]]  s defaults on\n"
 "\t s blank  s noblanks off\n"
-"\t s expose s noexpose\n"
+"\t s expose s noexpose   s query\n"
 "\t s activate   s reset\n"
 "For status information:  q\n"
 "To print version: -version\n"
-- 
2.11.0

___
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 v2] present: Only call present_flip_notify if vblank->queued == FALSE

2017-01-07 Thread Michel Dänzer 

Any takers?

In the absence of a negative review, I'll push this next week, since
more people are running into this.


On 28/11/16 10:54 AM, Michel Dänzer wrote:
> From: Michel Dänzer 
> 
> We are no longer using the present_flip_queue list only for presents
> which have already been submitted to the driver for page flipping, but
> also for those which we are queueing up to be flipped later, marked
> with vblank->queued == TRUE. We were incorrectly calling
> present_flip_notify for such entries, failing the assertion in
> present_flip_notify (or presumably resulting in other undesirable
> behaviour with assertions disabled).
> 
> Reproduction recipe: Run the JavaFX test case referenced by
> https://bugs.freedesktop.org/show_bug.cgi?id=98831#c6 and alt-tab out
> of it while it's fullscreen. May take a few attempts to hit the
> assertion failure.
> 
> Fixes: bab0f450a719 ("present: Fix presentation of flips out of order")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98854
> Signed-off-by: Michel Dänzer 
> ---
> 
> v2: Add bugzilla reference
> 
>  present/present.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/present/present.c b/present/present.c
> index a7ca06e..ef89045 100644
> --- a/present/present.c
> +++ b/present/present.c
> @@ -536,7 +536,10 @@ present_event_notify(uint64_t event_id, uint64_t ust, 
> uint64_t msc)
>  }
>  xorg_list_for_each_entry(vblank, _flip_queue, event_queue) {
>  if (vblank->event_id == event_id) {
> -present_flip_notify(vblank, ust, msc);
> +if (vblank->queued)
> +present_execute(vblank, ust, msc);
> +else
> +present_flip_notify(vblank, ust, msc);
>  return;
>  }
>  }
> 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
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