Re: [PATCH] Xext/xres.c: Possible buffer underrun

[Rami Ylimäki]

On 07/23/2012 03:05 PM, Colin Harrison wrote:

Hi,

I got a crash at free(counts) in ProcXResQueryClientResources() in
Xext/xres.c when using client xrestop.

Traced to an out-by-one error in ResFindAllRes() (please check code and
confirm?).

Fixed, for me (MinGW compilation for Windows), with the patch...

--- ./Xext/save_xres.c  2012-07-10 11:16:44.191904782 +0100
+++ ./Xext/xres.c   2012-07-16 16:19:50.078292944 +0100
@@ -274,7 +274,7 @@
  {
  int *counts = (int *) cdata;

-counts[(type  TypeMask) - 1]++;
+counts[(type  TypeMask)]++;


This would probably send wrong resource counts back to clients. It's 
better to just skip any resources with RT_NONE as the resource type.


It'd be also nice to know whether it's acceptable to have RT_NONE 
resources in the resource database. StoreFontClientFont seems to add 
such resources, but I'm not too familiar with that code.



  }

  static int

Thanks,
Colin Harrison



Regards,
Rami Ylimäki

___
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] dix: Prevent access to freed memory if a client kills itself.

[Rami Ylimäki]

The 'Dispatch' function accesses freed client structure if a client
happens to kill itself in a request. For example, I have a test client
that is used to check that it handles the XIO error correctly. The XIO
error is generated by requesting the client to kill itself with
XKillClient.

We don't have to care about LBX specific functionality anymore because
LBX support has been removed from the server.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
Reviewed-by: Jamey Sharp ja...@minilop.net
---
Just and older patch that is already reviewed but was never picked.

 dix/dispatch.c |   24 ++--
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/dix/dispatch.c b/dix/dispatch.c
index 44c2433..fced038 100644
--- a/dix/dispatch.c
+++ b/dix/dispatch.c
@@ -3278,21 +3278,17 @@ ProcKillClient(ClientPtr client)
 }
 
 rc = dixLookupClient(killclient, stuff-id, client, DixDestroyAccess);
-if (rc == Success) {
-   CloseDownClient(killclient);
-   /* if an LBX proxy gets killed, isItTimeToYield will be set */
-   if (isItTimeToYield || (client == killclient))
-   {
-   /* force yield and return Success, so that Dispatch()
-* doesn't try to touch client
-*/
-   isItTimeToYield = TRUE;
-   return Success;
-   }
-   return Success;
+if (rc == Success)
+{
+if (client == killclient)
+{
+MarkClientException(client);
+isItTimeToYield = TRUE;
+}
+else
+CloseDownClient(killclient);
 }
-else
-   return rc;
+return rc;
 }
 
 int
-- 
1.7.4.1

___
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: [Resend: PATCH v2] Add Solaris support to DetermineClientCmd

[Rami Ylimäki]

On 12/20/2011 11:15 PM, Alan Coopersmith wrote:

Uses /proc/pid/psinfo to read command  partial arguments.


Based on the comments in the code, reading from that file looks OK. I'm 
not otherwise familiar with this interface.




Moves cmdsize  argsize variables into non-Solaris #else clause
to avoid unused variable warnings.


Seems OK.



Fixes format mismatch errors when building with DEBUG defined on
a 64-bit platform (where Mask is defined as CARD32).


Looks fine as well.



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


Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

___
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] Include client name if available in PrintDeviceGrabInfo

[Rami Ylimäki]

On 12/08/2011 09:02 AM, Alan Coopersmith wrote:

Also adds missing newline to first line of output.

Before patch:

[3581472.414] (II) Printing all currently active device grabs:
[3581472.414] Active grab 0x180 (core) on device 'Virtual core pointer' (2):
   client pid 26174 uid 0 gid 10
[3581472.415]   at 3581469139 (from active grab) (device thawed, state 1)
[3581472.415] core event mask 0x0
[3581472.415]   owner-events true, kb 1 ptr 1, confine 0, cursor 0x0
[3581472.415] Active grab 0x180 (core) on device 'Virtual core keyboard' (3)
:  client pid 26174 uid 0 gid 10
[3581472.415]   at 3581469139 (from active grab) (device thawed, state 1)
[3581472.415] core event mask 0x3
[3581472.415]   owner-events true, kb 1 ptr 1, confine 0, cursor 0x0
[3581472.415] (II) End list of active device grabs

After patch:
[3581736.601] (II) Printing all currently active device grabs:
[3581736.601] Active grab 0x160 (core) on device 'Virtual core pointer' (2):
[3581736.601]   client pid 26741 /usr/bin/xscreensaver -nosplash
[3581736.601]   at 3581735000 (from active grab) (device thawed, state 1)
[3581736.601] core event mask 0x0
[3581736.601]   owner-events true, kb 1 ptr 1, confine 0, cursor 0x0
[3581736.601] Active grab 0x160 (core) on device 'Virtual core keyboard' (3)
:
[3581736.601]   client pid 26741 /usr/bin/xscreensaver -nosplash
[3581736.601]   at 3581735000 (from active grab) (device thawed, state 1)
[3581736.601] core event mask 0x3
[3581736.601]   owner-events true, kb 1 ptr 1, confine 0, cursor 0x0
[3581736.601] (II) End list of active device grabs

Signed-off-by: Alan Coopersmithalan.coopersm...@oracle.com


Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi


+ErrorF(  (no client information available for client %d)\n,
+   CLIENT_ID(grab-resource));


I'd prefer the format to be %lx as that is used generally for resources. 
That would make it easier to find the client XID from output of xrestop 
for example.


-- Rami

___
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] Save major/minor opcodes in ClientRec for RecordAReply

[Rami Ylimäki]

On 11/08/2011 08:22 PM, Keith Packard wrote:

The record extension needs the major and minor opcodes in the reply
hook, but the request buffer may have been freed by the time the hook
is invoked. Saving the request major and minor codes as the request is
executed avoids fetching from the defunct request buffer.

This patch also eliminates the public MinorOpcodeOfRequest function,
making it static to dispatch. Usages of that function have been
replaced with direct access to the new ClientRec field.

Signed-off-by: Keith Packardkei...@keithp.com
---

Here's what I was thinking of to fix this -- just record the major and
minor opcodes of the request in the ClientRec during Dispatch and then
using those fields in RecordAReply instead of fetching the discarded
request buffer.

This is entirely untested; I don't know how to make the old code break.


Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

This is the easiest and most reliable way to fix the problem. I tested 
the fix and it works.




Reproduction steps:

1. Run X server under Valgrind.
2. Record replies to an offending request (ListFontsWithInfo, 
RecordEnableContext, DRI2WaitMSC, ... ?). I used cnee --record -erpmar 
128-135 to record replies to Record extension as I know that the major 
code of that extension is somewhere in range 128-135 in my environment.
3. Run a client that executes the offending request in another shell. I 
used cnee --record --mouse as it executes RecordEnableContext.

4. Kill the client that was used in step 3. I just sent Ctrl+C to cnee.
5. Examine Valgrind log. With the above steps I can always see 
complaints about invalid reads when major-op is read from the freed 
request buffer. After the fix this specific complaint is gone.




This is unrelated but during testing I found a yet another bug in Record 
extension at least in 1.9.5 server:


1. In one shell: cnee --record -repra 0-255
2. In other shell cnee --record --mouse

X server will examine whether the range is valid, determines that it's 
not and crashes with an assert. This is unrelated to the fix.


___
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 3/3] record: Preserve client input buffer for RecordEnableContext request.

[Rami Ylimäki]

On 10/03/2011 06:03 PM, Keith Packard wrote:

On Mon,  3 Oct 2011 15:16:27 +0300, Rami Ylimäkirami.ylim...@vincit.fi  wrote:


This request installs hooks that keep sending replies even after the
request handler has finished. Each reply accesses the input buffer. If
we let the buffer to be shared, we will eventually read garbage or
even from freed memory.

Would it be possible to simply store any necessary data locally, instead
of hacking up the OS layer to store it for us?



It should be possible, but I think that the current approach is the 
simplest way to solve the problem. I'll present some alternatives below 
that occurred to me when fixing the problem.


A client might start recording replies without selecting any requests 
for recording. This means that the Record extension would have to track 
and cache requests of all clients even though the requests themselves 
wouldn't be recorded. Caching would need to be done for all clients, 
instead of just recorded clients, because we wouldn't know in advance 
whether replies to problematic requests, such as RecordEnableContext, 
would be recorded later on.


Saving the op-codes of latest requests could be done in a couple of ways:
1. Add op-code fields into the ClientRec structure and update them in 
Dispatch/ReadRequestFromClient just like ClientRec::requestBuffer is 
updated.
2. Track client destruction and creation with ClientStateCallback and 
wrap the client request vector for all clients in Record extension. This 
would allow the the latest request op-codes to be saved into a client 
private structure of Record extension, where it could be fetched when a 
reply is recorded.


Any preferences? I guess option 2 is the most local way to fix the 
problem, but also requires more code lines than the other approaches.


-- Rami

___
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 0/3] Prevent access to freed input buffer.

[Rami Ylimäki]

Some requests peek the client input buffer even after the request
handler has returned. For example, RecordEnableContext calls
IgnoreClient for the connection that originates the request, but also
installs callbacks that keep sending data to the connection until
RecordDisableContext is called from another connection. The callbacks
also assume that they can fetch the latest request op-codes from the
client input buffer (in RecordAReply), which is usually true for
recorded clients, but fails for recording clients.

Rami Ylimäki (3):
  os: Collect copy-pasted code into functions.
  os: Allow requests to preseve client input buffers for a longer time.
  record: Preserve client input buffer for RecordEnableContext request.

 include/os.h|2 +
 os/connection.c |7 +++
 os/io.c |  141 ---
 os/osdep.h  |4 ++
 record/record.c |7 +++
 5 files changed, 102 insertions(+), 59 deletions(-)

___
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 1/3] os: Collect copy-pasted code into functions.

[Rami Ylimäki]

No functional changes.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 os/io.c |  126 +-
 1 files changed, 67 insertions(+), 59 deletions(-)

diff --git a/os/io.c b/os/io.c
index 955bf8b..6bfddfc 100644
--- a/os/io.c
+++ b/os/io.c
@@ -190,11 +190,69 @@ YieldControlDeath(void)
 timesThisConnection = 0;
 }
 
+/**
+ * Check whether the previous request indicated that an input buffer
+ * can be reused. If the input buffer belongs to the same client, it
+ * can be trivially reused. If the input buffer belongs to some other
+ * client, it is stolen and marked as free so that clients without
+ * input buffers can reuse it. However, if the input buffer has grown
+ * too large in the latter case, it's just released.
+ *
+ * @param[in] oc connection that requires an input buffer
+ */
+static void FreeAvailableInput(OsCommPtr oc)
+{
+if (AvailableInput)
+{
+   if (AvailableInput != oc)
+   {
+   register ConnectionInputPtr aci = AvailableInput-input;
+   if (aci-size  BUFWATERMARK)
+   {
+   free(aci-buffer);
+   free(aci);
+   }
+   else
+   {
+   aci-next = FreeInputs;
+   FreeInputs = aci;
+   }
+   AvailableInput-input = NULL;
+   }
+   AvailableInput = NULL;
+}
+}
+
+/**
+ * Find an input buffer for a request. If a client already has an
+ * input buffer, nothing needs to be done. Otherwise the buffer is
+ * taken from the list of reusable input buffers. If no input buffers
+ * can be reused, a new one is allocated.
+ *
+ * @param[in,out] oc connection that requires an input buffer
+ *
+ * @return the input buffer that was given for the connection
+ */
+static ConnectionInputPtr ReuseFreeInput(OsCommPtr oc)
+{
+ConnectionInputPtr oci = oc-input;
+if (!oci)
+{
+   if ((oci = FreeInputs))
+   FreeInputs = oci-next;
+   else
+   oci = AllocateInputBuffer();
+}
+if (oci)
+   oc-input = oci;
+return oci;
+}
+
 int
 ReadRequestFromClient(ClientPtr client)
 {
 OsCommPtr oc = (OsCommPtr)client-osPrivate;
-ConnectionInputPtr oci = oc-input;
+ConnectionInputPtr oci;
 int fd = oc-fd;
 unsigned int gotnow, needed;
 int result;
@@ -208,40 +266,14 @@ ReadRequestFromClient(ClientPtr client)
  * times).  This was done to save memory.
  */
 
-if (AvailableInput)
-{
-   if (AvailableInput != oc)
-   {
-   register ConnectionInputPtr aci = AvailableInput-input;
-   if (aci-size  BUFWATERMARK)
-   {
-   free(aci-buffer);
-   free(aci);
-   }
-   else
-   {
-   aci-next = FreeInputs;
-   FreeInputs = aci;
-   }
-   AvailableInput-input = (ConnectionInputPtr)NULL;
-   }
-   AvailableInput = (OsCommPtr)NULL;
-}
+FreeAvailableInput(oc);
 
 /* make sure we have an input buffer */
 
-if (!oci)
+if (!(oci = ReuseFreeInput(oc)))
 {
-   if ((oci = FreeInputs))
-   {
-   FreeInputs = oci-next;
-   }
-   else if (!(oci = AllocateInputBuffer()))
-   {
-   YieldControlDeath();
-   return -1;
-   }
-   oc-input = oci;
+YieldControlDeath();
+return -1;
 }
 
 /* advance to start of next request */
@@ -501,37 +533,13 @@ Bool
 InsertFakeRequest(ClientPtr client, char *data, int count)
 {
 OsCommPtr oc = (OsCommPtr)client-osPrivate;
-ConnectionInputPtr oci = oc-input;
+ConnectionInputPtr oci;
 int fd = oc-fd;
 int gotnow, moveup;
 
-if (AvailableInput)
-{
-   if (AvailableInput != oc)
-   {
-   ConnectionInputPtr aci = AvailableInput-input;
-   if (aci-size  BUFWATERMARK)
-   {
-   free(aci-buffer);
-   free(aci);
-   }
-   else
-   {
-   aci-next = FreeInputs;
-   FreeInputs = aci;
-   }
-   AvailableInput-input = (ConnectionInputPtr)NULL;
-   }
-   AvailableInput = (OsCommPtr)NULL;
-}
-if (!oci)
-{
-   if ((oci = FreeInputs))
-   FreeInputs = oci-next;
-   else if (!(oci = AllocateInputBuffer()))
-   return FALSE;
-   oc-input = oci;
-}
+FreeAvailableInput(oc);
+if (!(oci = ReuseFreeInput(oc)))
+return FALSE;
 oci-bufptr += oci-lenLastReq;
 oci-lenLastReq = 0;
 gotnow = oci-bufcnt + oci-buffer - oci-bufptr;
-- 
1.7.1

___
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] record: Prevent out of bounds access when recording a reply.

[Rami Ylimäki]

Any pad bytes in replies are written to the client from a zeroed
array. However, record extension tries to incorrectly access the pad
bytes from the end of reply data.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 include/os.h|3 ++-
 os/io.c |1 +
 record/record.c |   48 +++-
 3 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/include/os.h b/include/os.h
index 5401ea4..fda0181 100644
--- a/include/os.h
+++ b/include/os.h
@@ -451,9 +451,10 @@ extern _X_EXPORT CallbackListPtr ReplyCallback;
 typedef struct {
 ClientPtr client;
 const void *replyData;
-unsigned long dataLenBytes;
+unsigned long dataLenBytes; /* actual bytes from replyData + pad bytes */
 unsigned long bytesRemaining;
 Bool startOfReply;
+unsigned long padBytes; /* pad bytes from zeroed array */
 } ReplyInfoRec;
 
 /* stuff for FlushCallback */
diff --git a/os/io.c b/os/io.c
index 068f5f0..955bf8b 100644
--- a/os/io.c
+++ b/os/io.c
@@ -809,6 +809,7 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
replyinfo.client = who;
replyinfo.replyData = buf;
replyinfo.dataLenBytes = count + padBytes;
+   replyinfo.padBytes = padBytes;
if (who-replyBytesRemaining)
{ /* still sending data of an earlier reply */
who-replyBytesRemaining -= count + padBytes;
diff --git a/record/record.c b/record/record.c
index 68311ac..5cae2b9 100644
--- a/record/record.c
+++ b/record/record.c
@@ -269,8 +269,9 @@ RecordFlushReplyBuffer(
  *   device events and EndOfData, pClient is NULL.
  * category is the category of the protocol element, as defined
  *   by the RECORD spec.
- * data is a pointer to the protocol data, and datalen is its length
- *   in bytes.
+ * data is a pointer to the protocol data, and datalen - padlen
+ *   is its length in bytes.
+ * padlen is the number of pad bytes from a zeroed array.
  * futurelen is the number of bytes that will be sent in subsequent
  *   calls to this function to complete this protocol element.  
  *   In those subsequent calls, futurelen will be -1 to indicate
@@ -290,7 +291,7 @@ RecordFlushReplyBuffer(
  */
 static void
 RecordAProtocolElement(RecordContextPtr pContext, ClientPtr pClient,
-  int category, pointer data, int datalen, int futurelen)
+  int category, pointer data, int datalen, int padlen, int 
futurelen)
 {
 CARD32 elemHeaderData[2];
 int numElemHeaders = 0;
@@ -398,9 +399,13 @@ RecordAProtocolElement(RecordContextPtr pContext, 
ClientPtr pClient,
}
if (datalen)
{
+   static char padBuffer[3]; /* as in FlushClient */
memcpy(pContext-replyBuffer + pContext-numBufBytes,
-  data, datalen);
-   pContext-numBufBytes += datalen;
+  data, datalen - padlen);
+   pContext-numBufBytes += datalen - padlen;
+   memcpy(pContext-replyBuffer + pContext-numBufBytes,
+  padBuffer, padlen);
+   pContext-numBufBytes += padlen;
}
 }
 else
@@ -483,19 +488,19 @@ RecordABigRequest(RecordContextPtr pContext, ClientPtr 
client, xReq *stuff)
 /* record the request header */
 bytesLeft = client-req_len  2;
 RecordAProtocolElement(pContext, client, XRecordFromClient,
-  (pointer)stuff, SIZEOF(xReq), bytesLeft);
+  (pointer)stuff, SIZEOF(xReq), 0, bytesLeft);
 
 /* reinsert the extended length field that was squished out */
 bigLength = client-req_len + bytes_to_int32(sizeof(bigLength));
 if (client-swapped)
swapl(bigLength);
 RecordAProtocolElement(pContext, client, XRecordFromClient,
-   (pointer)bigLength, sizeof(bigLength), /* continuation */ -1);
+   (pointer)bigLength, sizeof(bigLength), 0, /* continuation */ 
-1);
 bytesLeft -= sizeof(bigLength);
 
 /* record the rest of the request after the length */
 RecordAProtocolElement(pContext, client, XRecordFromClient,
-   (pointer)(stuff + 1), bytesLeft, /* continuation */ -1);
+   (pointer)(stuff + 1), bytesLeft, 0, /* continuation */ -1);
 } /* RecordABigRequest */
 
 
@@ -542,7 +547,7 @@ RecordARequest(ClientPtr client)
RecordABigRequest(pContext, client, stuff);
else
RecordAProtocolElement(pContext, client, XRecordFromClient,
-   (pointer)stuff, client-req_len  2, 0);
+   (pointer)stuff, client-req_len  2, 0, 0);
}
else /* extension, check minor opcode */
{
@@ -566,7 +571,7 @@ RecordARequest(ClientPtr client)
else
RecordAProtocolElement(pContext, client

Re: [PATCH] dix: Prevent access to freed memory if a client kills itself.

[Rami Ylimäki]

On 09/22/2011 07:34 PM, Jamey Sharp wrote:

This patch makes sense to me, but I have a couple of requests:

I'm not sure why you extracted out a separate function; I'm not
convinced that makes the code more clear, in this case.


Originally, there was no good reason for extracting the code to a 
separate function. However, I'm currently suspecting that a certain XACE 
module 
(https://meego.gitorious.org/maemo-multimedia/xserver-policy-enforcement) might 
be suffering from the same problem. In this case I'd want to have a 
separate function and export it as well so that I wouldn't have to 
copy-paste its contents into the XACE module. If it turns out that XPE 
works fine, I can remove that function in the next patch revision.



More importantly, I'd like to see justification in the commit message
for deleting LBX support here. I seem to recall that support was
deleted from the rest of the server already, which would be excellent
justification, but please say so if that's true.


To be honest, I didn't know what LBX was when I wrote the patch and 
didn't take into account the effect of extensions on closing down 
clients. But it seems that LBX has been gone for 5 years now, so I can 
clarify this in the commit message of the next patch revision.


-- Rami

___
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 v2 xserver] dix: Prevent access to freed memory if a client kills itself.

[Rami Ylimäki]

The 'Dispatch' function accesses freed client structure if a client
happens to kill itself in a request. For example, I have a test client
that is used to check that it handles the XIO error correctly. The XIO
error is generated by requesting the client to kill itself with
XKillClient.

We don't have to care about LBX specific functionality anymore because
LBX support has been removed from the server.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
XPE turned out to be OK and there is no need to export functions for
it.

 dix/dispatch.c |   24 ++--
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/dix/dispatch.c b/dix/dispatch.c
index 43cb4d1..b544899 100644
--- a/dix/dispatch.c
+++ b/dix/dispatch.c
@@ -3263,21 +3263,17 @@ ProcKillClient(ClientPtr client)
 }
 
 rc = dixLookupClient(killclient, stuff-id, client, DixDestroyAccess);
-if (rc == Success) {
-   CloseDownClient(killclient);
-   /* if an LBX proxy gets killed, isItTimeToYield will be set */
-   if (isItTimeToYield || (client == killclient))
-   {
-   /* force yield and return Success, so that Dispatch()
-* doesn't try to touch client
-*/
-   isItTimeToYield = TRUE;
-   return Success;
-   }
-   return Success;
+if (rc == Success)
+{
+if (client == killclient)
+{
+MarkClientException(client);
+isItTimeToYield = TRUE;
+}
+else
+CloseDownClient(killclient);
 }
-else
-   return rc;
+return rc;
 }
 
 int
-- 
1.7.1

___
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] dix: Prevent access to freed memory if a client kills itself.

[Rami Ylimäki]

The 'Dispatch' function accesses freed client structure if a client
happens to kill itself in a request. For example, I have a test client
that is used to check that it handles the XIO error correctly. The XIO
error is generated by requesting the client to kill itself with
XKillClient.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 dix/dispatch.c |   32 +---
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/dix/dispatch.c b/dix/dispatch.c
index 43cb4d1..cc5ee09 100644
--- a/dix/dispatch.c
+++ b/dix/dispatch.c
@@ -3226,6 +3226,20 @@ ProcChangeAccessControl(ClientPtr client)
 return ChangeAccessControl(client, stuff-mode == EnableAccess);
 }
 
+/**
+ * Prevents a client from killing itself immediately.
+ */
+static void CloseDownClientByClient(ClientPtr client, ClientPtr killclient)
+{
+if (client == killclient)
+{
+MarkClientException(client);
+isItTimeToYield = TRUE;
+}
+else
+CloseDownClient(killclient);
+}
+
 /*
  * CloseDownRetainedResources
  *
@@ -3263,21 +3277,9 @@ ProcKillClient(ClientPtr client)
 }
 
 rc = dixLookupClient(killclient, stuff-id, client, DixDestroyAccess);
-if (rc == Success) {
-   CloseDownClient(killclient);
-   /* if an LBX proxy gets killed, isItTimeToYield will be set */
-   if (isItTimeToYield || (client == killclient))
-   {
-   /* force yield and return Success, so that Dispatch()
-* doesn't try to touch client
-*/
-   isItTimeToYield = TRUE;
-   return Success;
-   }
-   return Success;
-}
-else
-   return rc;
+if (rc == Success)
+   CloseDownClientByClient(client, killclient);
+return rc;
 }
 
 int
-- 
1.7.1

___
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] libxfont: An uninitialized pointer causes a crash if pcf header is corrupted.

[Rami Ylimäki]

On 09/06/2011 02:00 PM, Olli Vertanen wrote:

If pcfReadTOC() or pcfGetProperties() fail in the beginning
of execution of pcfReadFont(), function tries to free an
uninitialized pointer (isStringProp) when bailing out.

The pointer gets now initialized correctly.

Signed-off-by: Olli Vertanenolli.verta...@symbio.com
---
  src/bitmap/pcfread.c |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/bitmap/pcfread.c b/src/bitmap/pcfread.c
index 182144a..de4e93d 100644
--- a/src/bitmap/pcfread.c
+++ b/src/bitmap/pcfread.c
@@ -408,6 +408,8 @@ pcfReadFont(FontPtr pFont, FontFilePtr file,

  pFont-info.nprops = 0;
  pFont-info.props = 0;
+pFont-info.isStringProp=0;
+
  if (!(tables = pcfReadTOC(file,ntables)))
goto Bail;



Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

___
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: randr: stop immutable output properties from being deleted (v2)

[Rami Ylimäki]

On 08/24/2011 01:26 AM, Luc Verhaegen wrote:

On Wed, Aug 24, 2011 at 12:19:58AM +0200, Luc Verhaegen wrote:

This version of the server side patch now no longer mangles the output
properties list directly. This incurs a slight bit of overhead, in what is
an extremely low frequency path anyway. It also fixes the error thrown in
case the property does not exist for the listed output, to match
QueryOutputProperty.

The new version of the randrproto.txt patch now fixes the two issues daniels
spotted and now fully documents all the errors that RRDeleteOutputProperty
might throw.

Luc Verhaegen.

 From master this applied to the n9's xserver 1.9.5 tree cleanly, and was
built and tested succesfully.

Luc Verhaegen.


Bumping this thread up because I'd like to see these changes committed.

Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

___
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] dri2: Initialize needInvalidate member of DRI2Drawable.

[Rami Ylimäki]

If the client is not behaving correctly and swaps buffers before
getting them, Valgrind will complain about uninitialized memory being
used in DRI2InvalidateDrawable.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
This may not be a huge problem in practice but it's always nice to
initialize memory fully after allocation. I haven't actually tested
this with the latest X server, I have only adapted the patch from
1.9.5.

 hw/xfree86/dri2/dri2.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index af3bcae..68eda89 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -179,6 +179,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
 pPriv-last_swap_ust = 0;
 list_init(pPriv-reference_list);
 pPriv-serialNumber = DRI2DrawableSerial(pDraw);
+pPriv-needInvalidate = FALSE;
 
 if (pDraw-type == DRAWABLE_WINDOW) {
pWin = (WindowPtr) pDraw;
-- 
1.7.1

___
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 v2 xserver 1/2] xkb: Introduce helper function to handle similar reallocations.

[Rami Ylimäki]

This is preparation for a memory leak fix and doesn't contain any
functional changes.

Note that two variables are generally used for reallocation and
clearing of arrays: geom-sz_elems (reallocation) and geom-num_elems
(clearing). The interface of XkbGeomRealloc is deliberately kept
simple and it only accepts geom-sz_elems as argument, because that is
needed to determine whether the array needs to be resized. When the
array is cleared, we just assume that either geom-sz_elems and
geom-num_elems are synchronized to be equal or that unused elements
are cleared whenever geom-num_elems is set to be less than
geom-sz_elems without reallocation.

Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 xkb/XKBGAlloc.c |   64 +
 xkb/xkbUtils.c  |  121 ---
 xkb/xkbgeom.h   |   20 +
 3 files changed, 111 insertions(+), 94 deletions(-)

diff --git a/xkb/XKBGAlloc.c b/xkb/XKBGAlloc.c
index d1adea3..dbe0069 100644
--- a/xkb/XKBGAlloc.c
+++ b/xkb/XKBGAlloc.c
@@ -445,6 +445,57 @@ XkbFreeGeometry(XkbGeometryPtr geom,unsigned which,Bool 
freeMap)
 
 /******/
 
+/**
+ * Resize and clear an XKB geometry item array. The array size may
+ * grow or shrink unlike in _XkbGeomAlloc.
+ *
+ * @param buffer[in,out]  buffer to reallocate and clear
+ * @param szItems[in] currently allocated item count for buffer
+ * @param nrItems[in] required item count for buffer
+ * @param itemSize[in]size of a single item in buffer
+ * @param clearance[in]   items to clear after reallocation
+ *
+ * @see _XkbGeomAlloc
+ *
+ * @return TRUE if reallocation succeeded. Otherwise FALSE is returned
+ * and contents of buffer aren't touched.
+ */
+Bool
+XkbGeomRealloc(void **buffer, int szItems, int nrItems,
+   int itemSize, XkbGeomClearance clearance)
+{
+void *items;
+int clearBegin;
+/* Check validity of arguments. */
+if (!buffer)
+return FALSE;
+items = *buffer;
+if (!((items  (szItems  0)) || (!items  !szItems)))
+return FALSE;
+/* Check if there is need to resize. */
+if (nrItems != szItems)
+if (!(items = realloc(items, nrItems * itemSize)))
+return FALSE;
+/* Clear specified items to zero. */
+switch (clearance)
+{
+case XKB_GEOM_CLEAR_EXCESS:
+clearBegin = szItems;
+break;
+case XKB_GEOM_CLEAR_ALL:
+clearBegin = 0;
+break;
+case XKB_GEOM_CLEAR_NONE:
+default:
+clearBegin = nrItems;
+break;
+}
+if (items  (clearBegin  nrItems))
+memset((char *)items + (clearBegin * itemSize), 0, (nrItems - 
clearBegin) * itemSize);
+*buffer = items;
+return TRUE;
+}
+
 static Status
 _XkbGeomAlloc( void ** old,
unsigned short *num,
@@ -461,18 +512,15 @@ _XkbGeomAlloc(void ** old,
return Success;
 
 *total= (*num)+num_new;
-if ((*old)!=NULL)
-(*old)= realloc((*old),(*total)*sz_elem);
-else (*old)= calloc((*total),sz_elem);
-if ((*old)==NULL) {
+
+if (!XkbGeomRealloc(old, *num, *total, sz_elem, XKB_GEOM_CLEAR_EXCESS))
+{
+   free(*old);
+   (*old)= NULL;
*total= *num= 0;
return BadAlloc;
 }
 
-if (*num0) {
-   char *tmp= (char *)(*old);
-   memset(tmp[sz_elem*(*num)], 0, (num_new*sz_elem));
-}
 return Success;
 }
 
diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
index 14dc784..7fea194 100644
--- a/xkb/xkbUtils.c
+++ b/xkb/xkbUtils.c
@@ -1395,42 +1395,26 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 
 /* properties */
 if (src-geom-num_properties) {
-if (src-geom-num_properties != dst-geom-sz_properties) {
-/* If we've got more properties in the destination than
- * the source, run through and free all the excess ones
- * first. */
-if (src-geom-num_properties  dst-geom-sz_properties) {
-for (i = src-geom-num_properties,
- dprop = dst-geom-properties + i;
- i  dst-geom-num_properties;
- i++, dprop++) {
-free(dprop-name);
-free(dprop-value);
-}
+/* If we've got more properties in the destination than
+ * the source, run through and free all the excess ones
+ * first. */
+if (src-geom-num_properties  dst-geom-sz_properties) {
+for (i = src-geom-num_properties, dprop = 
dst-geom-properties + i;
+ i  dst-geom-num_properties;
+ i++, dprop++) {
+free(dprop-name);
+free(dprop-value);
 }
-
-if (dst-geom-sz_properties

[PATCH v2 xserver 2/2] xkb: Prevent leaking of XKB geometry information on copy.

[Rami Ylimäki]

Currently shapes, sections and doodads may leak on copy.

Reviewed-by: Peter Hutterer peter.hutte...@who-t.net
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 xkb/xkbUtils.c |8 +++-
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
index 7fea194..323731f 100644
--- a/xkb/xkbUtils.c
+++ b/xkb/xkbUtils.c
@@ -1538,10 +1538,10 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 }
 
 if (src-geom-num_shapes) {
-tmp = calloc(src-geom-num_shapes, sizeof(XkbShapeRec));
-if (!tmp)
+/* Reallocate and clear all items. */
+if (!XkbGeomRealloc((void **)dst-geom-shapes, 
dst-geom-sz_shapes, src-geom-num_shapes,
+sizeof(XkbShapeRec), XKB_GEOM_CLEAR_ALL))
 return FALSE;
-dst-geom-shapes = tmp;
 
 for (i = 0, sshape = src-geom-shapes, dshape = dst-geom-shapes;
  i  src-geom-num_shapes;
@@ -1658,7 +1658,6 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 }
 
 dst-geom-num_sections = 0;
-dst-geom-sections = NULL;
 }
 
 if (src-geom-num_sections) {
@@ -1768,7 +1767,6 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 }
 }
 dst-geom-num_doodads = 0;
-dst-geom-doodads = NULL;
 }
 
 if (src-geom-num_doodads) {
-- 
1.6.3.3

___
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 v2 xserver 0/2] Fix for XKB geometry leaks when mapping changes

[Rami Ylimäki]

v1
--
Running setxkbmap repeatedly to change the keyboard mapping may lead
to leaking of geometry related XKB data. The setxkbmap command does
XkbGetKbdByName request, which copies keymap using XkbCopyKeymap, but
the copying doesn't always release allocated pointers in _XkbCopyGeom.

v2
--
This version contains fixes based on review comments.

Previously the _XkbGeomRealloc function was introduced as a static
function that was only used in _XkbCopyGeom. The function has now been
made public and is also used by _XkbGeomAlloc.

Peter Hutterer:

- Use enum to define how array is cleared.
- Don't use assertions, just fail the geometry when there are
  problems.

Rami Ylimäki (2):
  xkb: Introduce helper function to handle similar reallocations.
  xkb: Prevent leaking of XKB geometry information on copy.

 xkb/XKBGAlloc.c |   64 ---
 xkb/xkbUtils.c  |  129 --
 xkb/xkbgeom.h   |   20 +
 3 files changed, 114 insertions(+), 99 deletions(-)

___
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: [Xcb] [PATCH libXau] Avoid heap corruption when calling XauFileName from multiple threads.

[Rami Ylimäki]

On 03/28/2011 09:14 PM, Mark Kettenis wrote:

Date: Mon, 28 Mar 2011 10:21:06 -0700
From: Alan Coopersmithalan.coopersm...@oracle.com


This commit fixes only the heap corruption and sporadic crashes. It's
still possible that XauFileName returns a badly formed filename string
if called from multiple threads. For example, changing contents of
HOME environment variable could make the returned string to be
malformed. However, there shouldn't be crashes.

Perhaps we just need to define a new function to do this, that doesn't
use a static global variable of any sort, and declare that multi-threaded
code needs to use it.

That does make more sense than adding hacks to limit the potential
damage.  The current interface simply is inherently thread-unsafe.


I was actually thinking about implementing XauGetFileName as an 
alternative for XauFileName, that would return dynamically allocated 
filename string and require it to be freed by callers. However, the real 
problem is that getenv has to be called at some point to get XAUTHORITY 
or HOME, and that can't be done safely inside a library in any portable way.


The only way to solve the problem in a thread safe manner would be to 
pass copies of XAUTHORITY and HOME to Xlib/Xcb when creating a new 
connection. Alternatively the semantics of connection creation should be 
changed so that environment variables would be completely ignored. Both 
of these options aren't very good from backward compatibility point of view.


We chose to fix the common case and make connection creation safe as 
long as environment is not modified at the same time. We didn't 
implement a new function, because if we offer a new function for 
multithreaded clients, it should be also protected against environment 
changes. That would again require a lot of changes in users of libXau. 
In other words, the fix avoids a real problem and while not perfect, 
it's good enough. 
[http://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good :)]


Could you give your opinion about the environment problem? If a new 
function is created for multithreaded clients, what should be done about 
reading XAUTHORITY and HOME in a safe manner? Alternatively, do you 
think that protecting against environment changes is not that important 
and fixing the allocation problems are enough?


-- Rami

___
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 libXau] Avoid heap corruption when calling XauFileName from multiple threads.

[Rami Ylimäki]

An XCB test application will always crash because of heap corruption
if it's running xcb_connect/xcb_disconnect continuously from multiple
threads. The problem can also happen in real applications if
XOpenDisplay and xcb_connect are called simultaneously.

This commit fixes only the heap corruption and sporadic crashes. It's
still possible that XauFileName returns a badly formed filename string
if called from multiple threads. For example, changing contents of
HOME environment variable could make the returned string to be
malformed. However, there shouldn't be crashes.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Signed-off-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 AuFileName.c |   34 +-
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/AuFileName.c b/AuFileName.c
index b21b048..fdf03e0 100644
--- a/AuFileName.c
+++ b/AuFileName.c
@@ -33,14 +33,14 @@ in this Software without prior written authorization from 
The Open Group.
 #include X11/Xauth.h
 #include X11/Xos.h
 #include stdlib.h
+#include limits.h
 
 char *
 XauFileName (void)
 {
 const char *slashDotXauthority = /.Xauthority;
 char*name;
-static char*buf;
-static int bsize;
+static charbuf[PATH_MAX] = {0};
 #ifdef WIN32
 chardir[128];
 #endif
@@ -48,6 +48,9 @@ XauFileName (void)
 
 if ((name = getenv (XAUTHORITY)))
return name;
+/* This function assumes that the HOME environment variable
+ * doesn't change between multiple threads. If it does change, the
+ * returned string may not contain a valid file name. */
 name = getenv (HOME);
 if (!name) {
 #ifdef WIN32
@@ -60,16 +63,21 @@ XauFileName (void)
 #endif
return NULL;
 }
-size = strlen (name) + strlen(slashDotXauthority[1]) + 2;
-if (size  bsize) {
-   if (buf)
-   free (buf);
-   buf = malloc ((unsigned) size);
-   if (!buf)
-   return NULL;
-   bsize = size;
-}
-strcpy (buf, name);
-strcat (buf, slashDotXauthority + (name[1] == '\0' ? 1 : 0));
+/* Length of home directory location. */
+size = strlen (name);
+/* Check whether /home/user + /.Xauthority + \0 fits into
+ * buf. */
+if (size + strlen (slashDotXauthority) + 1  sizeof(buf))
+return NULL;
+/* Construct /home/user/.Xauthority. Avoid writing null
+ * character when the first part of the string is copied. That
+ * could temporarily split the contents of buf and cause a
+ * problem with multiple threads. We are assuming here that the
+ * contents of name stay the same if this function is called
+ * simultaneously from multiple threads.
+ */
+memcpy (buf, name, size);
+strcpy (buf + size, slashDotXauthority + ((size = 1) ? 1 : 0));
+
 return buf;
 }
-- 
1.6.3.3

___
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 v2 libXau] Avoid heap corruption when calling XauFileName from multiple threads.

[Rami Ylimäki]

An XCB test application will always crash because of heap corruption
if it's running xcb_connect/xcb_disconnect continuously from multiple
threads. The problem can also happen in real applications if
XOpenDisplay and xcb_connect are called simultaneously.

This commit fixes only the heap corruption and sporadic crashes. It's
still possible that XauFileName returns a badly formed filename string
if called from multiple threads. For example, changing contents of
HOME environment variable could make the returned string to be
malformed. However, there shouldn't be crashes.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Signed-off-by: Erkki Seppälä erkki.sepp...@vincit.fi
---

Minor fixes have been made based on review comments.

v2
--

Mark Kettenis:

- Don't initialize static array explicitly to zero.

  This change is OK to us.

- Use strcat as it was used before for clarity.

  Unfortunately this is not possible, because strcat assumes that
  there is a null character in the middle of the string. We want to
  avoid at all cost of splitting the result string temporarily, so
  that multithreaded users wouldn't suffer. We did replace the memcpy
  of v1 with strncpy though.

Arnaud Fontaine:

- Don't use PATH_MAX as it's not always defined.

  We have chosen to use it and define it as 1024 if it doesn't come
  from a header. This is in line with similar Xorg code that is using
  PATH_MAX. The reason for this is that we need to allocate space for
  the returned filename string at compile time and some constant is
  needed.

  It could be possible to allocate the required filename string
  dynamically. However, doing this would require linking against
  pthreads to prevent allocation race conditions and possibly using
  GCC constructors/destructors to avoid memory leaks if libXau is
  loaded dynamically. It's much simpler to just use static
  storage. Also enabling pthreads wouldn't improve things much, as the
  getenv() of XauFileName can't really be protected against putenv()
  happening elsewhere, the function would still remain unsafe if
  environment would be changed.

 AuFileName.c |   47 ++-
 1 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/AuFileName.c b/AuFileName.c
index b21b048..9d28011 100644
--- a/AuFileName.c
+++ b/AuFileName.c
@@ -33,14 +33,27 @@ in this Software without prior written authorization from 
The Open Group.
 #include X11/Xauth.h
 #include X11/Xos.h
 #include stdlib.h
+#include limits.h /* PATH_MAX */
+
+/* PATH_MAX is not necessarily defined. However, the specification of
+ * XauFileName requires us to use statically allocated space for the
+ * filename string. Therefore we need some kind of estimate for a
+ * maximum path length.
+ */
+#ifndef PATH_MAX
+#ifdef MAXPATHLEN
+#define PATH_MAX MAXPATHLEN
+#else /* MAXPATHLEN */
+#define PATH_MAX 1024
+#endif /* MAXPATHLEN */
+#endif /* PATH_MAX */
 
 char *
 XauFileName (void)
 {
 const char *slashDotXauthority = /.Xauthority;
 char*name;
-static char*buf;
-static int bsize;
+static charbuf[PATH_MAX];
 #ifdef WIN32
 chardir[128];
 #endif
@@ -48,6 +61,9 @@ XauFileName (void)
 
 if ((name = getenv (XAUTHORITY)))
return name;
+/* This function assumes that the HOME environment variable
+ * doesn't change between multiple threads. If it does change, the
+ * returned string may not contain a valid file name. */
 name = getenv (HOME);
 if (!name) {
 #ifdef WIN32
@@ -60,16 +76,21 @@ XauFileName (void)
 #endif
return NULL;
 }
-size = strlen (name) + strlen(slashDotXauthority[1]) + 2;
-if (size  bsize) {
-   if (buf)
-   free (buf);
-   buf = malloc ((unsigned) size);
-   if (!buf)
-   return NULL;
-   bsize = size;
-}
-strcpy (buf, name);
-strcat (buf, slashDotXauthority + (name[1] == '\0' ? 1 : 0));
+/* Length of home directory location. */
+size = strlen (name);
+/* Check whether /home/user + /.Xauthority + \0 fits into
+ * buf. */
+if (size + strlen (slashDotXauthority) + 1  sizeof(buf))
+return NULL;
+/* Construct /home/user/.Xauthority. Avoid writing null
+ * character when the first part of the string is copied. That
+ * could temporarily split the contents of buf and cause a
+ * problem with multiple threads. We are assuming here that the
+ * contents of name stay the same if this function is called
+ * simultaneously from multiple threads.
+ */
+strncpy (buf, name, size);
+strcpy (buf + size, slashDotXauthority + ((size = 1) ? 1 : 0));
+
 return buf;
 }
-- 
1.6.3.3

___
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 0/2] Fix for XKB geometry leaks when mapping changes

[Rami Ylimäki]

Running setxkbmap repeatedly to change the keyboard mapping may lead
to leaking of geometry related XKB data. The setxkbmap command does
XkbGetKbdByName request, which copies keymap using XkbCopyKeymap, but
the copying doesn't always release allocated pointers in _XkbCopyGeom.

Rami Ylimäki (2):
  xkb: Introduce helper function to handle similar reallocations.
  xkb: Prevent leaking of XKB geometry information on copy.

 xkb/xkbUtils.c |  159 ++--
 1 files changed, 74 insertions(+), 85 deletions(-)

___
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 1/2] xkb: Introduce helper function to handle similar reallocations.

[Rami Ylimäki]

This is preparation for a memory leak fix and doesn't contain any
functional changes.

Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 xkb/xkbUtils.c |  153 ++--
 1 files changed, 71 insertions(+), 82 deletions(-)

diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
index 3a56bea..f4c8a6e 100644
--- a/xkb/xkbUtils.c
+++ b/xkb/xkbUtils.c
@@ -1375,6 +1375,38 @@ _XkbCopyCompat(XkbDescPtr src, XkbDescPtr dst)
 return TRUE;
 }
 
+/**
+ * Resize and clear an XKB geometry item array.
+ *
+ * @param items[in/out]   array to reallocate and clear
+ * @param szItems[in] currently allocated item count for items
+ * @param nrItems[in] required item count for items
+ * @param clearBegin[in]  first item to clear after allocation
+ * @param clearEnd[in]one past last item to clear after allocation
+ * @param itemSize[in]size of a single item in items
+ *
+ * @see _XkbGeomAlloc, which is very similar
+ *
+ * @return reallocated array
+ */
+static void *
+_XkbGeomRealloc(void *items, int szItems, int nrItems,
+int clearBegin, int clearEnd, int itemSize)
+{
+/* Check if there is need to resize. */
+if (nrItems != szItems)
+{
+assert((items  (szItems  0)) || (!items  !szItems));
+items = realloc(items, nrItems * itemSize);
+}
+/* Clear specified items to zero. */
+clearEnd = (clearEnd  nrItems) ? clearEnd : nrItems;
+if (items  (clearBegin  clearEnd))
+memset((char *)items + (clearBegin * itemSize), 0, (clearEnd - 
clearBegin) * itemSize);
+
+return items;
+}
+
 static Bool
 _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 {
@@ -1398,42 +1430,28 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 
 /* properties */
 if (src-geom-num_properties) {
-if (src-geom-num_properties != dst-geom-sz_properties) {
-/* If we've got more properties in the destination than
- * the source, run through and free all the excess ones
- * first. */
-if (src-geom-num_properties  dst-geom-sz_properties) {
-for (i = src-geom-num_properties,
- dprop = dst-geom-properties + i;
- i  dst-geom-num_properties;
- i++, dprop++) {
-free(dprop-name);
-free(dprop-value);
-}
+/* If we've got more properties in the destination than
+ * the source, run through and free all the excess ones
+ * first. */
+if (src-geom-num_properties  dst-geom-sz_properties) {
+for (i = src-geom-num_properties, dprop = 
dst-geom-properties + i;
+ i  dst-geom-num_properties;
+ i++, dprop++) {
+free(dprop-name);
+free(dprop-value);
 }
-
-if (dst-geom-sz_properties)
-tmp = realloc(dst-geom-properties,
-   src-geom-num_properties *
-sizeof(XkbPropertyRec));
-else
-tmp = malloc(src-geom-num_properties *
-  sizeof(XkbPropertyRec));
-if (!tmp)
-return FALSE;
-dst-geom-properties = tmp;
 }
 
+/* Reallocate and clear all new items if the buffer grows. */
+tmp = _XkbGeomRealloc(dst-geom-properties, 
dst-geom-sz_properties, src-geom-num_properties,
+  dst-geom-num_properties, 
src-geom-num_properties, sizeof(XkbPropertyRec));
+if (!tmp)
+return FALSE;
+dst-geom-properties = tmp;
 /* We don't set num_properties as we need it to try and avoid
  * too much reallocing. */
 dst-geom-sz_properties = src-geom-num_properties;
 
-if (dst-geom-sz_properties  dst-geom-num_properties) {
-memset(dst-geom-properties + dst-geom-num_properties, 0,
-  (dst-geom-sz_properties - dst-geom-num_properties) *
-  sizeof(XkbPropertyRec));
-}
-
 for (i = 0,
   sprop = src-geom-properties,
   dprop = dst-geom-properties;
@@ -1482,36 +1500,22 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 
 /* colors */
 if (src-geom-num_colors) {
-if (src-geom-num_colors != dst-geom-sz_colors) {
-if (src-geom-num_colors  dst-geom-sz_colors) {
-for (i = src-geom-num_colors,
- dcolor = dst-geom-colors + i;
- i  dst-geom-num_colors;
- i++, dcolor++) {
-free(dcolor-spec

[PATCH xserver 2/2] xkb: Prevent leaking of XKB geometry information on copy.

[Rami Ylimäki]

Currently shapes, sections and doodads may leak on copy.

Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 xkb/xkbUtils.c |6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
index f4c8a6e..e7681de 100644
--- a/xkb/xkbUtils.c
+++ b/xkb/xkbUtils.c
@@ -1577,7 +1577,9 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 }
 
 if (src-geom-num_shapes) {
-tmp = calloc(src-geom-num_shapes, sizeof(XkbShapeRec));
+/* Reallocate and clear all items. */
+tmp = _XkbGeomRealloc(dst-geom-shapes, dst-geom-sz_shapes, 
src-geom-num_shapes,
+  0, src-geom-num_shapes, 
sizeof(XkbShapeRec));
 if (!tmp)
 return FALSE;
 dst-geom-shapes = tmp;
@@ -1697,7 +1699,6 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 }
 
 dst-geom-num_sections = 0;
-dst-geom-sections = NULL;
 }
 
 if (src-geom-num_sections) {
@@ -1809,7 +1810,6 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
 }
 }
 dst-geom-num_doodads = 0;
-dst-geom-doodads = NULL;
 }
 
 if (src-geom-num_doodads) {
-- 
1.6.3.3

___
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: X*IfEvent, generic events and threads

[Rami Ylimäki]

Hi,

The documentation for predicate function used with X*IfEvent states that 
calling back to Xlib from the predicate is forbidden. This is 
inconvenient with generic events, because calling XGetEventData is 
mandatory for filtering them. However, calling XGetEventData in the 
predicate is forbidden by the documentation and also causes a deadlock 
with multithreaded clients.


Would it make sense to create new variants like X*IfGenericEvent using 
an extended predicate function that would also receive a pointer to the 
generic event data? That way event filtering could be done for generic 
events as well without violating the specification for predicates. 
Current situation is a bit annoying, because only legacy events can be 
filtered reliably.


-- Rami

___
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] xkb: Release XKB component names when compiling keymap.

[Rami Ylimäki]

Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
I tested this with X server 1.9. However, I did cherry pick enough
patches from master so that I could make the patch apply cleanly on
X server master also.

 include/xkbsrv.h |5 +
 xkb/XKBAlloc.c   |   19 +++
 xkb/ddxLoad.c|   22 +++---
 xkb/xkb.c|   11 +--
 4 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/include/xkbsrv.h b/include/xkbsrv.h
index 9f1507e..34ef574 100644
--- a/include/xkbsrv.h
+++ b/include/xkbsrv.h
@@ -447,6 +447,11 @@ extern _X_EXPORT void  XkbFreeKeyboard(
Bool/* freeDesc */
 );
 
+extern _X_EXPORT void  XkbFreeComponentNames(
+   XkbComponentNamesPtr/* names */,
+   Bool/* freeNames */
+);
+
 extern _X_EXPORT  void XkbSetActionKeyMods(
XkbDescPtr  /* xkb */,
XkbAction * /* act */,
diff --git a/xkb/XKBAlloc.c b/xkb/XKBAlloc.c
index c52e091..87f8a5a 100644
--- a/xkb/XKBAlloc.c
+++ b/xkb/XKBAlloc.c
@@ -335,3 +335,22 @@ XkbFreeKeyboard(XkbDescPtr xkb,unsigned which,Bool freeAll)
free(xkb);
 return;
 }
+
+
+/******/
+
+void
+XkbFreeComponentNames(XkbComponentNamesPtr names, Bool freeNames)
+{
+if (names)
+{
+free(names-keycodes);
+free(names-types);
+free(names-compat);
+free(names-symbols);
+free(names-geometry);
+memset(names, 0, sizeof(XkbComponentNamesRec));
+}
+if (freeNames)
+free(names);
+}
diff --git a/xkb/ddxLoad.c b/xkb/ddxLoad.c
index 00ac034..501104e 100644
--- a/xkb/ddxLoad.c
+++ b/xkb/ddxLoad.c
@@ -449,23 +449,23 @@ XkbRMLVOtoKcCGST(DeviceIntPtr dev, XkbRMLVOSet *rmlvo, 
XkbComponentNamesPtr kccg
 static XkbDescPtr
 XkbCompileKeymapForDevice(DeviceIntPtr dev, XkbRMLVOSet *rmlvo, int need)
 {
-XkbDescPtr xkb;
+XkbDescPtr xkb = NULL;
 unsigned int provided;
-XkbComponentNamesRec kccgst;
+XkbComponentNamesRec kccgst = {0};
 char name[PATH_MAX];
 
-if (!XkbRMLVOtoKcCGST(dev, rmlvo, kccgst))
-return NULL;
-
-provided = XkbDDXLoadKeymapByNames(dev, kccgst, XkmAllIndicesMask, need,
-   xkb, name, PATH_MAX);
-if ((need  provided) != need) {
-if (xkb) {
-XkbFreeKeyboard(xkb, 0, TRUE);
-xkb = NULL;
+if (XkbRMLVOtoKcCGST(dev, rmlvo, kccgst)) {
+provided = XkbDDXLoadKeymapByNames(dev, kccgst, XkmAllIndicesMask, 
need,
+   xkb, name, PATH_MAX);
+if ((need  provided) != need) {
+if (xkb) {
+XkbFreeKeyboard(xkb, 0, TRUE);
+xkb = NULL;
+}
 }
 }
 
+XkbFreeComponentNames(kccgst, FALSE);
 return xkb;
 }
 
diff --git a/xkb/xkb.c b/xkb/xkb.c
index 39dbab4..bd0512a 100644
--- a/xkb/xkb.c
+++ b/xkb/xkb.c
@@ -5898,16 +5898,7 @@ ProcXkbGetKbdByName(ClientPtr client)
XkbFreeKeyboard(new,XkbAllComponentsMask,TRUE);
new= NULL;
 }
-free(names.keycodes);
-names.keycodes = NULL;
-free(names.types);
-names.types = NULL;
-free(names.compat);
-names.compat = NULL;
-free(names.symbols);
-names.symbols = NULL;
-free(names.geometry);
-names.geometry = NULL;
+XkbFreeComponentNames(names, FALSE);
 return Success;
 }
 
-- 
1.6.3.3

___
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] xkb: Initialize pad bytes sent in replies of geometry requests.

[Rami Ylimäki]

Valgrind complains about uninitialized data being written to clients.

Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 xkb/xkb.c |   12 +---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/xkb/xkb.c b/xkb/xkb.c
index 8d61e01..39dbab4 100644
--- a/xkb/xkb.c
+++ b/xkb/xkb.c
@@ -4305,7 +4305,7 @@ ProcXkbSetNames(ClientPtr client)
 static char *
 XkbWriteCountedString(char *wire,char *str,Bool swap)
 {
-CARD16 len,*pLen;
+CARD16 len,*pLen, paddedLen;
 
 if (!str)
 return wire;
@@ -4317,8 +4317,9 @@ XkbWriteCountedString(char *wire,char *str,Bool swap)
register int n;
swaps(pLen,n);
 }
-memcpy(wire[2],str,len);
-wire+= ((2+len+3)/4)*4;
+paddedLen= pad_to_int32(sizeof(len)+len)-sizeof(len);
+strncpy(wire[sizeof(len)],str,paddedLen);
+wire+= sizeof(len)+paddedLen;
 return wire;
 }
 
@@ -4429,6 +4430,7 @@ xkbShapeWireDesc *shapeWire;
if (shape-approx!=NULL)
 shapeWire-approxNdx= XkbOutlineIndex(shape,shape-approx);
else shapeWire-approxNdx= XkbNoShape;
+   shapeWire-pad= 0;
if (swap) {
register int n;
swapl(shapeWire-name,n);
@@ -4441,6 +4443,7 @@ xkbShapeWireDesc *shapeWire;
olWire= (xkbOutlineWireDesc *)wire;
olWire-nPoints= ol-num_points;
olWire-cornerRadius= ol-corner_radius;
+   olWire-pad= 0;
wire= (char *)olWire[1];
ptWire= (xkbPointWireDesc *)wire;
for (p=0,pt=ol-points;pol-num_points;p++,pt++) {
@@ -4554,6 +4557,8 @@ xkbOverlayWireDesc *  olWire;
olWire= (xkbOverlayWireDesc *)wire;
olWire-name= ol-name;
olWire-nRows= ol-num_rows;
+   olWire-pad1= 0;
+   olWire-pad2= 0;
if (swap) {
register int n;
swapl(olWire-name,n);
@@ -4566,6 +4571,7 @@ xkbOverlayWireDesc *  olWire;
rowWire= (xkbOverlayRowWireDesc *)wire;
rowWire-rowUnder= row-row_under;
rowWire-nKeys= row-num_keys;
+   rowWire-pad1= 0;
wire= (char *)rowWire[1];
for (k=0,key=row-keys;krow-num_keys;k++,key++) {
xkbOverlayKeyWireDesc * keyWire;
-- 
1.6.3.3

___
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 v2 evdev 0/3] Memory leak fixes.

[Rami Ylimäki]

v2:

XKB options are now released with XkbFreeRMLVOSet.

The first version of these patches assigned a dynamically allocated
device filename to a const pointer. Calling free with this pointer
causes a warning, but this warning was suppressed by casting its
constness away when calling free. The warning could be alternatively
avoided by not using a const pointers in the first place.

In my opinion, using const pointer in this case is completely
fine. Removing the constness just because the string is dynamically
allocated isn't necessary. Therefore I prefer keeping the cast instead
of removing the constness of device filename string.

However, patch 3 of this series can be squashed with the 2nd patch to
remove the constness. Otherwise the 3rd patch could be just dropped.

Rami Ylimäki (3):
  Release leaked XKB options on input device disconnect.
  Release leaked device identifier on input device disconnect.
  Remove constness of device filename to avoid warning when freed.

 src/evdev.c |   19 +--
 src/evdev.h |2 +-
 2 files changed, 18 insertions(+), 3 deletions(-)

___
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 v2 evdev 1/3] Release leaked XKB options on input device disconnect.

[Rami Ylimäki]

Currently the XKB options duplicated in EvdevAddKeyClass are never
released. For example, connecting and disconnecting a bluetooth
keyboard repeatedly causes a steadily growing memory leak.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 src/evdev.c |   16 ++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index 45873c1..18b7246 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -2162,6 +2162,18 @@ EvdevOpenDevice(InputInfoPtr pInfo)
 return Success;
 }
 
+static void
+EvdevUnInit(InputDriverPtr drv, InputInfoPtr pInfo, int flags)
+{
+EvdevPtr pEvdev = pInfo ? pInfo-private : NULL;
+if (pEvdev)
+{
+/* Release strings allocated in EvdevAddKeyClass. */
+XkbFreeRMLVOSet(pEvdev-rmlvo, FALSE);
+}
+xf86DeleteInput(pInfo, flags);
+}
+
 #if GET_ABI_MAJOR(ABI_XINPUT_VERSION)  12
 static int NewEvdevPreInit(InputDriverPtr, InputInfoPtr, int);
 
@@ -2198,7 +2210,7 @@ EvdevPreInit(InputDriverPtr drv, IDevPtr dev, int flags)
 }
 
 
-xf86DeleteInput(pInfo, 0);
+EvdevUnInit(drv, pInfo, 0);
 return NULL;
 }
 
@@ -2277,7 +2289,7 @@ _X_EXPORT InputDriverRec EVDEV = {
 evdev,
 NULL,
 EvdevPreInit,
-NULL,
+EvdevUnInit,
 NULL,
 #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) = 12
 evdevDefaults
-- 
1.6.3.3

___
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 v2 evdev 2/3] Release leaked device identifier on input device disconnect.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 src/evdev.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index 18b7246..db62375 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -2170,6 +2170,9 @@ EvdevUnInit(InputDriverPtr drv, InputInfoPtr pInfo, int 
flags)
 {
 /* Release strings allocated in EvdevAddKeyClass. */
 XkbFreeRMLVOSet(pEvdev-rmlvo, FALSE);
+/* Release string allocated in EvdevOpenDevice. */
+free((void *)pEvdev-device); /* (const char *) */
+pEvdev-device = NULL;
 }
 xf86DeleteInput(pInfo, flags);
 }
-- 
1.6.3.3

___
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 v2 evdev 3/3] Remove constness of device filename to avoid warning when freed.

[Rami Ylimäki]

A warning from free() can be avoided by casting the constness away
from its argument pointer or by not declaring the pointer as const in
the first place.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 src/evdev.c |2 +-
 src/evdev.h |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index db62375..474d20d 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -2171,7 +2171,7 @@ EvdevUnInit(InputDriverPtr drv, InputInfoPtr pInfo, int 
flags)
 /* Release strings allocated in EvdevAddKeyClass. */
 XkbFreeRMLVOSet(pEvdev-rmlvo, FALSE);
 /* Release string allocated in EvdevOpenDevice. */
-free((void *)pEvdev-device); /* (const char *) */
+free(pEvdev-device);
 pEvdev-device = NULL;
 }
 xf86DeleteInput(pInfo, flags);
diff --git a/src/evdev.h b/src/evdev.h
index f640fdd..80e4580 100644
--- a/src/evdev.h
+++ b/src/evdev.h
@@ -116,7 +116,7 @@ typedef struct {
 } EventQueueRec, *EventQueuePtr;
 
 typedef struct {
-const char *device;
+char *device;
 int grabDevice; /* grab the event device? */
 
 int num_vals;   /* number of valuators */
-- 
1.6.3.3

___
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: Spinning in _XReply

[Rami Ylimäki]

On 03/04/2011 01:20 AM, Karl Tomlinson wrote:

On Thu, 24 Feb 2011 12:27:28 +1300, xm...@karlt.net wrote:


Rami Ylimäki writes:


On 02/22/2011 11:26 PM, xm...@karlt.net wrote:

Although wrong, our code has been working well enough for us to get
useful information.  If someone is able to point out a reason why
this now works less successfully than previously, then that would
bump up the priority at our end.

By superficially looking at the Xlib code, the behavior in older
Xlib version (1.3.3) is different depending on whether Xlib is
configured --with-xcb or --without-xcb. When Xcb is disabled,
error handler is called with display unlocked and _XReply seems to
be prepared to handle requests from error handler. With Xcb, which
is the default now with Xlib 1.4, the display remains locked
during error handling and nested requests aren't allowed.

It looks like the display locks are no-ops unless XInitThreads is
called or when built with --enable-checked-locks.

http://lists.x.org/archives/xorg-devel/2011-February/019533.html
does seem to indicate another potential issue (breaking from the
loop only when req == current).  I guess that hasn't bitten us
because we don't return to the first _XReply.

That seems to have been the situation until
http://cgit.freedesktop.org/xorg/lib/libX11/commit/?id=933aee1d5c53b0cc7d608011a29188b594c8d70b

I see that we now spin even without XInitThreads.
That is version 1.3.4 --with-xcb.

I'm expecting to fix this as part of
https://bugzilla.mozilla.org/show_bug.cgi?id=576933
(Fall out from GDK not expecting input devices to disappear.)

I'm planning to open another connection to get the extension
codes.  Although there's no promise we'll get the right codes in
the future, it shouldn't hang.


Opening another connection could be perhaps avoided by using Xcb 
directly for the XListExtensions and XQueryExtension requests. The Xlib 
Display pointer can be converted to an Xcb connection with 
XGetXCBConnection and that could in turn be used to execute the same 
requests with Xcb. You could ask the codes from the correct connection 
without worrying about internal Xlib problems.


If XInitThreads is used, the call to XSynchronize in the error handler 
is also problematic and I don't think it's possible to determine whether 
the Xlib connection was synchronous or not from the error handler with 
enabled locks.


-- Rami

___
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: Spinning in _XReply

[Rami Ylimäki]

On 03/04/2011 10:32 AM, Rami Ylimäki wrote:

On 03/04/2011 01:20 AM, Karl Tomlinson wrote:

On Thu, 24 Feb 2011 12:27:28 +1300, xm...@karlt.net wrote:


Rami Ylimäki writes:


On 02/22/2011 11:26 PM, xm...@karlt.net wrote:

Although wrong, our code has been working well enough for us to get
useful information.  If someone is able to point out a reason why
this now works less successfully than previously, then that would
bump up the priority at our end.

By superficially looking at the Xlib code, the behavior in older
Xlib version (1.3.3) is different depending on whether Xlib is
configured --with-xcb or --without-xcb. When Xcb is disabled,
error handler is called with display unlocked and _XReply seems to
be prepared to handle requests from error handler. With Xcb, which
is the default now with Xlib 1.4, the display remains locked
during error handling and nested requests aren't allowed.

It looks like the display locks are no-ops unless XInitThreads is
called or when built with --enable-checked-locks.

http://lists.x.org/archives/xorg-devel/2011-February/019533.html
does seem to indicate another potential issue (breaking from the
loop only when req == current).  I guess that hasn't bitten us
because we don't return to the first _XReply.

That seems to have been the situation until
http://cgit.freedesktop.org/xorg/lib/libX11/commit/?id=933aee1d5c53b0cc7d608011a29188b594c8d70b 



I see that we now spin even without XInitThreads.
That is version 1.3.4 --with-xcb.

I'm expecting to fix this as part of
https://bugzilla.mozilla.org/show_bug.cgi?id=576933
(Fall out from GDK not expecting input devices to disappear.)

I'm planning to open another connection to get the extension
codes.  Although there's no promise we'll get the right codes in
the future, it shouldn't hang.


Opening another connection could be perhaps avoided by using Xcb 
directly for the XListExtensions and XQueryExtension requests. The 
Xlib Display pointer can be converted to an Xcb connection with 
XGetXCBConnection and that could in turn be used to execute the same 
requests with Xcb. You could ask the codes from the correct connection 
without worrying about internal Xlib problems.


If XInitThreads is used, the call to XSynchronize in the error handler 
is also problematic and I don't think it's possible to determine 
whether the Xlib connection was synchronous or not from the error 
handler with enabled locks.


-- Rami



Also, a word of warning about asking extension list with Xcb. The latest 
unreleased version of Xcb has a regression related to iteration over 
extensions:
https://bugs.freedesktop.org/show_bug.cgi?id=34037. However, this should 
be easily fixable, when someone finds time to do it, and you shouldn't 
be affected by yet unreleased versions.


-- Rami

___
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 evdev 2/2] Release leaked device identifier on input device disconnect.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 src/evdev.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index a77be9e..165400f 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1839,6 +1839,9 @@ EvdevUnInit(InputDriverPtr drv, InputInfoPtr pInfo, int 
flags)
 free(pEvdev-rmlvo.variant);
 free(pEvdev-rmlvo.options);
 memset(pEvdev-rmlvo, 0, sizeof(pEvdev-rmlvo));
+/* Release string allocated in EvdevOpenDevice. */
+free((void *)pEvdev-device); /* (const char *) */
+pEvdev-device = NULL;
 }
 xf86DeleteInput(pInfo, flags);
 }
-- 
1.6.3.3

___
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 evdev 1/2] Release leaked XKB options on input device disconnect.

[Rami Ylimäki]

Currently the XKB options duplicated in EvdevAddKeyClass are never
released. For example, connecting and disconnecting a bluetooth
keyboard repeatedly causes a steadily growing memory leak.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 src/evdev.c |   19 ++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index bb30655..a77be9e 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1826,6 +1826,23 @@ EvdevOpenDevice(InputInfoPtr pInfo)
 return Success;
 }
 
+static void
+EvdevUnInit(InputDriverPtr drv, InputInfoPtr pInfo, int flags)
+{
+EvdevPtr pEvdev = pInfo ? pInfo-private : NULL;
+if (pEvdev)
+{
+/* Release strings allocated in EvdevAddKeyClass. */
+free(pEvdev-rmlvo.rules);
+free(pEvdev-rmlvo.model);
+free(pEvdev-rmlvo.layout);
+free(pEvdev-rmlvo.variant);
+free(pEvdev-rmlvo.options);
+memset(pEvdev-rmlvo, 0, sizeof(pEvdev-rmlvo));
+}
+xf86DeleteInput(pInfo, flags);
+}
+
 static int
 EvdevPreInit(InputDriverPtr drv, InputInfoPtr pInfo, int flags)
 {
@@ -1897,7 +1914,7 @@ _X_EXPORT InputDriverRec EVDEV = {
 evdev,
 NULL,
 EvdevPreInit,
-NULL,
+EvdevUnInit,
 NULL,
 evdevDefaults
 };
-- 
1.6.3.3

___
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 1/3] xkb: Ensure that XKB device private won't leak on device disconnect.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 xkb/xkbActions.c |   11 ++-
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/xkb/xkbActions.c b/xkb/xkbActions.c
index c020444..a29aeb7 100644
--- a/xkb/xkbActions.c
+++ b/xkb/xkbActions.c
@@ -67,20 +67,13 @@ xkbUnwrapProc(DeviceIntPtr device, DeviceHandleProc proc,
 Bool
 XkbInitPrivates(void)
 {
-return dixRegisterPrivateKey(xkbDevicePrivateKeyRec, PRIVATE_DEVICE, 0);
+return dixRegisterPrivateKey(xkbDevicePrivateKeyRec, PRIVATE_DEVICE, 
sizeof(xkbDeviceInfoRec));
 }
 
 void
 XkbSetExtension(DeviceIntPtr device, ProcessInputProc proc)
 {
-xkbDeviceInfoPtr xkbPrivPtr;
-
-xkbPrivPtr = (xkbDeviceInfoPtr) calloc(1, sizeof(xkbDeviceInfoRec));
-if (!xkbPrivPtr)
-   return;
-xkbPrivPtr-unwrapProc = NULL;
-
-dixSetPrivate(device-devPrivates, xkbDevicePrivateKey, xkbPrivPtr);
+xkbDeviceInfoPtr xkbPrivPtr = XKBDEVICEINFO(device);
 WRAP_PROCESS_INPUT_PROC(device, xkbPrivPtr, proc, xkbUnwrapProc);
 }
 
-- 
1.6.3.3

___
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 2/3] dix: Release input device config info when the device disconnects.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 dix/devices.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/dix/devices.c b/dix/devices.c
index 9aaadc4..5ea6ad6 100644
--- a/dix/devices.c
+++ b/dix/devices.c
@@ -936,6 +936,8 @@ CloseDevice(DeviceIntPtr dev)
 }
 
 free(dev-deviceGrab.sync.event);
+free(dev-config_info); /* Allocated in xf86ActivateDevice. */
+dev-config_info = NULL;
 dixFreeObjectWithPrivates(dev, PRIVATE_DEVICE);
 }
 
-- 
1.6.3.3

___
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 3/3] config: Ensure that stolen option list elements are released.

[Rami Ylimäki]

NewInputDeviceRequest steals the contents of option list elements but
doesn't use the elements themselves for anything. Therefore the list
elements need to be released always.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 config/hal.c  |6 +++---
 config/udev.c |6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/config/hal.c b/config/hal.c
index 6e2850c..5ade2f6 100644
--- a/config/hal.c
+++ b/config/hal.c
@@ -393,10 +393,10 @@ unwind:
 free(driver);
 free(name);
 free(config_info);
-while (!dev  (tmpo = options)) {
+while ((tmpo = options)) {
 options = tmpo-next;
-free(tmpo-key);
-free(tmpo-value);
+free(tmpo-key);/* NULL if dev != NULL */
+free(tmpo-value);  /* NULL if dev != NULL */
 free(tmpo);
 }
 
diff --git a/config/udev.c b/config/udev.c
index cb38276..a2f5710 100644
--- a/config/udev.c
+++ b/config/udev.c
@@ -191,10 +191,10 @@ device_added(struct udev_device *udev_device)
 
  unwind:
 free(config_info);
-while (!dev  (tmpo = options)) {
+while ((tmpo = options)) {
 options = tmpo-next;
-free(tmpo-key);
-free(tmpo-value);
+free(tmpo-key);/* NULL if dev != NULL */
+free(tmpo-value);  /* NULL if dev != NULL */
 free(tmpo);
 }
 
-- 
1.6.3.3

___
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: Spinning in _XReply

[Rami Ylimäki]

On 02/23/2011 04:35 PM, Michal Suchanek wrote:

X server seems to preserve the extension codes until the server is
regenerated and therefore all connections should use the same codes.


Is that guaranteed by some protocol or does it just happen to work as
the requests in error handler did?



This is a good point and I can't find a definite answer for it.

Documentation of C.2.1 XInitExtension from Xlib manual says that the 
Xlib specific extension number in XExtCodes is connection specific, 
which makes sense, because the number is managed internally in Xlib and 
not asked from server. Then C.5.11 Deriving the Correct Extension 
Opcode says that a separate XExtCodes should be maintained for each 
connection.


So I think that one shouldn't assume that opcodes stay the same, even 
though it seems to be so in practice.


-- Rami

___
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: Spinning in _XReply

[Rami Ylimäki]

On 02/25/2011 02:48 PM, Daniel Stone wrote:

On Fri, Feb 25, 2011 at 02:08:38PM +0200, Rami Ylimäki wrote:

On 02/23/2011 04:35 PM, Michal Suchanek wrote:

X server seems to preserve the extension codes until the server is
regenerated and therefore all connections should use the same codes.


Is that guaranteed by some protocol or does it just happen to work as
the requests in error handler did?

This is a good point and I can't find a definite answer for it.

Documentation of C.2.1 XInitExtension from Xlib manual says that
the Xlib specific extension number in XExtCodes is connection
specific, which makes sense, because the number is managed
internally in Xlib and not asked from server. Then C.5.11 Deriving
the Correct Extension Opcode says that a separate XExtCodes should
be maintained for each connection.

So I think that one shouldn't assume that opcodes stay the same,
even though it seems to be so in practice.

Er, really? How would sending requests then work without an event from
the server telling you to round-trip and get the extension list again?



Looks like I removed a little bit too much context when answering. The 
original question was about whether one can trust that extension opcodes 
are the same between connections. The answer seems to be that there is 
no guarantee of that, because XExtCodes structure is connection 
specific. Of course the opcodes should stay the same over the lifetime 
of a connection but nobody guarantees, that XInitExtension returns the 
same opcodes for different connections, even though in practice it's so.


-- Rami

___
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: Spinning in _XReply

[Rami Ylimäki]

On 02/22/2011 11:26 PM, xm...@karlt.net wrote:

Although wrong, our code has been working well enough for us to get
useful information.  If someone is able to point out a reason why
this now works less successfully than previously, then that would
bump up the priority at our end.


By superficially looking at the Xlib code, the behavior in older Xlib 
version (1.3.3) is different depending on whether Xlib is configured 
--with-xcb or --without-xcb. When Xcb is disabled, error handler is 
called with display unlocked and _XReply seems to be prepared to handle 
requests from error handler. With Xcb, which is the default now with 
Xlib 1.4, the display remains locked during error handling and nested 
requests aren't allowed.


Even the older _XReply contains a comment indicating that doing requests 
from an error handler wasn't considered good behavior even though it 
happened to work previously:


/* Pull out the serial number now, so that (currently illegal) requests
 * generated by an error handler don't confuse us.
 */


The information we want is already on display-ext_procs-codes
but This structure is private to the library.  I'm not aware of
a public API to get this information from Xlib.

I guess we could use a separate connection to query the extension
codes.  Is it reasonable to assume that all connections to the
same display would use the same codes?


X server seems to preserve the extension codes until the server is 
regenerated and therefore all connections should use the same codes.


-- Rami

___
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: Spinning in _XReply

[Rami Ylimäki]

On 02/22/2011 11:14 AM, Julien Cristau wrote:

On Sat, Jan 29, 2011 at 09:54:37 -0800, Jeremy Huddleston wrote:


2) emacs' error handler seems bugged.  Is it legal to call XSync() within the 
error handler?  It certainly seems like it shouldn't.  Did we used to actually 
support this with the xtrans version of libX11?


Not as far as I can tell.  From XSetErrorHandler(3):

Because this condition is not assumed to be fatal, it is acceptable for
your error handler to return; the returned value is ignored.  However,
the error handler should not call any functions (directly or indi‐
rectly) on the display that will generate protocol requests or that
will look for input events.

That's not a new requirement.


I've been running Xlib with thread-safety checks 
(http://lists.x.org/archives/xorg-devel/2011-February/018979.html) and 
have found some problems even in single threaded clients. For example, 
one client was doing X requests from a signal handler, which is not 
allowed. Today I found out that even Mozilla code is suffering from this 
problem 
(http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsX11ErrorHandler.cpp).


Adding Karl into CC. It looks like nsX11ErrorHandler.cpp is also calling 
Xlib functions resulting in protocol requests, which is not allowed.


-- Rami

___
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 v3 0/3] Allow detection of missing XInitThreads calls.

[Rami Ylimäki]

v1:
The change initialized threads by default. Such feature allows unsafe
X clients to work when they shouldn't.

v2:
Fixes problems in v1 and makes multithreaded X clients abort with an
error message if they enter a critical section concurrently before
XInitThreads is called. After the first XInitThreads call from the
client, situation is normalized.

The difference is that with reconfigured Xlib, unsafe X clients abort
in a predictable place instead of crashing in random locations.

v3:
Fixes dependencies between libX11 and libX11-xcb during
build process and also in the built libraries. These libraries have
been now made more independent.
___
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 v3 x11proto-core 1/3] Add xthread_trylock for detecting unsafe multihreaded Xlib usage.

[Rami Ylimäki]

Xlib can be configured with --enable-checked-locks option to check if
it's called from multiple threads even though XInitThreads hasn't been
called first. This detection is currently implemented with
phtread_trylock.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Signed-off-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 Xthreads.h |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Xthreads.h b/Xthreads.h
index 3d44208..15cbb2d 100644
--- a/Xthreads.h
+++ b/Xthreads.h
@@ -230,6 +230,7 @@ typedef pthread_mutex_t xmutex_rec;
 #  define xthread_set_specific(k,v) pthread_setspecific(k,v)
 #  define xmutex_clear(m) pthread_mutex_destroy(m)
 #  define xmutex_lock(m) pthread_mutex_lock(m)
+#  define xmutex_trylock(m) pthread_mutex_trylock(m)
 #  define xmutex_unlock(m) pthread_mutex_unlock(m)
 #  ifndef XPRE_STANDARD_API
 #   define xthread_key_create(kp,d) pthread_key_create(kp,d)
@@ -290,6 +291,9 @@ typedef xmutex_rec *xmutex_t;
 # ifndef xmutex_free
 #  define xmutex_free(m) xfree((char *)m)
 # endif
+# ifndef xmutex_trylock /* generate compiler error */
+#  define xmutex_trylock(m) xmutex_trylock not defined()
+# endif
 # ifndef xthread_have_id
 #  define xthread_have_id(id) id
 # endif
-- 
1.6.3.3

___
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 v3 2/3] config: Move Xlib-xcb to its own subdirectory.

[Rami Ylimäki]

This change makes it easier to use different compiler and linker flags
for libX11 and libX11-xcb. Also libX11-xcb doesn't depend on libX11
anymore, because it's not using any of its symbols.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 configure.ac|1 +
 src/Makefile.am |9 +++--
 src/Xxcbint.h   |   46 --
 src/x11_xcb.c   |   15 ---
 src/xcb/Makefile.am |   26 ++
 src/xcb/Xxcbint.h   |   46 ++
 src/xcb/x11_xcb.c   |   15 +++
 7 files changed, 91 insertions(+), 67 deletions(-)
 delete mode 100644 src/Xxcbint.h
 delete mode 100644 src/x11_xcb.c
 create mode 100644 src/xcb/Makefile.am
 create mode 100644 src/xcb/Xxcbint.h
 create mode 100644 src/xcb/x11_xcb.c

diff --git a/configure.ac b/configure.ac
index 40d032d..a69735b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -448,6 +448,7 @@ AC_OUTPUT([Makefile
man/xkb/Makefile
src/Makefile
src/util/Makefile
+  src/xcb/Makefile
   src/xcms/Makefile
   src/xlibi18n/Makefile
   modules/Makefile
diff --git a/src/Makefile.am b/src/Makefile.am
index 8b0953c..05c7663 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1,9 +1,9 @@
 if XKB
 XKB_SUBDIRS = xkb
 endif
-SUBDIRS = util xcms xlibi18n $(XKB_SUBDIRS)
+SUBDIRS = util xcms xlibi18n $(XKB_SUBDIRS) . xcb
 
-lib_LTLIBRARIES = libX11.la libX11-xcb.la
+lib_LTLIBRARIES = libX11.la
 
 BUILT_SOURCES=ks_tables.h
 CLEANFILES=ks_tables.h ks_tables_h
@@ -13,6 +13,7 @@ AM_CPPFLAGS= \
-I$(top_srcdir)/include/X11 \
-I$(top_builddir)/include \
-I$(top_builddir)/include/X11 \
+   -I$(top_srcdir)/src/xcb \
-I$(top_srcdir)/src/xcms \
-I$(top_srcdir)/src/xkb \
-I$(top_srcdir)/src/xlibi18n \
@@ -349,10 +350,6 @@ EXTRA_DIST = \
udcInf.c \
UIThrStubs.c 
 
-libX11_xcb_la_SOURCES = x11_xcb.c Xxcbint.h
-libX11_xcb_la_LDFLAGS = -version-number 1:0:0 -no-undefined
-libX11_xcb_la_LIBADD = libX11.la
-
 #
 # Figure out which sub-libraries to link into Xlib
 #
diff --git a/src/Xxcbint.h b/src/Xxcbint.h
deleted file mode 100644
index 1fa1a4d..000
--- a/src/Xxcbint.h
+++ /dev/null
@@ -1,46 +0,0 @@
-/* Copyright (C) 2003-2006 Jamey Sharp, Josh Triplett
- * This file is licensed under the MIT license. See the file COPYING. */
-
-#ifndef XXCBINT_H
-#define XXCBINT_H
-
-#include assert.h
-#include stdint.h
-#include X11/Xlibint.h
-#include X11/Xlib-xcb.h
-#include locking.h
-
-#define XLIB_SEQUENCE_COMPARE(a,op,b)  (((long) (a) - (long) (b)) op 0)
-
-typedef struct PendingRequest PendingRequest;
-struct PendingRequest {
-   PendingRequest *next;
-   unsigned long sequence;
-   unsigned reply_waiter;
-};
-
-typedef struct _X11XCBPrivate {
-   xcb_connection_t *connection;
-   PendingRequest *pending_requests;
-   PendingRequest *pending_requests_tail;
-   xcb_generic_event_t *next_event;
-   char *real_bufmax;
-   char *reply_data;
-   int reply_length;
-   int reply_consumed;
-   uint64_t last_flushed;
-   enum XEventQueueOwner event_owner;
-   XID next_xid;
-
-   /* handle simultaneous threads waiting for responses */
-   xcondition_t event_notify;
-   int event_waiter;
-   xcondition_t reply_notify;
-} _X11XCBPrivate;
-
-/* xcb_disp.c */
-
-int _XConnectXCB(Display *dpy, _Xconst char *display, int *screenp);
-void _XFreeX11XCBStructure(Display *dpy);
-
-#endif /* XXCBINT_H */
diff --git a/src/x11_xcb.c b/src/x11_xcb.c
deleted file mode 100644
index 3ddf403..000
--- a/src/x11_xcb.c
+++ /dev/null
@@ -1,15 +0,0 @@
-/* Copyright (C) 2003,2006 Jamey Sharp, Josh Triplett
- * This file is licensed under the MIT license. See the file COPYING. */
-
-#include Xlibint.h
-#include Xxcbint.h
-
-xcb_connection_t *XGetXCBConnection(Display *dpy)
-{
-   return dpy-xcb-connection;
-}
-
-void XSetEventQueueOwner(Display *dpy, enum XEventQueueOwner owner)
-{
-   dpy-xcb-event_owner = owner;
-}
diff --git a/src/xcb/Makefile.am b/src/xcb/Makefile.am
new file mode 100644
index 000..5c68639
--- /dev/null
+++ b/src/xcb/Makefile.am
@@ -0,0 +1,26 @@
+AM_CPPFLAGS= \
+   -I$(top_srcdir)/include/X11 \
+   -I$(top_builddir)/include/X11 \
+   -I$(top_srcdir)/src \
+   -D_BSD_SOURCE
+
+AM_CFLAGS= \
+   $(X11_CFLAGS) \
+   $(XMALLOC_ZERO_CFLAGS) \
+   $(CWARNFLAGS)
+
+lib_LTLIBRARIES = libX11-xcb.la
+
+libX11_xcb_la_SOURCES = x11_xcb.c Xxcbint.h
+libX11_xcb_la_LDFLAGS = -version-number 1:0:0 -no-undefined
+
+if LINT
+# Check source code with tools like lint  sparse
+
+ALL_LINT_FLAGS=$(LINT_FLAGS) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) \
+   $(AM_CPPFLAGS) $(CPPFLAGS) $(AM_CFLAGS)
+
+lint:
+   $(LINT) $(ALL_LINT_FLAGS) $(libX11_xcb_la_SOURCES)
+
+endif LINT
diff --git a/src/xcb/Xxcbint.h b/src/xcb/Xxcbint.h
new file mode 100644
index

[PATCH v3 libX11 3/3] config: Add --enable-checked-locks to catch missing XInitThreads calls.

[Rami Ylimäki]

This change makes it possible to detect missing XInitThreads calls in
X clients. If a client hasn't called XInitThreads and enters a
critical section in Xlib concurrently, the client will be aborted with
a message informing about the missing call.

Enabling thread safety by default causes a minor performance
regression for single threaded X clients. However, the performance
regression is only noticeable in the worst case situation. In
realistic use cases the overhead from unnecessary locking is difficult
to see.

Relative x11perf results showing the amount of regression for worst
case and some common operations are shown below.

 unlocked/locked   Operation
- 
   1.001   500-pixel line
   1.000   Copy 500x500 from window to window
   2.609   X protocol NoOperation

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 configure.ac|   37 +
 src/Makefile.am |8 
 src/locking.c   |   55 ++-
 3 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index a69735b..46152d1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -270,6 +270,38 @@ AC_ARG_ENABLE(xthreads,
 [Disable Xlib support for Multithreading]),
   [xthreads=$enableval],[xthreads=yes])
 
+# Determine whether the user wants to check for unsafe usage of Xlib
+# from multiple threads.
+AC_ARG_ENABLE(
+checked_locks,
+AS_HELP_STRING(
+[--enable-checked-locks],
+[Check if multithreaded clients have missed calling 
XInitThreads]),
+[checked_locks=$enableval],
+[checked_locks=no])
+
+# Check if XInitThreads can be called by default on library
+# initialization. The following conditions must be true for that.
+#
+# - Option --enable-xthreads is given.
+# - Option --enable-checked-locks is given.
+# - Compiler is GCC.
+# - GCC supports contructors.
+if test x$xthreads = xyes  test x$checked_locks = xyes  test x$GCC = 
xyes ; then
+   # Set warnings as errors so that GCC fails if the attribute is not
+   # supported.
+   old_CFLAGS=$CFLAGS
+   CFLAGS=-Werror
+   # Check if GCC recognizes constructor attribute.
+   AC_COMPILE_IFELSE(
+   [ void __attribute__((constructor)) test(void) {}; ],
+   checked_locks=yes,
+   checked_locks=no)
+   # Restore flags.
+   CFLAGS=$old_CFLAGS
+fi
+AM_CONDITIONAL(XTHREADS_CHECKED_LOCKS, [test x$checked_locks = xyes])
+
 AC_CHECK_LIB(c, getpwuid_r, [mtsafeapi=yes], [mtsafeapi=no])
 
 case x$xthreads in
@@ -279,6 +311,10 @@ xyes)
then
AC_DEFINE(XUSE_MTSAFE_API,1,[Whether libX11 needs to use MT safe API's])
fi
+   if test x$checked_locks = xyes
+   then
+   AC_DEFINE(XTHREADS_CHECKED_LOCKS,1,[Whether multithreaded clients have 
missed calling XInitThreads])
+   fi
;;
 *)
;;
@@ -481,6 +517,7 @@ echo  Loadable xcursor library support:
$XLIB_LOADABLE_XCURSOR
 echo  Threading support:   $xthreads
 echo  Use Threads safe API:$mtsafeapi
 echo  Threads stubs in libX11: $thrstubs
+echo  Missing XInitThreads calls are detected: $checked_locks
 echo  XCMS:$XCMS
 echo  Internationalization support:$XLOCALE
 echo  XF86BigFont support: $XF86BIGFONT
diff --git a/src/Makefile.am b/src/Makefile.am
index 05c7663..dfc07c6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -366,12 +366,20 @@ if XKB
 USE_XKB_LIBS = $(XKB_LIBS)
 endif
 
+if XTHREADS_CHECKED_LOCKS
+USE_XTHREAD_CFLAGS = $(XTHREAD_CFLAGS)
+USE_XTHREAD_LIBS = $(XTHREADLIB)
+endif XTHREADS_CHECKED_LOCKS
+
+AM_CFLAGS += $(USE_XTHREAD_CFLAGS)
+
 libX11_la_LDFLAGS = -version-number 6:3:0 -no-undefined
 
 libX11_la_LIBADD = \
$(USE_I18N_LIBS) \
$(USE_XCMS_LIBS) \
$(USE_XKB_LIBS) \
+   $(USE_XTHREAD_LIBS) \
$(X11_LIBS)
 
 preprocess: $(patsubst %.c,%.ii,$(libX11_la_SOURCES))
diff --git a/src/locking.c b/src/locking.c
index 4f9a40f..aa310d6 100644
--- a/src/locking.c
+++ b/src/locking.c
@@ -47,7 +47,7 @@ in this Software without prior written authorization from The 
Open Group.
 
 #include Xprivate.h
 #include locking.h
-#ifdef XTHREADS_WARN
+#if defined(XTHREADS_WARN) || defined(XTHREADS_CHECKED_LOCKS)
 #include stdio.h /* for warn/debug stuff */
 #endif
 
@@ -99,12 +99,43 @@ static xthread_t _Xthread_self(void)
 static LockInfoRec global_lock;
 static LockInfoRec i18n_lock;
 
+#ifdef XTHREADS_CHECKED_LOCKS
+/** User hasn't called XInitThreads yet and we wan't to detect whether
+ *  Xlib is used incorrectly from multiple threads. */
+static Bool check_locks = True;
+#endif /* XTHREADS_CHECKED_LOCKS */
+
+/**
+ * Normally this function will just lock the parameter mutex. However

Re: [PATCH v3 2/3] config: Move Xlib-xcb to its own subdirectory.

[Rami Ylimäki]

2011/2/4 Gaetan Nadon mems...@videotron.ca

You will need to rebase, file has changed since.


Sure.


 -SUBDIRS = util xcms xlibi18n $(XKB_SUBDIRS)
 +SUBDIRS = util xcms xlibi18n $(XKB_SUBDIRS) . xcb

  Can you explain why libX11 must be built before xcb subdir?
 The commit text says it does not use any symbols from libX11.


The dependency removal was the last thing that I did. Of course I then
forgot to fix this line.

 BUILT_SOURCES=ks_tables.h
  CLEANFILES=ks_tables.h ks_tables_h
 @@ -13,6 +13,7 @@ AM_CPPFLAGS= \
   -I$(top_srcdir)/include/X11 \
   -I$(top_builddir)/include \
   -I$(top_builddir)/include/X11 \
 + -I$(top_srcdir)/src/xcb \
   -I$(top_srcdir)/src/xcms \
   -I$(top_srcdir)/src/xkb \
   -I$(top_srcdir)/src/xlibi18n \


I noticed that I can get rid of this change by just not moving the Xxcbint.h
header file under xcb. The makefile under xcb already has a line to find the
header from parent directory into xcb/x11_xcb.c. It seems that leaving
Xxcbint.h where it was makes more sense.

+AM_CPPFLAGS= \
 + -I$(top_srcdir)/include/X11 \
 + -I$(top_builddir)/include/X11 \
 + -I$(top_srcdir)/src \
 + -D_BSD_SOURCE

  _BSD_SOURCE is not required due to AC_USE_SYSTEM_EXTENSIONS.
 Notice the duplication in the gcc command


Ok.


  +libX11_xcb_la_SOURCES = x11_xcb.c Xxcbint.h
 +libX11_xcb_la_LDFLAGS = -version-number 1:0:0 -no-undefined

  AM_LDFLAGS would do just fine. Now that each target is separated no need
 to use per-target flags


Ack.



 Thanks!

 Reviewed-by: Gaetan Nadon mems...@videotron.ca


Thanks, I'll include your corrections and tag to a pull request.

-- Rami
___
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 v2 0/2] Allow detection of missing XInitThreads calls.

[Rami Ylimäki]

Previous version of the change initialized threads by default. Such
feature allows unsafe X clients to work when they shouldn't.

This version makes multithreaded X clients abort with an error message
if they enter a critical section concurrently before XInitThreads is
called. After the first XInitThreads call from the client, situation
is normalized.

The difference is that with reconfigured Xlib, unsafe X client abort
in a predictable place instead of crashing in random locations.
___
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 v2 x11proto-core 1/2] Add xthread_trylock for detecting unsafe multihreaded Xlib usage.

[Rami Ylimäki]

Xlib can be configured with --enable-checked-locks option to check if
it's called from multiple threads even though XInitThreads hasn't been
called first. This detection is currently implemented with
phtread_trylock.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Signed-off-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 Xthreads.h |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Xthreads.h b/Xthreads.h
index 3d44208..15cbb2d 100644
--- a/Xthreads.h
+++ b/Xthreads.h
@@ -230,6 +230,7 @@ typedef pthread_mutex_t xmutex_rec;
 #  define xthread_set_specific(k,v) pthread_setspecific(k,v)
 #  define xmutex_clear(m) pthread_mutex_destroy(m)
 #  define xmutex_lock(m) pthread_mutex_lock(m)
+#  define xmutex_trylock(m) pthread_mutex_trylock(m)
 #  define xmutex_unlock(m) pthread_mutex_unlock(m)
 #  ifndef XPRE_STANDARD_API
 #   define xthread_key_create(kp,d) pthread_key_create(kp,d)
@@ -290,6 +291,9 @@ typedef xmutex_rec *xmutex_t;
 # ifndef xmutex_free
 #  define xmutex_free(m) xfree((char *)m)
 # endif
+# ifndef xmutex_trylock /* generate compiler error */
+#  define xmutex_trylock(m) xmutex_trylock not defined()
+# endif
 # ifndef xthread_have_id
 #  define xthread_have_id(id) id
 # endif
-- 
1.6.3.3

___
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 v2 libX11 2/2] config: Add --enable-checked-locks to catch missing XInitThreads calls.

[Rami Ylimäki]

This change makes it possible to detect missing XInitThreads calls in
X clients. If a client hasn't called XInitThreads and enters a
critical section in Xlib concurrently, the client will be aborted with
a message informing about the missing call.

Enabling thread safety by default causes a minor performance
regression for single threaded X clients. However, the performance
regression is only noticeable in the worst case situation. In
realistic use cases the overhead from unnecessary locking is difficult
to see.

Relative x11perf results showing the amount of regression for worst
case and some common operations are shown below.

 unlocked/locked   Operation
- 
   1.001   500-pixel line
   1.000   Copy 500x500 from window to window
   2.609   X protocol NoOperation

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Erkki Seppälä erkki.sepp...@vincit.fi
---
 configure.ac|   37 +
 src/Makefile.am |5 +
 src/locking.c   |   55 ++-
 3 files changed, 92 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 40d032d..c5b000d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -270,6 +270,38 @@ AC_ARG_ENABLE(xthreads,
 [Disable Xlib support for Multithreading]),
   [xthreads=$enableval],[xthreads=yes])
 
+dnl Determine whether the user wants to check for unsafe usage of Xlib
+dnl from multiple threads.
+AC_ARG_ENABLE(
+checked_locks,
+AC_HELP_STRING(
+[--enable-checked-locks],
+[Check if multithreaded clients have missed calling 
XInitThreads]),
+[checked_locks=$enableval],
+[checked_locks=no])
+
+dnl Check if XInitThreads can be called by default on library
+dnl initialization. The following conditions must be true for that.
+dnl
+dnl - Option --enable-xthreads is given.
+dnl - Option --enable-checked-locks is given.
+dnl - Compiler is GCC.
+dnl - GCC supports contructors.
+if test x$xthreads = xyes  test x$checked_locks = xyes  test x$GCC = 
xyes ; then
+   dnl Set warnings as errors so that GCC fails if the attribute is
+   dnl not supported.
+   old_CFLAGS=$CFLAGS
+   CFLAGS=-Werror
+   dnl Check if GCC recognizes constructor attribute.
+   AC_COMPILE_IFELSE(
+   [ void __attribute__((constructor)) test(void) {}; ],
+   checked_locks=yes,
+   checked_locks=no)
+   dnl Restore flags.
+   CFLAGS=$old_CFLAGS
+fi
+AM_CONDITIONAL(XTHREADS_CHECKED_LOCKS, [test x$checked_locks = xyes])
+
 AC_CHECK_LIB(c, getpwuid_r, [mtsafeapi=yes], [mtsafeapi=no])
 
 case x$xthreads in
@@ -279,6 +311,10 @@ xyes)
then
AC_DEFINE(XUSE_MTSAFE_API,1,[Whether libX11 needs to use MT safe API's])
fi
+   if test x$checked_locks = xyes
+   then
+   AC_DEFINE(XTHREADS_CHECKED_LOCKS,1,[Whether multithreaded clients have 
missed calling XInitThreads])
+   fi
;;
 *)
;;
@@ -480,6 +516,7 @@ echo  Loadable xcursor library support:
$XLIB_LOADABLE_XCURSOR
 echo  Threading support:   $xthreads
 echo  Use Threads safe API:$mtsafeapi
 echo  Threads stubs in libX11: $thrstubs
+echo  Missing XInitThreads calls are detected: $checked_locks
 echo  XCMS:$XCMS
 echo  Internationalization support:$XLOCALE
 echo  XF86BigFont support: $XF86BIGFONT
diff --git a/src/Makefile.am b/src/Makefile.am
index 8b0953c..097496a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -377,6 +377,11 @@ libX11_la_LIBADD = \
$(USE_XKB_LIBS) \
$(X11_LIBS)
 
+if XTHREADS_CHECKED_LOCKS
+libX11_la_CFLAGS = $(AM_CFLAGS) $(XTHREAD_FLAGS)
+libX11_la_LIBADD += $(XTHREADLIB)
+endif XTHREADS_CHECKED_LOCKS
+
 preprocess: $(patsubst %.c,%.ii,$(libX11_la_SOURCES))
 .c.ii:
$(COMPILE) -E -o $@ `test -f '$' || echo '$(srcdir)/'`$
diff --git a/src/locking.c b/src/locking.c
index 4f9a40f..aa310d6 100644
--- a/src/locking.c
+++ b/src/locking.c
@@ -47,7 +47,7 @@ in this Software without prior written authorization from The 
Open Group.
 
 #include Xprivate.h
 #include locking.h
-#ifdef XTHREADS_WARN
+#if defined(XTHREADS_WARN) || defined(XTHREADS_CHECKED_LOCKS)
 #include stdio.h /* for warn/debug stuff */
 #endif
 
@@ -99,12 +99,43 @@ static xthread_t _Xthread_self(void)
 static LockInfoRec global_lock;
 static LockInfoRec i18n_lock;
 
+#ifdef XTHREADS_CHECKED_LOCKS
+/** User hasn't called XInitThreads yet and we wan't to detect whether
+ *  Xlib is used incorrectly from multiple threads. */
+static Bool check_locks = True;
+#endif /* XTHREADS_CHECKED_LOCKS */
+
+/**
+ * Normally this function will just lock the parameter mutex. However,
+ * the function will abort if all of the conditions listed below hold.
+ *
+ * 1. Xlib has been configured using --enable

Re: [PATCH v2 x11perf 3/3] Omit benchmarks reading contents of window that is larger than screen.

[Rami Ylimäki]

On 01/31/2011 06:08 PM, Michel Dänzer wrote:

On Mon, 2011-01-31 at 15:48 +0200, Rami Ylimäki wrote:

Signed-off-by: Rami Ylimäkirami.ylim...@vincit.fi
---
  do_blt.c |   10 ++
  1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/do_blt.c b/do_blt.c
index 4be2836..fae9e1d 100644
--- a/do_blt.c
+++ b/do_blt.c
@@ -208,6 +208,16 @@ InitCopyPix(XParms xp, Parms p, int reps)
  int
  InitGetImage(XParms xp, Parms p, int reps)
  {
+int screenWidth = DisplayWidth(xp-d, xp-vinfo.screen);
+int screenHeight = DisplayHeight(xp-d, xp-vinfo.screen);
+
+if ((windowWidth  screenWidth) || (windowHeight  screenHeight))
+{
+printf(Can't read contents of %dx%d window on %dx%d screen, benchmark 
omitted\n,
+   windowWidth, windowHeight, screenWidth, screenHeight);
+return False;
+}
+

This will only catch tests which end up calling InitGetImage.



Do you mean that XGetImage is also called elsewhere in x11perf and the 
patch doesn't touch those locations?


The XGetImage call in InitGetImage is the only one that is done with 
invalid parameters causing a BadMatch error. With this patch, I'm able 
to run x11perf with all tests without terminating prematurely because of 
an X error. I have tested this with a device that has a small screen and 
also with Xephyr using a small window.


-- Rami

___
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 libX11] config: Add an option to initialize threads by default.

[Rami Ylimäki]

On 01/31/2011 09:11 PM, Alan Coopersmith wrote:

On 01/31/11 02:46 AM, Rami Ylimäki wrote:

It's sometimes impossible to know in advance whether an X client is
using Xlib from multiple threads or not. For example, there could be
some generic X client that acts as a plugin container. Plugins could
be loaded to the container at runtime, but the container doesn't know
whether the plugins are using Xlib from a separate thread or not.

I don't know if it's a better answer, but an alternate solution we've
been carrying around in Solaris libX11 since long before I got here is
to allow XInitThreads() to be called by other plugins/libraries even after
the X connection is up and running.I've not checked how well it works
in the current world where half of XlibInt.c was ripped out and replaced
by xcb calls, but the code is available if someone wants to look at it.

Original version:
http://people.freedesktop.org/~alanc/thread-fixes/

Current version:
http://src.opensolaris.org/source/xref/x-cons/xnv-clone/open-src/lib/libX11/1234757.patch
http://src.opensolaris.org/source/xref/x-cons/xnv-clone/open-src/lib/libX11/4010755.patch



Thanks for the links.

The problem that your patches fix is very similar to what I have 
encountered and which the current semantics of XInitThreads are 
completely insufficient to solve. Libraries might find it convenient to 
use threads and/or Xlib to implement some functionality. However, an 
application linking to those libraries has no way of knowing that 
XInitThreads should be called, because the application doesn't 
necessarily even know that some of its libraries are using Xlib.


Therefore it's tempting to change the semantics as you have done. 
However, this is still insufficient to solve the problem in general 
case. It should be possible to guard against multiple simultaneous 
XInitThreads calls and also against XInitThreads calls happening during 
error handler and request/event processing. To make it work, you'd still 
need to have some locks initialized before the first call to 
XInitThreads is done, which pretty much reduces to enabling locking by 
default.


I discussed this with Erkki and we came up with the following solution:

  - add configuration option to initialize locking functions by default
  - the initial locking functions call try_lock() instead of lock() and 
assert on failure
  - the locking functions are initialized as before in first call to 
XInitThreads


So in practice we would be detecting incorrect usage of Xlib from 
multiple threads reliably until XInitThreads is called. This solution 
would at least give the application developer immediate feedback that 
Xlib is used incorrectly so that he wouldn't have to file a bug about a 
random crash in Xlib. What do you think?


-- Rami

___
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 libX11] config: Add an option to initialize threads by default.

[Rami Ylimäki]

It's sometimes impossible to know in advance whether an X client is
using Xlib from multiple threads or not. For example, there could be
some generic X client that acts as a plugin container. Plugins could
be loaded to the container at runtime, but the container doesn't know
whether the plugins are using Xlib from a separate thread or not.

This change makes it possible to guard a system against a missing
XInitThreads call in X clients. One might argue that this is a client
problem and that all X clients should call XInitThreads if it's
possible that they could use Xlib from multiple threads. However,
experience has shown that it's just too easy for developers to
overlook the need for this call.

Enabling thread safety by default causes a performance regression,
because even single threaded applications would start to lock data
structures. However, the performance regression is only noticeable in
a theoretical worst case situation. In realistic use cases the
overhead from unnecessary locking can't be seen. Relative x11perf
results showing the amount of regression for worst case and some
common operations are shown below.

enabled disabled Operation
---  -
  1.00 1.00  Copy 10x10 from window to window
  1.00 1.00  Copy 500x500 from window to window
  1.00 2.68  X protocol NoOperation

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 configure.ac  |   36 
 src/locking.c |   10 ++
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index 40d032d..27ed595 100644
--- a/configure.ac
+++ b/configure.ac
@@ -270,6 +270,37 @@ AC_ARG_ENABLE(xthreads,
 [Disable Xlib support for Multithreading]),
   [xthreads=$enableval],[xthreads=yes])
 
+dnl Check if the user wants XInitThreads to be called by default on
+dnl library initialization.
+AC_ARG_ENABLE(
+init_xthreads,
+AC_HELP_STRING(
+[--enable-init-xthreads],
+[Call XInitThreads on library initialization]),
+[init_xthreads=$enableval],
+[init_xthreads=no])
+
+dnl Check if XInitThreads can be called by default on library
+dnl initialization. The following conditions must be true for that.
+dnl
+dnl - Option --enable-xthreads is given.
+dnl - Option --enable-init-xthreads is given.
+dnl - Compiler is GCC.
+dnl - GCC supports library contructors.
+if test x$xthreads = xyes  test x$init_xthreads = xyes  test x$GCC = 
xyes ; then
+   dnl Set warnings as errors so that GCC fails if the attribute is
+   dnl not supported.
+   old_CFLAGS=$CFLAGS
+   CFLAGS=-Werror
+   dnl Check if GCC recognizes constructor attribute.
+   AC_COMPILE_IFELSE(
+   [ void __attribute__((constructor)) test(void) {}; ],
+   init_xthreads=yes,
+   init_xthreads=no)
+   dnl Restore flags.
+   CFLAGS=$old_CFLAGS
+fi
+
 AC_CHECK_LIB(c, getpwuid_r, [mtsafeapi=yes], [mtsafeapi=no])
 
 case x$xthreads in
@@ -279,6 +310,10 @@ xyes)
then
AC_DEFINE(XUSE_MTSAFE_API,1,[Whether libX11 needs to use MT safe API's])
fi
+   if test x$init_xthreads = xyes
+   then
+   AC_DEFINE(INIT_XTHREADS,1,[Whether XInitThreads is called on library 
initialization])
+   fi
;;
 *)
;;
@@ -480,6 +515,7 @@ echo  Loadable xcursor library support:
$XLIB_LOADABLE_XCURSOR
 echo  Threading support:   $xthreads
 echo  Use Threads safe API:$mtsafeapi
 echo  Threads stubs in libX11: $thrstubs
+echo  Threads initialized by default:  $init_xthreads
 echo  XCMS:$XCMS
 echo  Internationalization support:$XLOCALE
 echo  XF86BigFont support: $XF86BIGFONT
diff --git a/src/locking.c b/src/locking.c
index 4f9a40f..7673a20 100644
--- a/src/locking.c
+++ b/src/locking.c
@@ -615,6 +615,16 @@ Status XInitThreads(void)
 return 1;
 }
 
+#ifdef INIT_XTHREADS
+
+/** Initialize threads by default when Xlib is loaded. */
+static void __attribute__((constructor)) XInitLib(void)
+{
+if (!XInitThreads())
+exit(1);
+}
+
+#endif /* INIT_XTHREADS*/
 #else /* XTHREADS */
 Status XInitThreads(void)
 {
-- 
1.6.3.3

___
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 v2 x11perf 0/3] Window size related modifications.

[Rami Ylimäki]

Two changes are introduced:
- Ignore and skip a benchmark if XGetImage would generate a BadMatch error.
- Allow default test window size to be redefined with extra options.

Rami Ylimäki (3):
  Let window size to be variable instead of constant.
  Let user to override the default window dimensions.
  Omit benchmarks reading contents of window that is larger than
screen.

 do_arcs.c|   12 ++--
 do_blt.c |   46 --
 do_complex.c |8 
 do_lines.c   |   16 
 do_movewin.c |   10 +-
 do_rects.c   |4 ++--
 do_segs.c|   12 ++--
 do_text.c|   14 +++---
 do_traps.c   |   18 +-
 do_tris.c|4 ++--
 do_windows.c |6 +++---
 x11perf.c|   39 +--
 x11perf.h|5 ++---
 13 files changed, 111 insertions(+), 83 deletions(-)

___
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 v2 x11perf 1/3] Let window size to be variable instead of constant.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 do_arcs.c|   12 ++--
 do_blt.c |   36 ++--
 do_complex.c |8 
 do_lines.c   |   16 
 do_movewin.c |   10 +-
 do_rects.c   |4 ++--
 do_segs.c|   12 ++--
 do_text.c|   14 +++---
 do_traps.c   |   18 +-
 do_tris.c|4 ++--
 do_windows.c |6 +++---
 x11perf.c|   20 +++-
 x11perf.h|5 ++---
 13 files changed, 83 insertions(+), 82 deletions(-)

diff --git a/do_arcs.c b/do_arcs.c
index 5c5f1ba..63eda65 100644
--- a/do_arcs.c
+++ b/do_arcs.c
@@ -69,16 +69,16 @@ GenerateCircles(XParms xp, Parms p, Bool partialArcs, Bool 
ddashed)
 
y += size + 1;
rows++;
-   if (y = HEIGHT - size  - half || rows == MAXROWS) {
+   if (y = windowHeight - size  - half || rows == MAXROWS) {
/* Go to next column */
rows = 0;
x += size + 1;
-   if (x = WIDTH - size) {
+   if (x = windowWidth - size) {
yorg++;
-   if (yorg = size + half || yorg = HEIGHT - size - half) {
+   if (yorg = size + half || yorg = windowHeight - size - half) {
yorg = half;
xorg++;
-   if (xorg = size + half || xorg = WIDTH - size - half) {
+   if (xorg = size + half || xorg = windowWidth - size - 
half) {
xorg = half;
}
}
@@ -287,12 +287,12 @@ GenerateEllipses(XParms xp, Parms p, int partialArcs, 
Bool ddashed)
 
y += size + 1;
rows++;
-   if (y = HEIGHT - size - half || rows == MAXROWS) {
+   if (y = windowHeight - size - half || rows == MAXROWS) {
/* Go to next column */
rows = 0;
y = half;
x += size + 1;
-   if (x = WIDTH - size - half) {
+   if (x = windowWidth - size - half) {
x = half;
}
}
diff --git a/do_blt.c b/do_blt.c
index f843ae5..4be2836 100644
--- a/do_blt.c
+++ b/do_blt.c
@@ -41,22 +41,22 @@ InitBltLines(void)
 points[0].x = points[0].y = y = 0;
 for (i = 1; i != NUMPOINTS/2; i++) {
if (i  1) {
-   points[i].x = WIDTH-1;
+   points[i].x = windowWidth-1;
} else {
points[i].x = 0;
}
-   y += HEIGHT / (NUMPOINTS/2);
+   y += windowHeight / (NUMPOINTS/2);
points[i].y = y;
 }
 
 x = 0;
 for (i = NUMPOINTS/2; i!= NUMPOINTS; i++) {
if (i  1) {
-   points[i].y = HEIGHT-1;
+   points[i].y = windowHeight-1;
} else {
points[i].y = 0;
}
-   x += WIDTH / (NUMPOINTS/2);
+   x += windowWidth / (NUMPOINTS/2);
points[i].x = x;
 }
 }
@@ -91,18 +91,18 @@ DoScroll(XParms xp, Parms p, int reps)
XCopyArea(xp-d, xp-w, xp-w, xp-fggc, x, y + delta,
size, size, x, y);
y += size;
-   if (y + size + delta  HEIGHT) {
+   if (y + size + delta  windowHeight) {
yorg += delta;
-   if (yorg = size || yorg + size + delta  HEIGHT) {
+   if (yorg = size || yorg + size + delta  windowHeight) {
yorg = 0;
xorg++;
-   if (xorg = size || xorg + size  WIDTH) {
+   if (xorg = size || xorg + size  windowWidth) {
xorg = 0;
}
}
y = yorg;
x += size;
-   if (x + size  WIDTH) {
+   if (x + size  windowWidth) {
x = xorg;
}
}
@@ -138,8 +138,8 @@ InitCopyLocations(XParms xp, Parms p, int reps)
 xinc = (size  ~3) + 1;
 yinc = xinc + 3;
 
-width = (WIDTH - size)  ~31;
-height = (HEIGHT - size)  ~31;
+width = (windowWidth - size)  ~31;
+height = (windowHeight - size)  ~31;
 
 x1 = 0;
 y1 = 0;
@@ -197,10 +197,10 @@ InitCopyPix(XParms xp, Parms p, int reps)
 (void) InitCopyWin(xp, p, reps);
 
 /* Create pixmap to write stuff into, and initialize it */
-pix = XCreatePixmap(xp-d, xp-w, WIDTH, HEIGHT, xp-vinfo.depth);
+pix = XCreatePixmap(xp-d, xp-w, windowWidth, windowHeight, 
xp-vinfo.depth);
 pixgc = XCreateGC(xp-d, pix, 0, NULL);
 /* need a gc with GXcopy cos pixmaps contain junk on creation. mmm */
-XCopyArea(xp-d, xp-w, pix, pixgc, 0, 0, WIDTH, HEIGHT, 0, 0);
+XCopyArea(xp-d, xp-w, pix, pixgc, 0, 0, windowWidth, windowHeight, 0, 0);
 XFreeGC(xp-d, pixgc);
 return reps;
 }
@@ -211,7 +211,7 @@ InitGetImage(XParms xp, Parms p, int reps)
 (void) InitCopyWin(xp, p, reps);
 
 /* Create image to stuff bits into */
-image = XGetImage(xp-d, xp-w, 0, 0, WIDTH, HEIGHT, xp-planemask,
+image = XGetImage(xp-d, xp-w, 0, 0, windowWidth, windowHeight, 
xp-planemask,
  p-font==NULL?ZPixmap:XYPixmap);
 if(image==NULL

[PATCH v2 x11perf 2/3] Let user to override the default window dimensions.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 x11perf.c |   19 ++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/x11perf.c b/x11perf.c
index 39ab493..29ca1ec 100644
--- a/x11perf.c
+++ b/x11perf.c
@@ -497,6 +497,8 @@ usage(void)
 -rop rop0 rop1 ...  use the given rops to draw (default = GXcopy),
 -pm pm0 pm1 ... use the given planemasks to draw (default = 
~0),
 -depth depthuse a visual with depth planes per pixel,
+-width widthoverride the default width of test window,
+-height height  override the default height of test window,
 -vclass class   the visual class to use (default = root),
 -reps n fix the rep count (default = auto scale),
 -subs s0 s1 ... a list of the number of sub-windows to use,
@@ -1055,6 +1057,20 @@ main(int argc, char *argv[])
 depth = atoi(argv[i]);
 if (depth = 0)
usage ();
+} else if (strcmp(argv[i], -width) == 0) {
+   i++;
+   if (argc = i)
+usage ();
+windowWidth = atoi(argv[i]);
+if (windowWidth = 0)
+   usage ();
+} else if (strcmp(argv[i], -height) == 0) {
+   i++;
+   if (argc = i)
+usage ();
+windowHeight = atoi(argv[i]);
+if (windowHeight = 0)
+   usage ();
 } else if (strcmp(argv[i], -vclass) == 0) {
i++;
if (argc = i)
@@ -1326,7 +1342,8 @@ main(int argc, char *argv[])
 /* Figure out how long to call HardwareSync, so we can adjust for that
in our total elapsed time */
 (void) CalibrateTest(xparms, syncTest, 1, syncTime);
-printf(Sync time adjustment is %6.4f msecs.\n\n, syncTime/1000);
+printf(Sync time adjustment is %6.4f msecs.\n, syncTime/1000);
+printf(Window size is %dx%d.\n\n, windowWidth, windowHeight);
 
 ForEachTest (i) {
int child;
-- 
1.6.3.3

___
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 v2 x11perf 3/3] Omit benchmarks reading contents of window that is larger than screen.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 do_blt.c |   10 ++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/do_blt.c b/do_blt.c
index 4be2836..fae9e1d 100644
--- a/do_blt.c
+++ b/do_blt.c
@@ -208,6 +208,16 @@ InitCopyPix(XParms xp, Parms p, int reps)
 int 
 InitGetImage(XParms xp, Parms p, int reps)
 {
+int screenWidth = DisplayWidth(xp-d, xp-vinfo.screen);
+int screenHeight = DisplayHeight(xp-d, xp-vinfo.screen);
+
+if ((windowWidth  screenWidth) || (windowHeight  screenHeight))
+{
+printf(Can't read contents of %dx%d window on %dx%d screen, benchmark 
omitted\n,
+   windowWidth, windowHeight, screenWidth, screenHeight);
+return False;
+}
+
 (void) InitCopyWin(xp, p, reps);
 
 /* Create image to stuff bits into */
-- 
1.6.3.3

___
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 libX11] config: Add an option to initialize threads by default.

[Rami Ylimäki]

On 01/31/2011 04:14 PM, Tiago Vignatti wrote:

On 01/31/2011 04:06 PM, ext Julien Cristau wrote:

On Mon, Jan 31, 2011 at 15:12:29 +0200, Tiago Vignatti wrote:


On 01/31/2011 01:13 PM, ext Julien Cristau wrote:

On Mon, Jan 31, 2011 at 12:46:56 +0200, Rami Ylimäki wrote:


This change makes it possible to guard a system against a missing
XInitThreads call in X clients. One might argue that this is a client
problem and that all X clients should call XInitThreads if it's
possible that they could use Xlib from multiple threads. However,
experience has shown that it's just too easy for developers to
overlook the need for this call.


This change means that somebody developing their apps on a system with
it on will never see they need to call XInitThreads(), and stuff will
break when moving to an Xlib without that option.  I don't think 
that's

a good idea.


I do agree with Julien in the sense that developers could become
lazy and in general won't care much anymore about initializing
thread support.

OTOH, if you pick a Qt application for instance, you will see that
it is close to impossible to track from a stack trace of 80
functions whether XInitThreads() is called properly or not. At the
same time, it's not a big deal to maintain the support Rami made in
libX11. So why not?


Doesn't sound like you agree with me at all.  I just said why not, I'm
not going to repeat it.


well, as you can see above (I do agree with Julien in the sense), I 
partially agreed with you. Nothing to worry here :)




I understand what you're trying to solve, I
disagree that an xlib configure option is the way to go.


cool that you understand. So could you suggest and recommend another 
better way for everyone then?


Tiago


Would it be possible to guard against misbehaving applications by making 
it possible to configure libX11 with a dummy mutex?


typedef int dummy_mutex;

void lock(int *m) { assert(++(*m) == 1); };
void unlock(int *m) { *m = 0 };

It's not perfect but I guess it would be better than crashing in some 
random place in libX11.


-- Rami

___
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 libX11] config: Add an option to initialize threads by default.

[Rami Ylimäki]

Would it be possible to guard against misbehaving applications by 
making it possible to configure libX11 with a dummy mutex?


typedef int dummy_mutex;

void lock(int *m) { assert(++(*m) == 1); };
void unlock(int *m) { *m = 0 };

It's not perfect but I guess it would be better than crashing in some 
random place in libX11.


-- Rami



Answering to self... That wouldn't probably work because the locks in 
libX11 seem to be recursive.


-- Rami

___
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 x11perf] Make sure that window fits into screen.

[Rami Ylimäki]

XGetImage request will correctly fail and terminate x11perf with a
BadMatch error if window is larger than its backing pixmap.

This change makes all tests from putimage10 to getimagexy500 work on
screens that are smaller than the default window size.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 do_arcs.c|   12 ++--
 do_blt.c |   36 ++--
 do_complex.c |8 
 do_lines.c   |   16 
 do_movewin.c |   10 +-
 do_rects.c   |4 ++--
 do_segs.c|   12 ++--
 do_text.c|   14 +++---
 do_traps.c   |   18 +-
 do_tris.c|4 ++--
 do_windows.c |6 +++---
 x11perf.c|   24 +++-
 x11perf.h|5 ++---
 13 files changed, 87 insertions(+), 82 deletions(-)

diff --git a/do_arcs.c b/do_arcs.c
index 5c5f1ba..63eda65 100644
--- a/do_arcs.c
+++ b/do_arcs.c
@@ -69,16 +69,16 @@ GenerateCircles(XParms xp, Parms p, Bool partialArcs, Bool 
ddashed)
 
y += size + 1;
rows++;
-   if (y = HEIGHT - size  - half || rows == MAXROWS) {
+   if (y = windowHeight - size  - half || rows == MAXROWS) {
/* Go to next column */
rows = 0;
x += size + 1;
-   if (x = WIDTH - size) {
+   if (x = windowWidth - size) {
yorg++;
-   if (yorg = size + half || yorg = HEIGHT - size - half) {
+   if (yorg = size + half || yorg = windowHeight - size - half) {
yorg = half;
xorg++;
-   if (xorg = size + half || xorg = WIDTH - size - half) {
+   if (xorg = size + half || xorg = windowWidth - size - 
half) {
xorg = half;
}
}
@@ -287,12 +287,12 @@ GenerateEllipses(XParms xp, Parms p, int partialArcs, 
Bool ddashed)
 
y += size + 1;
rows++;
-   if (y = HEIGHT - size - half || rows == MAXROWS) {
+   if (y = windowHeight - size - half || rows == MAXROWS) {
/* Go to next column */
rows = 0;
y = half;
x += size + 1;
-   if (x = WIDTH - size - half) {
+   if (x = windowWidth - size - half) {
x = half;
}
}
diff --git a/do_blt.c b/do_blt.c
index f843ae5..4be2836 100644
--- a/do_blt.c
+++ b/do_blt.c
@@ -41,22 +41,22 @@ InitBltLines(void)
 points[0].x = points[0].y = y = 0;
 for (i = 1; i != NUMPOINTS/2; i++) {
if (i  1) {
-   points[i].x = WIDTH-1;
+   points[i].x = windowWidth-1;
} else {
points[i].x = 0;
}
-   y += HEIGHT / (NUMPOINTS/2);
+   y += windowHeight / (NUMPOINTS/2);
points[i].y = y;
 }
 
 x = 0;
 for (i = NUMPOINTS/2; i!= NUMPOINTS; i++) {
if (i  1) {
-   points[i].y = HEIGHT-1;
+   points[i].y = windowHeight-1;
} else {
points[i].y = 0;
}
-   x += WIDTH / (NUMPOINTS/2);
+   x += windowWidth / (NUMPOINTS/2);
points[i].x = x;
 }
 }
@@ -91,18 +91,18 @@ DoScroll(XParms xp, Parms p, int reps)
XCopyArea(xp-d, xp-w, xp-w, xp-fggc, x, y + delta,
size, size, x, y);
y += size;
-   if (y + size + delta  HEIGHT) {
+   if (y + size + delta  windowHeight) {
yorg += delta;
-   if (yorg = size || yorg + size + delta  HEIGHT) {
+   if (yorg = size || yorg + size + delta  windowHeight) {
yorg = 0;
xorg++;
-   if (xorg = size || xorg + size  WIDTH) {
+   if (xorg = size || xorg + size  windowWidth) {
xorg = 0;
}
}
y = yorg;
x += size;
-   if (x + size  WIDTH) {
+   if (x + size  windowWidth) {
x = xorg;
}
}
@@ -138,8 +138,8 @@ InitCopyLocations(XParms xp, Parms p, int reps)
 xinc = (size  ~3) + 1;
 yinc = xinc + 3;
 
-width = (WIDTH - size)  ~31;
-height = (HEIGHT - size)  ~31;
+width = (windowWidth - size)  ~31;
+height = (windowHeight - size)  ~31;
 
 x1 = 0;
 y1 = 0;
@@ -197,10 +197,10 @@ InitCopyPix(XParms xp, Parms p, int reps)
 (void) InitCopyWin(xp, p, reps);
 
 /* Create pixmap to write stuff into, and initialize it */
-pix = XCreatePixmap(xp-d, xp-w, WIDTH, HEIGHT, xp-vinfo.depth);
+pix = XCreatePixmap(xp-d, xp-w, windowWidth, windowHeight, 
xp-vinfo.depth);
 pixgc = XCreateGC(xp-d, pix, 0, NULL);
 /* need a gc with GXcopy cos pixmaps contain junk on creation. mmm */
-XCopyArea(xp-d, xp-w, pix, pixgc, 0, 0, WIDTH, HEIGHT, 0, 0);
+XCopyArea(xp-d, xp-w, pix, pixgc, 0, 0, windowWidth, windowHeight, 0, 0);
 XFreeGC(xp-d, pixgc);
 return reps;
 }
@@ -211,7 +211,7 @@ InitGetImage(XParms xp, Parms p, int reps)
 (void) InitCopyWin(xp, p, reps);
 
 /* Create

Re: [PATCH x11perf] Make sure that window fits into screen.

[Rami Ylimäki]

On 01/28/2011 03:14 PM, walter harms wrote:


Am 28.01.2011 13:59, schrieb Michel Dänzer:

On Fre, 2011-01-28 at 13:10 +0200, Rami Ylimäki wrote:

XGetImage request will correctly fail and terminate x11perf with a
BadMatch error if window is larger than its backing pixmap.

This change makes all tests from putimage10 to getimagexy500 work on
screens that are smaller than the default window size.

The numbers are not comparable if the operation dimensions are
different. It might be better to refuse tests that can't fit.



but the idea i correct. May be an extra test ?

On the other side: what is the time difference ?

re,
  wh


For example, compwinwin500 was 80% faster with reduced window size.

How about refusing the tests by default to make them comparable, but 
introducing -width, -height, and -redirect options for those who still 
want to run all tests? The extra options would print to the output to 
make it clear that nonstandard options are used.


-- Rami

___
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] [PATCH v2 10/12] [xserver] dix: Add facilities for client ID tracking.

[Rami Ylimäki]

On 12/31/2010 03:32 PM, Erkki Seppälä wrote:

From: Rami Ylimäkirami.ylim...@vincit.fi

An interface is provided for figuring out the PID and process name of
a client. Make some existing functionality from SELinux and IA
extensions available for general use.

Signed-off-by: Rami Ylimäkirami.ylim...@vincit.fi
Reviewed-by: Tiago Vignattitiago.vigna...@nokia.com
---
  configure.ac |   15 ++-
  dix/Makefile.am  |1 +
  dix/client.c |  345 ++
  dix/main.c   |3 +
  hw/xfree86/loader/sdksyms.sh |1 +
  include/Makefile.am  |1 +
  include/client.h |   60 
  include/dix-config.h.in  |3 +
  include/os.h |3 +
  os/access.c  |   76 +
  10 files changed, 507 insertions(+), 1 deletions(-)
  create mode 100644 dix/client.c
  create mode 100644 include/client.h


This is an old version of the patch. The most recent version can be 
found from http://lists.x.org/archives/xorg-devel/2010-December/017441.html.


-- Rami

___
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 0/3] Client ID tracking

[Rami Ylimäki]

Fixes based on comments of various reviewers. The changes are also
available at clientids branch of
ssh://g...@gitorious.org/rjy-fdo/xserver.git.

v2
--
Mark Kettenis:
move /proc specific code into os
treat pid -1 as error instead of 0
Tiago Vignatti:
preserve copyrights
amend authorship
add description of the framework
use doxygen style
rename cmd - cmdline in ClientIdsPrivateRec
compile only if XRES or XSELINUX is enabled

v3
--
Fernando Carrijo:
don't use -o or -a with test shell command
Tiago Vignatti:
use doxygen @file directive
move pid_t related functions into os
remove unnecessary header inclusions
let client ID tracking be disabled with --disable-clientids
create dummy stubs for initialization functions if disabled
don't make XSELinux depend on client ID tracking
squash copyrights into main commit
don't add copyright for restructuring of minor code segments

Review comments not respected:
don't install client ID tracking headers
Rationale:
As soon as client ID tracking is incorporated into X server, it
will be used from debugging code of at least one video driver.

v4
--
Keith Packard:
add fields to client record instead of using client privates
move code from DIX to OS

Rami Ylimäki (3):
  config: Fix linking order of Xnest libraries.
  os: Add facilities for client ID tracking.
  Xext: Use general OS functions to determine client command string in
SELinux.

 Xext/xselinux_hooks.c|   34 +++---
 configure.ac |   15 ++-
 dix/dispatch.c   |   10 ++
 dix/main.c   |3 +
 hw/xfree86/loader/sdksyms.sh |1 +
 include/Makefile.am  |1 +
 include/client.h |   59 
 include/dix-config.h.in  |3 +
 include/dixstruct.h  |2 +
 os/Makefile.am   |1 +
 os/client.c  |  310 ++
 11 files changed, 421 insertions(+), 18 deletions(-)
 create mode 100644 include/client.h
 create mode 100644 os/client.c

___
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 2/3] os: Add facilities for client ID tracking.

[Rami Ylimäki]

An interface is provided for figuring out the PID and process name of
a client. Make some existing functionality from SELinux and IA
extensions available for general use.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Tiago Vignatti tiago.vigna...@nokia.com
---
 configure.ac |   13 ++
 dix/dispatch.c   |   10 ++
 dix/main.c   |3 +
 hw/xfree86/loader/sdksyms.sh |1 +
 include/Makefile.am  |1 +
 include/client.h |   59 
 include/dix-config.h.in  |3 +
 include/dixstruct.h  |2 +
 os/Makefile.am   |1 +
 os/client.c  |  310 ++
 10 files changed, 403 insertions(+), 0 deletions(-)
 create mode 100644 include/client.h
 create mode 100644 os/client.c

diff --git a/configure.ac b/configure.ac
index cbbf5c3..202edf0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -623,6 +623,7 @@ AC_ARG_ENABLE(vbe,
AS_HELP_STRING([--enable-vbe], [Build Xorg with VB
 AC_ARG_ENABLE(int10-module, AS_HELP_STRING([--enable-int10-module], [Build 
Xorg with int10 module (default: enabled)]), [INT10MODULE=$enableval], 
[INT10MODULE=yes])
 AC_ARG_ENABLE(windowswm,  AS_HELP_STRING([--enable-windowswm], [Build XWin 
with WindowsWM extension (default: no)]), [WINDOWSWM=$enableval], 
[WINDOWSWM=no])
 AC_ARG_ENABLE(libdrm, AS_HELP_STRING([--enable-libdrm], [Build Xorg 
with libdrm support (default: enabled)]), [DRM=$enableval],[DRM=yes])
+AC_ARG_ENABLE(clientids,  AS_HELP_STRING([--disable-clientids], [Build 
Xorg with client ID tracking (default: enabled)]), [CLIENTIDS=$enableval], 
[CLIENTIDS=yes])
 
 dnl DDXes.
 AC_ARG_ENABLE(xorg,  AS_HELP_STRING([--enable-xorg], [Build 
Xorg server (default: auto)]), [XORG=$enableval], [XORG=auto])
@@ -976,6 +977,18 @@ if test x$RES = xyes; then
REQUIRED_MODULES=$REQUIRED_MODULES $RESOURCEPROTO
 fi
 
+# The XRes extension may support client ID tracking only if it has
+# been specifically enabled. Client ID tracking is implicitly not
+# supported if XRes extension is disabled.
+AC_MSG_CHECKING([whether to track client ids])
+if test x$RES = xyes  test x$CLIENTIDS = xyes; then
+   AC_DEFINE(CLIENTIDS, 1, [Support client ID tracking])
+else
+   CLIENTIDS=no
+fi
+AC_MSG_RESULT([$CLIENTIDS])
+AM_CONDITIONAL(CLIENTIDS, [test x$CLIENTIDS = xyes])
+
 if test x$GLX = xyes; then
PKG_CHECK_MODULES([XLIB], [x11])
PKG_CHECK_MODULES([GL], $GLPROTO $LIBGL)
diff --git a/dix/dispatch.c b/dix/dispatch.c
index 7b2132d..601b14a 100644
--- a/dix/dispatch.c
+++ b/dix/dispatch.c
@@ -130,6 +130,7 @@ int ProcInitialConnection();
 #include inputstr.h
 #include xkbsrv.h
 #include site.h
+#include client.h
 
 #ifdef XSERVER_DTRACE
 #include registry.h
@@ -3459,6 +3460,9 @@ CloseDownClient(ClientPtr client)
CallCallbacks((ClientStateCallback), (pointer)clientinfo);
}   
FreeClientResources(client);
+   /* Disable client ID tracking. This must be done after
+* ClientStateCallback. */
+   ReleaseClientIds(client);
 #ifdef XSERVER_DTRACE
XSERVER_CLIENT_DISCONNECT(client-index);
 #endif 
@@ -3496,6 +3500,7 @@ void InitClient(ClientPtr client, int i, pointer ospriv)
 client-smart_start_tick = SmartScheduleTime;
 client-smart_stop_tick = SmartScheduleTime;
 client-smart_check_tick = SmartScheduleTime;
+client-clientIds = NULL;
 }
 
 /
@@ -3535,6 +3540,11 @@ ClientPtr NextAvailableClient(pointer ospriv)
currentMaxClients++;
 while ((nextFreeClientID  MAXCLIENTS)  clients[nextFreeClientID])
nextFreeClientID++;
+
+/* Enable client ID tracking. This must be done before
+ * ClientStateCallback. */
+ReserveClientIds(client);
+
 if (ClientStateCallback)
 {
NewClientInfoRec clientinfo;
diff --git a/dix/main.c b/dix/main.c
index 692bec1..31e2d48 100644
--- a/dix/main.c
+++ b/dix/main.c
@@ -104,6 +104,7 @@ Equipment Corporation.
 #include extnsionst.h
 #include privates.h
 #include registry.h
+#include client.h
 #ifdef PANORAMIX
 #include panoramiXsrv.h
 #else
@@ -258,6 +259,7 @@ int main(int argc, char *argv[], char *envp[])
 InitCoreDevices();
InitInput(argc, argv);
InitAndStartDevices();
+   ReserveClientIds(serverClient);
 
dixSaveScreens(serverClient, SCREEN_SAVER_FORCER, ScreenSaverReset);
 
@@ -323,6 +325,7 @@ int main(int argc, char *argv[], char *envp[])
screenInfo.numScreens = i;
}
 
+   ReleaseClientIds(serverClient);
dixFreePrivates(serverClient-devPrivates, PRIVATE_CLIENT);
serverClient-devPrivates = NULL;
 
diff --git a/hw/xfree86/loader/sdksyms.sh b/hw/xfree86/loader/sdksyms.sh
index f60b8ed..18bb735 100755
--- a/hw/xfree86/loader/sdksyms.sh
+++ b/hw/xfree86/loader/sdksyms.sh
@@ -259,6 +259,7 @@ cat  sdksyms.c  EOF
 #include colormap.h
 #include

[PATCH v4 xserver 3/3] Xext: Use general OS functions to determine client command string in SELinux.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Reviewed-by: Tiago Vignatti tiago.vigna...@nokia.com
---
 Xext/xselinux_hooks.c |   34 +-
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/Xext/xselinux_hooks.c b/Xext/xselinux_hooks.c
index 560e1e9..f1d8e5d 100644
--- a/Xext/xselinux_hooks.c
+++ b/Xext/xselinux_hooks.c
@@ -40,6 +40,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN 
THE SOFTWARE.
 #include propertyst.h
 #include extnsionst.h
 #include xacestr.h
+#include client.h
 #include ../os/osdep.h
 #define _XSELINUX_NEED_FLASK_MAP
 #include xselinuxint.h
@@ -129,26 +130,25 @@ SELinuxLabelClient(ClientPtr client)
 
 /* For local clients, try and determine the executable name */
 if (XaceIsLocal(client)) {
-   struct ucred creds;
-   socklen_t len = sizeof(creds);
-   char path[PATH_MAX + 1];
-   size_t bytes;
-
-   memset(creds, 0, sizeof(creds));
-   if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, creds, len)  0)
-   goto finish;
+   /* Get cached command name if CLIENTIDS is enabled. */
+   const char *cmdname = GetClientCmdName(client);
+   Bool cached = (cmdname != NULL);
+   /* If CLIENTIDS is disabled, figure out the command name from
+* scratch. */
+   if (!cmdname)
+   {
+   pid_t pid = DetermineClientPid(client);
+   if (pid != -1)
+   DetermineClientCmd(pid, cmdname, NULL);
+   }
 
-   snprintf(path, PATH_MAX + 1, /proc/%d/cmdline, creds.pid);
-   fd = open(path, O_RDONLY);
-   if (fd  0)
+   if (!cmdname)
goto finish;
 
-   bytes = read(fd, path, PATH_MAX + 1);
-   close(fd);
-   if (bytes = 0)
-   goto finish;
+   strncpy(subj-command, cmdname, COMMAND_LEN - 1);
 
-   strncpy(subj-command, path, COMMAND_LEN - 1);
+   if (!cached)
+   free((void *) cmdname); /* const char * */
 }
 
 finish:
-- 
1.6.3.3

___
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][PATCH 1/5] XResource extension v1.2 implementation of XResQueryClientIds

[Rami Ylimäki]

On 12/03/2010 12:44 PM, Erkki Seppälä wrote:

From: Rami Ylimäkirami.ylim...@vincit.fi
Reviewed-by: Erkki Seppalaerkki.sepp...@vincit.fi

Let's preserve the previous Acked-by tag also.

+XResQueryClientIds
+num_specs:CARD32
+client_specs: LISTofCLIENTIDSPEC
+â–¶
+num_ids:  CARD32
+client_ids:   LISTofCLIENTIDVALUE
+
+Errors:   Value
We have to add Alloc error to the list of possible errors, because the 
implementation may return BadAlloc.


-- Rami

___
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] dri2: Destroy buffer before reseting the private key

[Rami Ylimäki]

Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

On 11/26/2010 05:07 PM, Pauli wrote:

From: Pauli Nieminenext-pauli.niemi...@nokia.com

Destroying buffers after reseting the private key would prevent DDX
from calling DRI2 functions that require private.

This can be result to
[DRI2] DRI2SwapComplete: bad drawable
when drivers calls SwapCompletion in state clean up code.

Restriction can be avoided if destroy buffer hook is called before
private key clean up.

Signed-off-by: Pauli Nieminenext-pauli.niemi...@nokia.com
---
  hw/xfree86/dri2/dri2.c |   14 +++---
  1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index e4693d9..336fee8 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -289,6 +289,13 @@ static int DRI2DrawableGone(pointer p, XID id)
  if (!list_is_empty(pPriv-reference_list))
return Success;

+if (pPriv-buffers != NULL) {
+   for (i = 0; i  pPriv-bufferCount; i++)
+   (*ds-DestroyBuffer)(pDraw, pPriv-buffers[i]);
+
+   free(pPriv-buffers);
+}
+
  pDraw = pPriv-drawable;
  if (pDraw-type == DRAWABLE_WINDOW) {
pWin = (WindowPtr) pDraw;
@@ -298,13 +305,6 @@ static int DRI2DrawableGone(pointer p, XID id)
dixSetPrivate(pPixmap-devPrivates, dri2PixmapPrivateKey, NULL);
  }

-if (pPriv-buffers != NULL) {
-   for (i = 0; i  pPriv-bufferCount; i++)
-   (*ds-DestroyBuffer)(pDraw, pPriv-buffers[i]);
-
-   free(pPriv-buffers);
-}
-
  free(pPriv);

  return Success;


___
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][PATCH v3 resourceproto 0/1] Protocol description of X Resource Extension version 1.2.

[Rami Ylimäki]

v1: Initial documentation of changes from version 1.1 to 1.2.
v2: Version 1.2 takes resource cross references into account.
v3: Version 1.1 has been reverse engineered from code to the document.

Rami Ylimäki (1):
  Protocol description of X Resource Extension version 1.2.

 RESproto.txt |  452 ++
 1 files changed, 452 insertions(+), 0 deletions(-)
 create mode 100644 RESproto.txt

___
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: How to get a list of windows and process mapping

[Rami Ylimäki]

 On 11/17/2010 06:41 PM, Trevor Woerner wrote:

On Tue, Nov 16, 2010 at 3:44 PM, Adam Majerad...@zombino.com  wrote:

Is there a method of getting a list of all the
windows on a given display or screen? Is there a method of mapping
these Windows to host/pid?

Maybe having a look at xwininfo (and its sources) will help get you started?
$ xwininfo -root -tree -wm


We are currently implementing a better mechanism to identify resource 
owners/clients with X resource extension v1.2: 
http://lists.x.org/archives/xorg-devel/2010-November/015538.html.


It will still take a few weeks to implement that so with current X 
server versions you need to do it the xrestop/xwininfo way.


-- Rami

___
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 2/2 v3] dix: fix root window background behaviour for protocol calls

[Rami Ylimäki]

 Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

On 11/10/2010 09:48 PM, Tiago Vignatti wrote:

Instead always paint root tiled (-retro like), protocol calls
(XSetWindowBackgroundPixmap and related) should behave accordingly when None
and ParentRelative is set as background pixmap.

It follow what the protocol states: changing the background of a root window
to None or ParentRelative restores the default background pixmap.

Signed-off-by: Tiago Vignattitiago.vigna...@nokia.com
Signed-off-by: Ville Syrjäläville.syrj...@nokia.com
---
One would want to create another patch cleaning-up and use
SetRootWindowBackground() on InitRootWindow()

changes made on v2:
- remove canDoBGNoneRoot check (kudos to Ville)
- restores always the default background pixmap (kudos to Rami)

changes made on v3:
- ParentRelative should follow None (kudos to Ville)
- BlackPixel and WhitePixel should also be caught (kudos to Ville)
- now using SetRootWindowBackground for that code

  dix/window.c |   24 ++--
  1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/dix/window.c b/dix/window.c
index bfaa6f5..cba583d 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -956,6 +956,26 @@ DestroySubwindows(WindowPtr pWin, ClientPtr client)
  return Success;
  }

+static void
+SetRootWindowBackground(WindowPtr pWin, ScreenPtr pScreen, Mask *index2)
+{
+/* following the protocol: Changing the background of a root window to
+ * None or ParentRelative restores the default background pixmap */
+if (bgNoneRoot) {
+   pWin-backgroundState = XaceBackgroundNoneState(pWin);
+   pWin-background.pixel = pScreen-whitePixel;
+}
+else if (party_like_its_1989)
+   MakeRootTile(pWin);
+else {
+   if (whiteRoot)
+   pWin-background.pixel = pScreen-whitePixel;
+   else
+   pWin-background.pixel = pScreen-blackPixel;
+   *index2 = CWBackPixel;
+}
+}
+
  /*
   *  ChangeWindowAttributes
   *
@@ -1005,7 +1025,7 @@ ChangeWindowAttributes(WindowPtr pWin, Mask vmask, XID 
*vlist, ClientPtr client)
if (pWin-backgroundState == BackgroundPixmap)
(*pScreen-DestroyPixmap)(pWin-background.pixmap);
if (!pWin-parent)
-   MakeRootTile(pWin);
+   SetRootWindowBackground(pWin, pScreen,index2);
else {
pWin-backgroundState = XaceBackgroundNoneState(pWin);
pWin-background.pixel = pScreen-whitePixel;
@@ -1022,7 +1042,7 @@ ChangeWindowAttributes(WindowPtr pWin, Mask vmask, XID 
*vlist, ClientPtr client)
if (pWin-backgroundState == BackgroundPixmap)
(*pScreen-DestroyPixmap)(pWin-background.pixmap);
if (!pWin-parent)
-   MakeRootTile(pWin);
+   SetRootWindowBackground(pWin, pScreen,index2);
else
pWin-backgroundState = ParentRelative;
borderRelative = TRUE;


___
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][PATCH v2 resourceproto 0/1] Protocol description of X Resource Extension version 1.2.

[Rami Ylimäki]

Hi,

The version 2 of this patch fixes a case that was overlooked in
version 1. Previously the protocol made it possible to query sizes of
isolated resources. Fixed version can be used to query sizes of all
lower level resources referenced by a higher level resource as
well. This was done so that XResQueryClientPixmapBytes could be
generalized to all resource types.

Gaetan, let's wait pushing this document to the repository until there
is approval of the changes and somebody has reviewed the document.

Rami Ylimäki (1):
  Protocol description of X Resource Extension version 1.2.

 RESproto.txt |  317 ++
 1 files changed, 317 insertions(+), 0 deletions(-)
 create mode 100644 RESproto.txt

___
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][PATCH v2 resourceproto 1/1] Protocol description of X Resource Extension version 1.2.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
Acked-by: Gaetan Nadon mems...@videotron.ca

---
 RESproto.txt |  317 ++
 1 files changed, 317 insertions(+), 0 deletions(-)
 create mode 100644 RESproto.txt

diff --git a/RESproto.txt b/RESproto.txt
new file mode 100644
index 000..67f8905
--- /dev/null
+++ b/RESproto.txt
@@ -0,0 +1,317 @@
+   DRAFT FOR REVIEW
+   The X Resource Extension
+ Version 1.2
+ Rami Ylimäki
+rami.ylim...@vincit.fi
+
+ ❧❧❧
+
+1. Introduction
+
+The protocol description of X Resource Extension version 1.1 has been
+either lost or has never been written. This specification documents
+only the changes from version 1.1 to version 1.2. The X Resource
+Extension is quite simple and therefore the description of version 1.1
+could be reverse engineered into this document later if the need
+arises.
+
+Version 1.2 is a minor release and therefore the changes are
+compatible with the previous version. Main enhacements over version
+1.1 are:
+
+- Client identification is now possible. For example, servers
+  supporting version 1.2 may report PID of local clients.
+
+- Size of any resource can be queried from the server. Servers may not
+  necessarily support size calculation for every resource. However,
+  clients have now at least the chance to let the server do resource
+  size estimation for them.
+
+ ❧❧❧
+
+2. Notations used in this document
+
+Notation for data types and requests follows the guidelines set in
+sections 2-4 of X Window System Protocol standard.
+
+ ❧❧❧
+
+3. Interoperability between version 1.1 and 1.2
+
+Version 1.2 only introduces two new requests. However, these requests
+could be seen as extended versions of existing requests. Even though
+we aren't deprecating any old requests, libraries could implement some
+old requests using the new ones.
+
+The new XResQueryClientIds request could be used instead of
+XResQueryClients.
+
+The new XResQueryResourceBytes request could be used instead of
+XResQueryClientPixmapBytes.
+
+Using the old requests is still acceptable though because we don't
+want to change the existing requests between version 1.1 and 1.2.
+
+ ❧❧❧
+
+4. Data types
+
+4.1 Types in version 1.1
+
+CLIENTXIDRANGE [ resource_base: CARD32
+ resource_mask: CARD32 ]
+This type is used for reply of XResQueryClients in version 1.1.
+resource_base
+First resource ID reserved for a client. Used also to identify the
+clients themselves.
+resource_mask
+Mask that can be used to identify a client from some resource
+ID. Just zero the bits indicated by this mask from any resource ID
+to identify the client that owns the resource.
+
+4.2 Types in version 1.2
+
+4.2.1 Types used by XResQueryClientIds
+
+CLIENTIDMASK { ClientXid = 0x1, LocalClientPid = 0x2 }
+A bitmask specifying a client identification method. Currently
+only the PID of local clients is supported in the form of
+LocalClientPid. ClientXid is provided for backward compatibility
+with version 1.1 so that the new 1.2 requests (XResQueryClientIds)
+can be used in place of the older ones (XResQueryClients).
+
+CLIENTIDSPEC [ client: XID or None
+   mask:   SETofCLIENTIDMASK or None ]
+A data structure for selecting client IDs.
+client
+ID of a resource allocated for some client. Only the part
+identifying a client is actually used. The resource_base of
+CLIENTXIDRANGE can be used if the client doesn't own any
+resources. However, any resource ID is accepted because that makes
+identifying the owners of existing resources easy. The null
+resource None can be used to select all clients.
+mask
+Collection of identification methods that should be applied on the
+client. The special value None can be used to apply all supported
+identification methods.
+
+CLIENTIDVALUE [ spec:   CLIENTIDSPEC
+length: CARD32
+value:  LISTofCARD32 ]
+A data structure specifying a single client ID.
+spec
+A unique identifier for a specific ID of some client. Wildcards
+such as None and bitmask unions aren't allowed. The data structure
+must always identify a single client and single ID type. However,
+the client doesn't have to be specified as the resource_base of
+CLIENTXIDRANGE and can be any resource owned by the client.
+length
+Specifies the length of an ID in units of CARD32. The length
+depends on the ID type. In version 1.2 the lengths are 0 for
+ClientXid and 4 for LocalClientPid. The length of ClientXid is 0
+because that is already stored in the spec field.
+value
+Actual ID data. In version 1.2 this is missing

[RFC][PATCH resourceproto] Protocol description of X Resource Extension version 1.2.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 RESproto.txt |  268 ++
 1 files changed, 268 insertions(+), 0 deletions(-)
 create mode 100644 RESproto.txt

diff --git a/RESproto.txt b/RESproto.txt
new file mode 100644
index 000..74193bb
--- /dev/null
+++ b/RESproto.txt
@@ -0,0 +1,268 @@
+   DRAFT FOR REVIEW
+   The X Resource Extension
+ Version 1.2
+ Rami Ylimäki
+rami.ylim...@vincit.fi
+
+ ❧❧❧
+
+1. Introduction
+
+The protocol description of X Resource Extension version 1.1 has been
+either lost or has never been written. This specification documents
+only the changes from version 1.1 to version 1.2. The X Resource
+Extension is quite simple and therefore the description of version 1.1
+could be reverse engineered into this document later if the need
+arises.
+
+Version 1.2 is a minor release and therefore the changes are
+compatible with the previous version. Main enhacements over version
+1.1 are:
+
+- Client identification is now possible. For example, servers
+  supporting version 1.2 may report PID of local clients.
+
+- Size of any resource can be queried from the server. Servers may not
+  necessarily support size calculation for every resource. However,
+  clients have now at least the chance to let the server do resource
+  size estimation for them.
+
+ ❧❧❧
+
+2. Notations used in this document
+
+Notation for data types and requests follows the guidelines set in
+sections 2-4 of X Window System Protocol standard.
+
+ ❧❧❧
+
+3. Interoperability between version 1.1 and 1.2
+
+Version 1.2 only introduces two new requests. However, these requests
+could be seen as extended versions of existing requests. Even though
+we aren't deprecating any old requests, libraries could implement some
+old requests using the new ones.
+
+The new XResQueryClientIds request could be used instead of
+XResQueryClients.
+
+The new XResQueryResourceBytes request could be used instead of
+XResQueryClientPixmapBytes.
+
+Using the old requests is still acceptable though because we don't
+want to change the existing requests between version 1.1 and 1.2.
+
+ ❧❧❧
+
+4. Data types
+
+4.1 Types in version 1.1
+
+CLIENTXIDRANGE [ resource_base: CARD32
+ resource_mask: CARD32 ]
+This type is used for reply of XResQueryClients in version 1.1.
+resource_base
+First resource ID reserved for a client. Used also to identify the
+clients themselves.
+resource_mask
+Mask that can be used to identify a client from some resource
+ID. Just zero the bits indicated by this mask from any resource ID
+to identify the client that owns the resource.
+
+4.2 Types in version 1.2
+
+4.2.1 Types used by XResQueryClientIds
+
+CLIENTIDMASK { ClientXid = 0x1, LocalClientPid = 0x2 }
+A bitmask specifying a client identification method. Currently
+only the PID of local clients is supported in the form of
+LocalClientPid. ClientXid is provided for backward compatibility
+with version 1.1 so that the new 1.2 requests (XResQueryClientIds)
+can be used in place of the older ones (XResQueryClients).
+
+CLIENTIDSPEC [ client: XID or None
+   mask:   SETofCLIENTIDMASK or None ]
+A data structure for selecting client IDs.
+client
+ID of a resource allocated for some client. Only the part
+identifying a client is actually used. The resource_base of
+CLIENTXIDRANGE can be used if the client doesn't own any
+resources. However, any resource ID is accepted because that makes
+identifying the owners of existing resources easy. The null
+resource None can be used to select all clients.
+mask
+Collection of identification methods that should be applied on the
+client. The special value None can be used to apply all supported
+identification methods.
+
+CLIENTIDVALUE [ spec:   CLIENTIDSPEC
+   length: CARD32
+value:  LISTofCARD32 ]
+A data structure specifying a single client ID.
+spec
+A unique identifier for a specific ID of some client. Wildcards
+such as None and bitmask unions aren't allowed. The data structure
+must always identify a single client and single ID type. However,
+the client doesn't have to be specified as the resource_base of
+CLIENTXIDRANGE and can be any resource owned by the client.
+length
+
+Specifies the length of an ID in units of CARD32. The length
+depends on the ID type. In version 1.2 the lengths are 0 for
+ClientXid and 4 for LocalClientPid.  The length of ClientXid is 0
+because that is already stored in the spec field.
+value
+Actual ID data. In version 1.2 this is missing for ClientXid and
+consists of a single CARD32

Re: [PATCH 2/2] dix: fix None root background behaviour for protocol calls

[Rami Ylimäki]

 On 11/09/2010 07:26 PM, Tiago Vignatti wrote:

Instead always paint root tiled (-retro like), protocol calls
(XSetWindowBackgroundPixmap and related) should behave accordingly when None
is set as background pixmap.

Now, even if the server is started without -background none, the user can
set the root background to None, which will have the same behaviour as that
option.


I think this is against the specification of ChangeWindowAttributes: 
Changing the background of a root window to None or ParentRelative 
restores the default background pixmap.


If X server was started with -br, then setting root window background to 
None should restore the black background, which was the default.


The minimal way to fix this would be to allow disabling of root window 
background only if -background none was given. A better way would be 
to create new XFixes requests to set and get the default background and 
then modify ChangeWindowAttributes to use whatever has been set as default.


It would also make the code easier to understand if InitRootWindow and 
ChangeWindowAttributes would call a common function to set the root 
window background.


-- Rami

___
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 1/4] dix: Provide means to report exact sizes of resources.

[Rami Ylimäki]

 On 11/03/2010 07:48 PM, Daniel Stone wrote:

Hi,

On Thu, Oct 28, 2010 at 01:23:41PM +0300, Rami Ylimäki wrote:

+/**
+ * Override the default function that calculates resource size. For
+ * example, video driver knows better how to calculate pixmap memory
+ * usage and can therefore wrap or override size calculation for
+ * RT_PIXMAP.
+ *
+ * @param[in] type Resource type used in size calculations.
+ *
+ * @param[in] sizeFunc Function to calculate the size of a single
+ * resource.
+ */
+void
+SetResourceTypeSizeFunc(RESTYPE type, SizeType sizeFunc)
+{
+resourceTypes[type  TypeMask].sizeFunc = sizeFunc;
+}

So, I guess the intention here would be to call
SetResourceTypeSizeFunc(RT_PIXMAP, my_driver_how_big_is_the_pixmap); ?


Yes.


In that case, it falls apart in the multi-screen case (yeah, I know, it
barely works).  dixFreePixmap and FreeWindowResources get around this by
calling the corresponding ScreenRec hook: would the intention be to add
one to make the sizing work correctly for multiple drivers, or?


I overlooked the case that multiple drivers could be in use. Would it 
make sense to require cooperation from drivers to make this work? A 
driver would call GetResourceTypeSizeFunc to save the old function 
before registering its own. Then the driver can check if the resource is 
associated with a screen, what screen it is associated with and either 
calculate the size or just forward the calculation to the original function.


To be honest I'm not exactly sure what is meant by this multi-screen 
case. Does that mean that each screen could have its own driver that is 
then responsible of calculating the size of those pixmaps whose 
pixmap-drawable.pScreen matches the driver screen?



Other than that, looks good to me, but one thing to bear in mind if we
keep on extending resource usage would be to add a size argument to
AddResource(), as well as a new ResourceSizeChanged(), rather than
having to call back for every resource.  The callbacks will work fine
for the pixmap/window/etc case as drivers already have all the relevant
size information, but we might end up with resource types in the future
where you'd need a great deal of unnecessary bookkeeping just for these
callbacks.


Makes sense to have these alternative ways of reporting the resource 
size. Every resource object would have a size associated with it. If the 
size happens to be zero (not given) try to call the size callback.



Cheers,
Daniel


-- Rami

___
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 v3] dix: adds support for none root window background

[Rami Ylimäki]

 Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

If there is a need to modify ChangeWindowAttributes, that can be a 
separate commit just like the addition of white/black/retro for 
-background option.


On 11/03/2010 11:17 PM, Tiago Vignatti wrote:

It lets the driver notify the server whether it can draw a background when
'-background none' option is used by the system platform. Use cases for that
could be video drivers performing mode-setting in kernel time, before X is up,
so a seamless transition would happen until X clients start to show up.

If the driver can copy the framebuffer cleanly then it can set the flag
(canDoBGNoneRoot), otherwise the server will fallback to the normal behaviour.
The system must explicit indicates willingness of doing so through
'-background none'. We could do this option as default; in such case,
malicious users would be able to steal the framebuffer with a bit of tricks.

For instance, I can see the content of my nVidia Quadro FX 580 framebuffer
old X session modifying a bit nv driver:

--- a/src/g80_driver.c
+++ b/src/g80_driver.c
@@ -850,17 +850,6 @@ G80ScreenInit(int scrnIndex, ScreenPtr pScreen, int argc,
c

  xf86DPMSInit(pScreen, xf86DPMSSet, 0);

-/* Clear the screen */
-if(pNv-xaa) {
-/* Use the acceleration engine */
-pNv-xaa-SetupForSolidFill(pScrn, 0, GXcopy, ~0);
-pNv-xaa-SubsequentSolidFillRect(pScrn,
-0, 0, pScrn-displayWidth, pNv-offscreenHeight);
-G80DmaKickoff(pNv);
-} else {
-/* Use a slow software clear path */
-memset(pNv-mem, 0, pitch * pNv-offscreenHeight);
-}
+pScreen-canDoBGNoneRoot = TRUE;

The commit is originally based on discussions happened on xorg-devel:
http://lists.freedesktop.org/archives/xorg-devel/2010-June/009755.html

Signed-off-by: Tiago Vignattitiago.vigna...@nokia.com
---
changes on v3:
- '-nr' converted to '-background none' (Daniel's review). Others may want to
   add similar usage for -wr and -br, but keeping these for backward
   compatibility.

changes on v2:
- clarified the usage and example on commit's message. Clarification was done
   particularly  in detriment of security issues (Pauli's review).
- added canDoBGNoneRoot in the end of the struct to minimize effects from ABI
change (Pauli's review)
- canDoBGNoneRoot changed to Bool type instead int.


  dix/window.c |6 ++
  include/opaque.h |1 +
  include/scrnintstr.h |6 ++
  os/utils.c   |9 +
  4 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/dix/window.c b/dix/window.c
index 1913030..bfaa6f5 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -137,6 +137,8 @@ Equipment Corporation.
   *ChangeWindowDeviceCursor
   **/

+Bool bgNoneRoot = FALSE;
+
  static unsigned char _back_lsb[4] = {0x88, 0x22, 0x44, 0x11};
  static unsigned char _back_msb[4] = {0x11, 0x44, 0x22, 0x88};

@@ -463,6 +465,10 @@ InitRootWindow(WindowPtr pWin)
  if (party_like_its_1989) {
  MakeRootTile(pWin);
  backFlag |= CWBackPixmap;
+} else if (pScreen-canDoBGNoneRoot  bgNoneRoot) {
+pWin-backgroundState = XaceBackgroundNoneState(pWin);
+pWin-background.pixel = pScreen-whitePixel;
+backFlag |= CWBackPixmap;
  } else {
if (whiteRoot)
  pWin-background.pixel = pScreen-whitePixel;
diff --git a/include/opaque.h b/include/opaque.h
index dfe440c..b1696ca 100644
--- a/include/opaque.h
+++ b/include/opaque.h
@@ -72,6 +72,7 @@ extern _X_EXPORT Bool defeatAccessControl;
  extern _X_EXPORT long maxBigRequestSize;
  extern _X_EXPORT Bool party_like_its_1989;
  extern _X_EXPORT Bool whiteRoot;
+extern _X_EXPORT Bool bgNoneRoot;

  extern _X_EXPORT Bool CoreDump;

diff --git a/include/scrnintstr.h b/include/scrnintstr.h
index e36b15f..cd4fb70 100644
--- a/include/scrnintstr.h
+++ b/include/scrnintstr.h
@@ -601,6 +601,12 @@ typedef struct _Screen {
  /* Device cursor procedures */
  DeviceCursorInitializeProcPtr DeviceCursorInitialize;
  DeviceCursorCleanupProcPtrDeviceCursorCleanup;
+
+/* set it in driver side if X server can copy the framebuffer content.
+ * Meant to be used together with '-background none' option, avoiding
+ * malicious users to steal framebuffer's content if that would be the
+ * default */
+Bool   canDoBGNoneRoot;
  } ScreenRec;

  static inline RegionPtr BitmapToRegion(ScreenPtr _pScreen, PixmapPtr pPix) {
diff --git a/os/utils.c b/os/utils.c
index 8921d7c..9ca9d39 100644
--- a/os/utils.c
+++ b/os/utils.c
@@ -502,6 +502,7 @@ void UseMsg(void)
  #endif
  ErrorF(-nolisten string   don't listen on protocol\n);
  ErrorF(-noreset   don't reset after last client exists\n);
+ErrorF(-background [none] create root window with no background\n);
  ErrorF(-reset reset after last client exists\n);
  ErrorF(-p #   screen-saver pattern duration 
(minutes)\n

Re: [PATCH] dix: adds support for none root window background

[Rami Ylimäki]

 On 10/28/2010 04:57 PM, Tiago Vignatti wrote:

diff --git a/dix/window.c b/dix/window.c
index cfebb9d..7a47221 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -137,6 +137,8 @@ Equipment Corporation.
   *ChangeWindowDeviceCursor
   **/

+Bool bgNoneRoot = FALSE;
+
  static unsigned char _back_lsb[4] = {0x88, 0x22, 0x44, 0x11};
  static unsigned char _back_msb[4] = {0x11, 0x44, 0x22, 0x88};

@@ -463,6 +465,10 @@ InitRootWindow(WindowPtr pWin)
  if (party_like_its_1989) {
  MakeRootTile(pWin);
  backFlag |= CWBackPixmap;
+} else if (pScreen-canDoBGNoneRoot  bgNoneRoot) {
+pWin-backgroundState = XaceBackgroundNoneState(pWin);
+pWin-background.pixel = pScreen-whitePixel;
+backFlag |= CWBackPixmap;
  } else {
if (whiteRoot)
  pWin-background.pixel = pScreen-whitePixel;


I think this part should be harmonized with ChangeWindowAttributes. It 
should be possible to set the root window background to None from 
clients also. Currently is seems that MakeRootTile is enforced in 
ChangeWindowAttributes even though I can't find justification for that.


From Xlib Programming manual 3rd edition, section 4.3.1.1 
background_pixmap:
Changing the background_pixmap attribute of the root window to None 
or ParentRelative restores the default background, which is 
server-dependent.


In my opinion the command line parameters should be taken into account 
in ChangeWindowAttributes as well so that MakeRootTile is not enforced 
there.


With that change:
Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

___
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

[PULL] arm bt fix revert, client id tracking, resource size reporting

[Rami Ylimäki]

 Hi Keith,

The original pull request has been updated with recent resource size 
reporting fixes.


The following changes since commit 1a0d9324b3d9fd93e685066e0e5cea0611878c0d:
  Aaron Plattner (1):
Revert Set DamageSetReportAfterOp to true for the damage 
extension (#30260)


are available in the git repository at:

  git://gitorious.org/rjy-fdo/xserver.git for-keith

Rami Ylimäki (7):
  Revert os: Prevent backtrace from being stopped in noreturn 
functions.

  dix: Add facilities for client ID tracking.
  Xext: Use general OS functions to determine client command string 
in SELinux.

  dix: Provide means to report exact sizes of resources.
  dix: Add reverse resource name lookup function to registry.
  render: Report pixmap usage of pictures to resource extension.
  composite: Report pixmap usage of client windows to resource 
extension.


 Xext/xres.c  |   67 +
 Xext/xselinux_hooks.c|   22 +--
 composite/compext.c  |   24 +++
 configure.ac |   28 ++--
 dix/Makefile.am  |1 +
 dix/client.c |  345 
++

 dix/main.c   |3 +
 dix/registry.c   |   10 ++
 dix/resource.c   |  199 -
 hw/xfree86/loader/sdksyms.sh |1 +
 include/Makefile.am  |1 +
 include/client.h |   60 
 include/dix-config.h.in  |3 +
 include/os.h |3 +
 include/registry.h   |6 +
 include/resource.h   |   23 +++
 os/Makefile.am   |   17 +--
 os/access.c  |   76 +
 render/picture.c |   22 +++
 19 files changed, 835 insertions(+), 76 deletions(-)
 create mode 100644 dix/client.c
 create mode 100644 include/client.h

___
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

Location of X resource protocol description

[Rami Ylimäki]

Hi,

Is there such thing as X resource extension protocol description document
anywhere? I'm planning to amend the document with some queries for client
PIDs and resource sizes. However, I can't find the documentation from
http://www.x.org/docs/ or
http://cgit.freedesktop.org/xorg/proto/resourceproto/tree/. I'd like to work
on top of an existing document if it exists.

-- Rami
___
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] DRI2: Free DRI2 drawable references in DRI2DestroyDrawable

[Rami Ylimäki]

 From resource management point of view this patch seems correct to me.

Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi

On 10/25/2010 04:55 PM, Pauli Nieminen wrote:

If client calls DRI2CreateDrawable multiple times for same drawable
DRI2 creates multiple references. Multiple references cause DRI2 send
multiple invalidate events for same client.

Problem is easy to spot from xtrace. For example following filtered
snippet from problematic client:

000::0b85: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000::0b89: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8e: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000::0b8f: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000::0bc4: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000::0bc5: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0

Problem is triggered because client side EGL implementation is using
DRI2 directly.

DRI2 code in server doesn't handle DRI2DestroyDrawable that leaks
references. If client recreates rendering target EGL destroys and
creates DRI2 drawable. In that case DRI2DestroyDrawable leaks DRI2
drawable references.

To fix this memory leak/performance problem server has to free the
reference when client requests for it.

If client calls DRI2CreateDrawable twice for same drawable same
reference is reused and reference count is incremented.

Signed-off-by: Pauli Nieminenext-pauli.niemi...@nokia.com
CC: Kristian Høgsbergk...@bitplanet.net

---
  hw/xfree86/dri2/dri2.c|  100 +++-
  hw/xfree86/dri2/dri2.h|4 +-
  hw/xfree86/dri2/dri2ext.c |2 +-
  3 files changed, 83 insertions(+), 23 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 34f735f..103db1a 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -193,6 +193,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
  typedef struct DRI2DrawableRefRec {
  XID id;
  XID dri2_id;
+int  refcnt;
  DRI2InvalidateProcPtr invalidate;
  void   *priv;
  struct list link;
@@ -229,6 +230,7 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
dri2_id,

  ref-id = id;
  ref-dri2_id = dri2_id;
+ref-refcnt = 1;
  ref-invalidate = invalidate;
  ref-priv = priv;
  list_add(ref-link,pPriv-reference_list);
@@ -236,11 +238,24 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
dri2_id,
  return Success;
  }

+static DRI2DrawableRefPtr
+DRI2FindClientDrawableRef(ClientPtr client, DRI2DrawablePtr pPriv)
+{
+   DRI2DrawableRefPtr ref;
+
+   list_for_each_entry(ref,pPriv-reference_list, link) {
+   if (CLIENT_ID(ref-dri2_id) == client-index

[PATCH 0/4] Improve reporting of resource sizes.

[Rami Ylimäki]

The motivations of this patchset is to make xrestop and X resource
extension report resource sizes more accurately.

Currently xrestop is not taking pixmap references from extensions into
account. X server pixmap size reporting functions are improved to
include pixmaps referenced from render pictures and composite windows.

The server resource size functions are also abstracted so that size
calculation functions can be wrapped or overridden by drivers. For
example, it's useful to also report pixmaps referenced by DRI2
buffers, but this can be only done from drivers. These patches make it
possible to report extra pixmap references from drivers as well as
improve the default pixmap size calculation functions.

The xrestop application is currently estimating sizes of
non-predefined resources by constants because X resource extension
doesn't provide a way to query the sizes from X server. We'd like to
improve X resource extension in the near future so that the size of
any resource could be queried. We aren't planning to implement size
calculation for every resource. However, it'd be much better if
xrestop could first query the size of a resource from X server and
then fall back to estimating the size with a constant if X server
doesn't support size calculation of that particular resource. Then the
size calculation of most important resources, such as DRI2 drawables,
could be implemented in X server and X restop would be able to report
the size automatically.

Rami Ylimäki (4):
  dix: Provide means to report exact sizes of resources.
  dix: Add reverse resource name lookup function to registry.
  render: Report pixmap usage of pictures to resource extension.
  composite: Report pixmap usage of client windows to resource
extension.

 Xext/xres.c |   67 +
 composite/compext.c |   24 ++
 dix/registry.c  |   10 +++
 dix/resource.c  |  199 ++-
 include/registry.h  |6 ++
 include/resource.h  |   23 ++
 render/picture.c|   22 ++
 7 files changed, 318 insertions(+), 33 deletions(-)

___
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 2/4] dix: Add reverse resource name lookup function to registry.

[Rami Ylimäki]

Currently only some pre-defined resource types can be accessed from
headers. Make it possible to find out types of new resources that
aren't pre-defined but need to be registered separately.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 dix/registry.c |   10 ++
 include/registry.h |6 ++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/dix/registry.c b/dix/registry.c
index fc35dbb..15f85cc 100644
--- a/dix/registry.c
+++ b/dix/registry.c
@@ -273,6 +273,16 @@ LookupResourceName(RESTYPE resource)
 return resources[resource] ? resources[resource] : XREGISTRY_UNKNOWN;
 }
 
+RESTYPE
+LookupResourceType(const char *name)
+{
+RESTYPE resource = nresource;
+while (--resource  0)
+if (resources[resource]  !strcmp(resources[resource], name))
+return resource;
+return RT_NONE;
+}
+
 /*
  * Setup and teardown
  */
diff --git a/include/registry.h b/include/registry.h
index 325f765..99a3e65 100644
--- a/include/registry.h
+++ b/include/registry.h
@@ -41,6 +41,11 @@ extern _X_EXPORT const char *LookupErrorName(int error);
 extern _X_EXPORT const char *LookupResourceName(RESTYPE rtype);
 
 /*
+ * Reverse lookup functions.
+ */
+extern _X_EXPORT RESTYPE LookupResourceType(const char *name);
+
+/*
  * Setup and teardown
  */
 extern _X_EXPORT void dixResetRegistry(void);
@@ -57,6 +62,7 @@ extern _X_EXPORT void dixResetRegistry(void);
 #define LookupEventName(a) XREGISTRY_UNKNOWN
 #define LookupErrorName(a) XREGISTRY_UNKNOWN
 #define LookupResourceName(a) XREGISTRY_UNKNOWN
+#define LookupResourceType(a) RT_NONE
 
 #define dixResetRegistry() { ; }
 
-- 
1.6.3.3

___
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 3/4] render: Report pixmap usage of pictures to resource extension.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 Xext/xres.c  |   14 ++
 render/picture.c |   22 ++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/Xext/xres.c b/Xext/xres.c
index e2fd8a7..ae9735a 100644
--- a/Xext/xres.c
+++ b/Xext/xres.c
@@ -216,6 +216,13 @@ ResFindGCPixmaps (pointer value, XID id, pointer cdata)
 ResFindResourcePixmaps(value, id, RT_GC, cdata);
 }
 
+static RESTYPE RT_PICTURE = RT_NONE;
+static void
+ResFindPicturePixmaps (pointer value, XID id, pointer cdata)
+{
+ResFindResourcePixmaps(value, id, RT_PICTURE, cdata);
+}
+
 static int
 ProcXResQueryClientPixmapBytes (ClientPtr client)
 {
@@ -252,6 +259,13 @@ ProcXResQueryClientPixmapBytes (ClientPtr client)
  ResFindGCPixmaps, 
   (pointer)(bytes));
 
+/* Render extension picture pixmaps. */
+RT_PICTURE = LookupResourceType(PICTURE);
+if (RT_PICTURE != RT_NONE)
+FindClientResourcesByType(clients[clientID], RT_PICTURE,
+  ResFindPicturePixmaps,
+  (pointer)(bytes));
+
 #ifdef COMPOSITE
 /* FIXME: include composite pixmaps too */
 #endif
diff --git a/render/picture.c b/render/picture.c
index 7fda6b9..64d9b72 100644
--- a/render/picture.c
+++ b/render/picture.c
@@ -606,6 +606,27 @@ PictureParseCmapPolicy (const char *name)
return PictureCmapPolicyInvalid;
 }
 
+/** @see GetDefaultBytes */
+static void
+GetPictureBytes(pointer value, XID id, ResourceSizePtr size)
+{
+PicturePtr picture = value;
+
+/* Currently only pixmap bytes are reported to clients. */
+size-resourceSize = 0;
+
+/* Calculate pixmap reference sizes. */
+size-pixmapRefSize = 0;
+if (picture-pDrawable  (picture-pDrawable-type == DRAWABLE_PIXMAP))
+{
+SizeType pixmapSizeFunc = GetResourceTypeSizeFunc(RT_PIXMAP);
+ResourceSizeRec pixmapSize = { 0, 0 };
+PixmapPtr pixmap = (PixmapPtr)picture-pDrawable;
+pixmapSizeFunc(pixmap, pixmap-drawable.id, pixmapSize);
+size-pixmapRefSize += pixmapSize.pixmapRefSize;
+}
+}
+
 Bool
 PictureInit (ScreenPtr pScreen, PictFormatPtr formats, int nformats)
 {
@@ -618,6 +639,7 @@ PictureInit (ScreenPtr pScreen, PictFormatPtr formats, int 
nformats)
PictureType = CreateNewResourceType (FreePicture, PICTURE);
if (!PictureType)
return FALSE;
+   SetResourceTypeSizeFunc(PictureType, GetPictureBytes);
PictFormatType = CreateNewResourceType (FreePictFormat, PICTFORMAT);
if (!PictFormatType)
return FALSE;
-- 
1.6.3.3

___
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 4/4] composite: Report pixmap usage of client windows to resource extension.

[Rami Ylimäki]

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 Xext/xres.c |   16 +---
 composite/compext.c |   24 
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/Xext/xres.c b/Xext/xres.c
index ae9735a..0d626e6 100644
--- a/Xext/xres.c
+++ b/Xext/xres.c
@@ -223,6 +223,13 @@ ResFindPicturePixmaps (pointer value, XID id, pointer 
cdata)
 ResFindResourcePixmaps(value, id, RT_PICTURE, cdata);
 }
 
+static RESTYPE RT_COMPOSITE_CLIENT_WINDOW = RT_NONE;
+static void
+ResFindCompositeClientWindowPixmaps (pointer value, XID id, pointer cdata)
+{
+ResFindResourcePixmaps(value, id, RT_COMPOSITE_CLIENT_WINDOW, cdata);
+}
+
 static int
 ProcXResQueryClientPixmapBytes (ClientPtr client)
 {
@@ -266,9 +273,12 @@ ProcXResQueryClientPixmapBytes (ClientPtr client)
   ResFindPicturePixmaps,
   (pointer)(bytes));
 
-#ifdef COMPOSITE
-/* FIXME: include composite pixmaps too */
-#endif
+/* Composite extension client window pixmaps. */
+RT_COMPOSITE_CLIENT_WINDOW = LookupResourceType(CompositeClientWindow);
+if (RT_COMPOSITE_CLIENT_WINDOW != RT_NONE)
+FindClientResourcesByType(clients[clientID], 
RT_COMPOSITE_CLIENT_WINDOW,
+  ResFindCompositeClientWindowPixmaps,
+  (pointer)(bytes));
 
 rep.type = X_Reply;
 rep.sequenceNumber = client-sequence;
diff --git a/composite/compext.c b/composite/compext.c
index 30d9dc2..676b9a5 100644
--- a/composite/compext.c
+++ b/composite/compext.c
@@ -508,6 +508,28 @@ SProcCompositeDispatch (ClientPtr client)
return BadRequest;
 }
 
+/** @see GetDefaultBytes */
+static void
+GetCompositeClientWindowBytes(pointer value, XID id, ResourceSizePtr size)
+{
+WindowPtr window = value;
+
+/* Currently only pixmap bytes are reported to clients. */
+size-resourceSize = 0;
+
+/* Calculate pixmap reference sizes. */
+size-pixmapRefSize = 0;
+if (window-redirectDraw != RedirectDrawNone)
+{
+SizeType pixmapSizeFunc = GetResourceTypeSizeFunc(RT_PIXMAP);
+ResourceSizeRec pixmapSize = { 0, 0 };
+ScreenPtr screen = window-drawable.pScreen;
+PixmapPtr pixmap = screen-GetWindowPixmap(window);
+pixmapSizeFunc(pixmap, pixmap-drawable.id, pixmapSize);
+size-pixmapRefSize += pixmapSize.pixmapRefSize;
+}
+}
+
 void
 CompositeExtensionInit (void)
 {
@@ -547,6 +569,8 @@ CompositeExtensionInit (void)
(FreeCompositeClientWindow, CompositeClientWindow);
 if (!CompositeClientWindowType)
return;
+SetResourceTypeSizeFunc(CompositeClientWindowType,
+GetCompositeClientWindowBytes);
 
 CompositeClientSubwindowsType = CreateNewResourceType
(FreeCompositeClientSubwindows, CompositeClientSubwindows);
-- 
1.6.3.3

___
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 1/4] dix: Provide means to report exact sizes of resources.

[Rami Ylimäki]

 On 10/28/2010 04:48 PM, Alan Coopersmith wrote:

+/**
+ * Get the function used to calculate resource size. Extensions and
+ * drivers need to be able to determine the current size calculation
+ * function if they want to wrap or override it.
+ *
+ * @param[in] type Resource type used in size calculations.
+ *
+ * @return Function to calculate the size of a single
+ * resource.
+ */
+SizeType
+GetResourceTypeSizeFunc(RESTYPE type)
+{
+return resourceTypes[type  TypeMask].sizeFunc;
+}

Should this add dixPrivatesSize(type) to the result or should the callers
like Xresource be doing that?



The original intention was that Xresource wouldn't do that automatically 
and the functions returned by that getter would calculate the size as 
well as they can. The resourceSize field of ResourceSizeRec should be 
filled with the amount of memory that is freed when the resource doesn't 
exist anymore.


It's probably best to add sizeof(PixmapRec) and 
dixPrivatesSize(PIXMAP_PRIVATE) to the result in GetPixmapBytes to get 
better estimate for the size and also to make GetPixmapBytes an example 
for other size calculation functions. I can do that in v2 of the patch 
if this change seems sensible to you.


-- Rami

___
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

[PULL] revert arm backtrace fix, introduce client id tracking

[Rami Ylimäki]

 Hi Keith,

The following changes applied on top of current master branch 
(1a0d9324b3d9fd93e685066e0e5cea0611878c0d)


Revert Set DamageSetReportAfterOp to true for the damage 
extension (#30260)


are available at:

git://gitorious.org/rjy-fdo/xserver.git

Rami Ylimäki (3):
  Revert os: Prevent backtrace from being stopped in noreturn
functions.
  dix: Add facilities for client ID tracking.
  Xext: Use general OS functions to determine client command string in
SELinux.

 Xext/xselinux_hooks.c|   22 +--
 configure.ac |   28 ++--
 dix/Makefile.am  |1 +
 dix/client.c |  345 
++

 dix/main.c   |3 +
 hw/xfree86/loader/sdksyms.sh |1 +
 include/Makefile.am  |1 +
 include/client.h |   60 
 include/dix-config.h.in  |3 +
 include/os.h |3 +
 os/Makefile.am   |   17 +--
 os/access.c  |   76 +
 12 files changed, 517 insertions(+), 43 deletions(-)
 create mode 100644 dix/client.c
 create mode 100644 include/client.h

___
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: libx11 crash (possible PATCH attached)

[Rami Ylimäki]

 Hi,

Sounds a lot like an already fixed problem: 
http://cgit.freedesktop.org/xorg/lib/libX11/commit/?id=4b8ff7db39f2fe7ef12968d462aaf3f9054b6c18.


-- Rami

On 10/14/2010 04:30 PM, Peter Clifton wrote:

Hi,

I ran across a repeatable crash in libx11 when dealing with some broken
OpenGL code I was writing. Unfortunately, I've forgotten how to repeat
the crash (as it was related to some obscure buggy scenario in my GL
setup).

What I did note was the location / cause, and a patch which got me past
the crash. I don't know if the patch is correct though.

This was against the Ubuntu xorg-edgers package of:
libx11-1.3.4+git20100720.554da76e


diff -u xcb_io.c.old xcb_io.c
--- xcb_io.c.old2010-10-14 14:23:44.456669003 +0100
+++ xcb_io.c2010-10-14 14:24:45.642061004 +0100
@@ -559,7 +559,7 @@
ConditionBroadcast(dpy, dpy-xcb-reply_notify);
assert(XLIB_SEQUENCE_COMPARE(req-sequence,=, dpy-request));
dpy-last_request_read = req-sequence;
-   if(!response)
+   if(!response  (req != current))
dequeue_pending_request(dpy, req);

if(req == current)


Basically, the bug was that req was equal to current, and as
response was NULL, the response was dequeued, freeing the memory in
current.

After the loop, a check was made for if(event_sequence ==
current-sequence), which dereferenced the free'd current response,
and caused a crash.

I don't know if not dequeuing the current response is the correct fix,
or whether some logic should be applied to skip further processing in
this case.

I thought I'd pass on the investigation and my possible fix to those who
know more about this, and hopefully it will help improve libx11's
resilience. I'm fairly sure the situation I hit was a corner case
though, as I've never seen libx11 crash like this before, only in the
case where I has a problem with my GL code.

Please note that I don't have any way to reproduce this crash any more,
so won't be of any use testing patches for it.



___
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 v3 1/2] dix: Add facilities for client ID tracking.

[Rami Ylimäki]

 On 10/02/2010 02:00 AM, Jesse Adkins wrote:

+AC_ARG_ENABLE(clientids,  AS_HELP_STRING([--disable-clientids], [Build 
Xorg with client ID tracking (default: enabled)]), [CLIENTIDS=$enableval], 
[CLIENTIDS=yes])

Shouldn't that be --enable-clientids instead of --disable-clientids?


Hi,

Alan already told the difference of these options, but did you 
originally mean that the option should be disabled by default and 
therefore --enable-clientids should be used instead?


I'm about to start work on the client side and propose a new request for 
XResource extension for querying the local client PID. Therefore I think 
it makes sense to keep the feature enabled by default in X server. 
Having that enabled shouldn't affect X server performance in a 
significant way.


-- Rami

___
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 v3 1/2] dix: Add facilities for client ID tracking.

[Rami Ylimäki]

An interface is provided for figuring out the PID and process name of
a client. Make some existing functionality from SELinux and IA
extensions available for general use.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 configure.ac |   15 ++-
 dix/Makefile.am  |1 +
 dix/client.c |  345 ++
 dix/main.c   |3 +
 hw/xfree86/loader/sdksyms.sh |1 +
 include/Makefile.am  |1 +
 include/client.h |   60 
 include/dix-config.h.in  |3 +
 include/os.h |3 +
 os/access.c  |   76 +
 10 files changed, 507 insertions(+), 1 deletions(-)
 create mode 100644 dix/client.c
 create mode 100644 include/client.h

diff --git a/configure.ac b/configure.ac
index 1a1f2d3..056d68f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -645,6 +645,7 @@ AC_ARG_ENABLE(vbe,
AS_HELP_STRING([--enable-vbe], [Build Xorg with VB
 AC_ARG_ENABLE(int10-module, AS_HELP_STRING([--enable-int10-module], [Build 
Xorg with int10 module (default: enabled)]), [INT10MODULE=$enableval], 
[INT10MODULE=yes])
 AC_ARG_ENABLE(windowswm,  AS_HELP_STRING([--enable-windowswm], [Build XWin 
with WindowsWM extension (default: no)]), [WINDOWSWM=$enableval], 
[WINDOWSWM=no])
 AC_ARG_ENABLE(libdrm, AS_HELP_STRING([--enable-libdrm], [Build Xorg 
with libdrm support (default: enabled)]), [DRM=$enableval],[DRM=yes])
+AC_ARG_ENABLE(clientids,  AS_HELP_STRING([--disable-clientids], [Build 
Xorg with client ID tracking (default: enabled)]), [CLIENTIDS=$enableval], 
[CLIENTIDS=yes])
 
 dnl DDXes.
 AC_ARG_ENABLE(xorg,  AS_HELP_STRING([--enable-xorg], [Build 
Xorg server (default: auto)]), [XORG=$enableval], [XORG=auto])
@@ -999,6 +1000,18 @@ if test x$RES = xyes; then
REQUIRED_MODULES=$REQUIRED_MODULES $RESOURCEPROTO
 fi
 
+# The XRes extension may support client ID tracking only if it has
+# been specifically enabled. Client ID tracking is implicitly not
+# supported if XRes extension is disabled.
+AC_MSG_CHECKING([whether to track client ids])
+if test x$RES = xyes  test x$CLIENTIDS = xyes; then
+   AC_DEFINE(CLIENTIDS, 1, [Support client ID tracking])
+else
+   CLIENTIDS=no
+fi
+AC_MSG_RESULT([$CLIENTIDS])
+AM_CONDITIONAL(CLIENTIDS, [test x$CLIENTIDS = xyes])
+
 if test x$GLX = xyes; then
PKG_CHECK_MODULES([XLIB], [x11])
PKG_CHECK_MODULES([GL], $GLPROTO $LIBGL)
@@ -1527,7 +1540,7 @@ if test x$XNEST = xyes; then
if test x$have_xnest = xno; then
AC_MSG_ERROR([Xnest build explicitly requested, but required 
modules not found.])
fi
-   XNEST_LIBS=$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB 
$GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $MIEXT_DAMAGE_LIB 
$MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $DIX_LIB 
$MAIN_LIB $OS_LIB
+   XNEST_LIBS=$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB 
$GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $MIEXT_DAMAGE_LIB 
$MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $MAIN_LIB 
$DIX_LIB $OS_LIB
XNEST_SYS_LIBS=$XNESTMODULES_LIBS $GLX_SYS_LIBS
AC_SUBST([XNEST_LIBS])
AC_SUBST([XNEST_SYS_LIBS])
diff --git a/dix/Makefile.am b/dix/Makefile.am
index 5e2dad7..49e41d0 100644
--- a/dix/Makefile.am
+++ b/dix/Makefile.am
@@ -8,6 +8,7 @@ libmain_la_SOURCES =\
 libdix_la_SOURCES =\
atom.c  \
colormap.c  \
+   client.c\
cursor.c\
deprecated.c\
devices.c   \
diff --git a/dix/client.c b/dix/client.c
new file mode 100644
index 000..1016a56
--- /dev/null
+++ b/dix/client.c
@@ -0,0 +1,345 @@
+/*
+ * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies). All
+ * rights reserved.
+ * Copyright (c) 1993, 2010, Oracle and/or its affiliates. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION

[PATCH v3 2/2] Xext: Use general OS functions to determine client command string in SELinux.

[Rami Ylimäki]

SELinux could be also modified to take advantage of client ID tracking
but we don't introduce that dependency here. People interested in
SELinux are free to add that dependency later.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 Xext/xselinux_hooks.c |   22 +-
 1 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/Xext/xselinux_hooks.c b/Xext/xselinux_hooks.c
index 560e1e9..73a1179 100644
--- a/Xext/xselinux_hooks.c
+++ b/Xext/xselinux_hooks.c
@@ -129,26 +129,14 @@ SELinuxLabelClient(ClientPtr client)
 
 /* For local clients, try and determine the executable name */
 if (XaceIsLocal(client)) {
-   struct ucred creds;
-   socklen_t len = sizeof(creds);
-   char path[PATH_MAX + 1];
-   size_t bytes;
+   pid_t pid = GetPidFromClient(client);
+   const char *cmd = GetCommandFromPid(pid);
 
-   memset(creds, 0, sizeof(creds));
-   if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, creds, len)  0)
+   if (!cmd)
goto finish;
 
-   snprintf(path, PATH_MAX + 1, /proc/%d/cmdline, creds.pid);
-   fd = open(path, O_RDONLY);
-   if (fd  0)
-   goto finish;
-
-   bytes = read(fd, path, PATH_MAX + 1);
-   close(fd);
-   if (bytes = 0)
-   goto finish;
-
-   strncpy(subj-command, path, COMMAND_LEN - 1);
+   strncpy(subj-command, cmd, COMMAND_LEN - 1);
+   free((void *) cmd);
 }
 
 finish:
-- 
1.6.3.3

___
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 v3 0/2] xserver: Client ID tracking

[Rami Ylimäki]

Fixes based on comments of various reviewers.

v2
--
Mark Kettenis:
move /proc specific code into os
treat pid -1 as error instead of 0
Tiago Vignatti:
preserve copyrights
amend authorship
add description of the framework
use doxygen style
rename cmd - cmdline in ClientIdsPrivateRec
compile only if XRES or XSELINUX is enabled

v3
--
Fernando Carrijo:
don't use -o or -a with test shell command
Tiago Vignatti:
use doxygen @file directive
move pid_t related functions into os
remove unnecessary header inclusions
let client ID tracking be disabled with --disable-clientids
create dummy stubs for initialization functions if disabled
don't make XSELinux depend on client ID tracking
squash copyrights into main commit
don't add copyright for restructuring of minor code segments

Review comments not respected:
don't install client ID tracking headers
Rationale:
As soon as client ID tracking is incorporated into X server, it
will be used from debugging code of at least one video driver.

Rami Ylimäki (2):
  dix: Add facilities for client ID tracking.
  Xext: Use general OS functions to determine client command string in
SELinux.

 Xext/xselinux_hooks.c|   22 +--
 configure.ac |   15 ++-
 dix/Makefile.am  |1 +
 dix/client.c |  345 ++
 dix/main.c   |3 +
 hw/xfree86/loader/sdksyms.sh |1 +
 include/Makefile.am  |1 +
 include/client.h |   60 
 include/dix-config.h.in  |3 +
 include/os.h |3 +
 os/access.c  |   76 +
 11 files changed, 512 insertions(+), 18 deletions(-)
 create mode 100644 dix/client.c
 create mode 100644 include/client.h

___
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: problem with multithreaded application and Xlib

[Rami Ylimäki]

 On 09/30/2010 06:44 PM, Francesco Abbate wrote:

What happens is that the secondary thread blocks during the XSync
operation until the main thread receive some events.



Hi,

XNextEvent may call _XReadEvents and XSync calls _XReply. If XNextEvent 
is called before XSync from a different thread and there are no events, 
this may happen. It's a bug in Xlib implementation. Search for FIXME 
in http://cgit.freedesktop.org/xorg/lib/libX11/tree/src/xcb_io.c. I 
don't know if you are facing this problem but based on your description 
that may be the case.



Best regards,
Francesco



-- Rami

___
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 libXtst] Allow more than 6 axes to be sent.

[Rami Ylimäki]

From: Tobias Koch tobias.k...@nokia.com

From: Tobias Koch tobias.k...@nokia.com

If the number of axes exceeds 6, X server will return BadValue for
XTestFakeInput because the number of axes in a single DeviceValuator
event is incorrectly set to the total number of axes.

Signed-off-by: Tobias Koch tobias.k...@nokia.com
Reviewed-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 src/XTest.c |6 ++
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/XTest.c b/src/XTest.c
index c04cc09..24ae8b9 100644
--- a/src/XTest.c
+++ b/src/XTest.c
@@ -267,12 +267,10 @@ send_axes(
 req-length += ((n_axes + 5) / 6) * (SIZEOF(xEvent)  2);
 ev.type = XI_DeviceValuator + (long)info-data;
 ev.deviceid = dev-device_id;
-ev.num_valuators = n_axes;
 ev.first_valuator = first_axis;
 while (n_axes  0) {
-   n = n_axes;
-   if (n  6)
-   n = 6;
+   n = n_axes  6 ? 6 : n_axes;
+   ev.num_valuators = n;
switch (n) {
case 6:
ev.valuator5 = *(axes+5);
-- 
1.6.3.3

___
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 v2 0/5] xserver: Client ID tracking

[Rami Ylimäki]

Fixes based on comments of various reviewers.

Mark Kettenis:
  move /proc specific code into os
  treat pid -1 as error instead of 0
Tiago Vignatti:
  preserve copyrights
  amend authorship
  add description of the framework
  use doxygen style
  rename cmd - cmdline in ClientIdsPrivateRec
  compile only if XRES or XSELINUX is enabled

Rami Ylimäki (5):
  dix: Add facilities for client ID tracking.
  dix: Enable client ID tracking in server.
  Xext: Enable client ID tracking in extensions (SELinux).
  dix: Fix copyrights.
  os: Fix copyrights.

 Xext/xselinux_hooks.c|   37 +
 configure.ac |   12 ++-
 dix/Makefile.am  |4 +
 dix/client.c |  339 ++
 dix/main.c   |7 +
 hw/xfree86/loader/sdksyms.sh |3 +
 include/Makefile.am  |1 +
 include/client.h |   54 +++
 include/os.h |2 +
 os/access.c  |   47 ++
 10 files changed, 476 insertions(+), 30 deletions(-)
 create mode 100644 dix/client.c
 create mode 100644 include/client.h

___
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 v2 1/5] dix: Add facilities for client ID tracking.

[Rami Ylimäki]

An interface is provided for figuring out the PID and process name of
a client. Make some existing functionality from SELinux and IA
extensions available for general use.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 dix/Makefile.am  |1 +
 dix/client.c |  338 ++
 include/client.h |   54 +
 include/os.h |2 +
 os/access.c  |   46 
 5 files changed, 441 insertions(+), 0 deletions(-)
 create mode 100644 dix/client.c
 create mode 100644 include/client.h

diff --git a/dix/Makefile.am b/dix/Makefile.am
index 5e2dad7..bc87da5 100644
--- a/dix/Makefile.am
+++ b/dix/Makefile.am
@@ -7,6 +7,7 @@ libmain_la_SOURCES =\
 
 libdix_la_SOURCES =\
atom.c  \
+   client.c\
colormap.c  \
cursor.c\
deprecated.c\
diff --git a/dix/client.c b/dix/client.c
new file mode 100644
index 000..99152a4
--- /dev/null
+++ b/dix/client.c
@@ -0,0 +1,338 @@
+/*
+ * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies). All
+ * rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ * Author: Rami Ylimäki rami.ylim...@vincit.fi
+ *
+ * This file contains functionality for identifying clients by various
+ * means. The primary purpose of identification is to simply aid in
+ * finding out which clients are using X server and how they are using
+ * it. For example, it's often necessary to monitor what requests
+ * clients are executing (to spot bad behaviour) and how they are
+ * allocating resources in X server (to spot excessive resource
+ * usage).
+ *
+ * This framework automatically allocates information, that can be
+ * used for client identification, when a client connects to the
+ * server. The information is freed when the client disconnects. The
+ * allocated information is just a collection of various IDs, such as
+ * PID and process name for local clients, that are likely to be
+ * useful in analyzing X server usage.
+ *
+ * Users of the framework can query ID information about clients at
+ * any time. To avoid repeated polling of IDs the users can also
+ * subscribe for notifications about the availability of ID
+ * information. Use GetClient* to query information and
+ * GetClientIds*Cbs to register for notifications.
+ */
+
+#include stdlib.h
+#include unistd.h
+
+#include os.h
+#include client.h
+#include dixstruct.h
+
+/* Key for identifying ID information for a client. */
+static DevPrivateKeyRec ClientIdsPrivKeyRec;
+static DevPrivateKey ClientIdsPrivKey = ClientIdsPrivKeyRec;
+
+/**
+ * @return Client private holding PID and command line string. Error
+ * (NULL) if PID is not available for the client.
+ */
+ClientIdsPrivatePtr GetClientIds(ClientPtr client)
+{
+PrivatePtr *privates = (client)-devPrivates;
+pointer priv = dixLookupPrivate(privates, ClientIdsPrivKey);
+return (ClientIdsPrivatePtr) priv;
+}
+
+/* Called after PID and command line string have been determined for a
+ * client (all clients, including remote clients, except server
+ * client). You may call GetClientPid and GetClientCmd after this
+ * notification. */
+static CallbackListPtr ClientIdsReservedCbs = NULL;
+
+/**
+ * @return Publisher of client ID allocation notifications.
+ *
+ * @see AddCallback
+ */
+CallbackListPtr *GetClientIdsReservedCbs(void)
+{
+return ClientIdsReservedCbs;
+}
+
+/* Called before PID and command line string will be invalidated for a
+ * client (all clients, including remote clients, except server
+ * client). GetClientPid and GetClientCmd will return errors when
+ * called after this notification. */
+static CallbackListPtr ClientIdsReleasedCbs = NULL;
+
+/**
+ * @return Publisher of client ID deallocation notifications.
+ *
+ * @see AddCallback
+ */
+CallbackListPtr *GetClientIdsReleasedCbs(void)
+{
+return ClientIdsReleasedCbs;
+}
+
+/**
+ * Try to determine a PID for a client

[PATCH v2 2/5] dix: Enable client ID tracking in server.

[Rami Ylimäki]

Let X server to keep track of client PIDs and process names. Also make
the client tracking interface available for external modules. Linking
order of Xnest libraries needs to be fixed, because libmain depends on
libdix and not vice versa.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 configure.ac |   12 +++-
 dix/Makefile.am  |5 -
 dix/main.c   |7 +++
 hw/xfree86/loader/sdksyms.sh |3 +++
 include/Makefile.am  |1 +
 5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1a1f2d3..586df80 100644
--- a/configure.ac
+++ b/configure.ac
@@ -999,6 +999,16 @@ if test x$RES = xyes; then
REQUIRED_MODULES=$REQUIRED_MODULES $RESOURCEPROTO
 fi
 
+AC_MSG_CHECKING([whether to track client ids])
+if test x$RES = xyes -o x$XSELINUX = xyes; then
+   CLIENTIDS=yes
+   AC_DEFINE(CLIENTIDS, 1, [Support client id tracking])
+else
+   CLIENTIDS=no
+fi
+AC_MSG_RESULT([$CLIENTIDS])
+AM_CONDITIONAL(CLIENTIDS, [test x$CLIENTIDS = xyes])
+
 if test x$GLX = xyes; then
PKG_CHECK_MODULES([XLIB], [x11])
PKG_CHECK_MODULES([GL], $GLPROTO $LIBGL)
@@ -1527,7 +1537,7 @@ if test x$XNEST = xyes; then
if test x$have_xnest = xno; then
AC_MSG_ERROR([Xnest build explicitly requested, but required 
modules not found.])
fi
-   XNEST_LIBS=$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB 
$GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $MIEXT_DAMAGE_LIB 
$MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $DIX_LIB 
$MAIN_LIB $OS_LIB
+   XNEST_LIBS=$FB_LIB $FIXES_LIB $MI_LIB $XEXT_LIB $DBE_LIB $RECORD_LIB 
$GLX_LIBS $RANDR_LIB $RENDER_LIB $DAMAGE_LIB $MIEXT_DAMAGE_LIB 
$MIEXT_SHADOW_LIB $XI_LIB $XKB_LIB $XKB_STUB_LIB $COMPOSITE_LIB $MAIN_LIB 
$DIX_LIB $OS_LIB
XNEST_SYS_LIBS=$XNESTMODULES_LIBS $GLX_SYS_LIBS
AC_SUBST([XNEST_LIBS])
AC_SUBST([XNEST_SYS_LIBS])
diff --git a/dix/Makefile.am b/dix/Makefile.am
index bc87da5..a74e219 100644
--- a/dix/Makefile.am
+++ b/dix/Makefile.am
@@ -7,7 +7,6 @@ libmain_la_SOURCES =\
 
 libdix_la_SOURCES =\
atom.c  \
-   client.c\
colormap.c  \
cursor.c\
deprecated.c\
@@ -42,6 +41,10 @@ libdix_la_SOURCES =  \
tables.c\
window.c
 
+if CLIENTIDS
+libdix_la_SOURCES += client.c
+endif
+
 EXTRA_DIST = buildatoms BuiltInAtoms Xserver.d Xserver-dtrace.h.in
 
 # Install list of protocol names
diff --git a/dix/main.c b/dix/main.c
index 5c46dc1..3362058 100644
--- a/dix/main.c
+++ b/dix/main.c
@@ -104,6 +104,7 @@ Equipment Corporation.
 #include extnsionst.h
 #include privates.h
 #include registry.h
+#include client.h
 #ifdef PANORAMIX
 #include panoramiXsrv.h
 #else
@@ -260,6 +261,9 @@ int main(int argc, char *argv[], char *envp[])
 InitCoreDevices();
InitInput(argc, argv);
InitAndStartDevices();
+#ifdef CLIENTIDS
+   InitClientIds(serverClient);
+#endif /* CLIENTIDS */
 
dixSaveScreens(serverClient, SCREEN_SAVER_FORCER, ScreenSaverReset);
 
@@ -325,6 +329,9 @@ int main(int argc, char *argv[], char *envp[])
screenInfo.numScreens = i;
}
 
+#ifdef CLIENTIDS
+   CloseClientIds(serverClient);
+#endif /* CLIENTIDS */
dixFreePrivates(serverClient-devPrivates, PRIVATE_CLIENT);
serverClient-devPrivates = NULL;
 
diff --git a/hw/xfree86/loader/sdksyms.sh b/hw/xfree86/loader/sdksyms.sh
index 13c5ae5..8da4341 100755
--- a/hw/xfree86/loader/sdksyms.sh
+++ b/hw/xfree86/loader/sdksyms.sh
@@ -264,6 +264,9 @@ cat  sdksyms.c  EOF
 #include colormap.h
 #include colormapst.h
 #include hotplug.h
+#if CLIENTIDS
+#include client.h
+#endif
 #include cursor.h
 #include cursorstr.h
 #include dix.h
diff --git a/include/Makefile.am b/include/Makefile.am
index e76de05..06cf46f 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -4,6 +4,7 @@ sdk_HEADERS =   \
bstore.h\
bstorestr.h \
callback.h  \
+   client.h\
closestr.h  \
closure.h   \
colormap.h  \
-- 
1.6.3.3

___
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 v2 3/5] Xext: Enable client ID tracking in extensions (SELinux).

[Rami Ylimäki]

Make SELinux use the public interface for querying client command
string. SELinux could be optimized further by removing its own copy of
the command string. However, that optimization has been left out for
simplicity.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 Xext/xselinux_hooks.c |   37 -
 1 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/Xext/xselinux_hooks.c b/Xext/xselinux_hooks.c
index 560e1e9..baca533 100644
--- a/Xext/xselinux_hooks.c
+++ b/Xext/xselinux_hooks.c
@@ -33,6 +33,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN 
THE SOFTWARE.
 #include libaudit.h
 
 #include X11/Xatom.h
+#include client.h
 #include selection.h
 #include inputstr.h
 #include scrnintstr.h
@@ -129,26 +130,12 @@ SELinuxLabelClient(ClientPtr client)
 
 /* For local clients, try and determine the executable name */
 if (XaceIsLocal(client)) {
-   struct ucred creds;
-   socklen_t len = sizeof(creds);
-   char path[PATH_MAX + 1];
-   size_t bytes;
+   const char *cmd = GetClientCmd(client);
 
-   memset(creds, 0, sizeof(creds));
-   if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, creds, len)  0)
+   if (!cmd)
goto finish;
 
-   snprintf(path, PATH_MAX + 1, /proc/%d/cmdline, creds.pid);
-   fd = open(path, O_RDONLY);
-   if (fd  0)
-   goto finish;
-
-   bytes = read(fd, path, PATH_MAX + 1);
-   close(fd);
-   if (bytes = 0)
-   goto finish;
-
-   strncpy(subj-command, path, COMMAND_LEN - 1);
+   strncpy(subj-command, cmd, COMMAND_LEN - 1);
 }
 
 finish:
@@ -741,16 +728,8 @@ SELinuxServer(CallbackListPtr *pcbl, pointer unused, 
pointer calldata)
 static void
 SELinuxClientState(CallbackListPtr *pcbl, pointer unused, pointer calldata)
 {
-NewClientInfoRec *pci = calldata;
-
-switch (pci-client-clientState) {
-case ClientStateInitial:
-   SELinuxLabelClient(pci-client);
-   break;
-
-default:
-   break;
-}
+ClientPtr client = calldata;
+SELinuxLabelClient(client);
 }
 
 static void
@@ -819,7 +798,7 @@ void
 SELinuxFlaskReset(void)
 {
 /* Unregister callbacks */
-DeleteCallback(ClientStateCallback, SELinuxClientState, NULL);
+DeleteCallback(GetClientIdsReservedCbs(), SELinuxClientState, NULL);
 DeleteCallback(ResourceStateCallback, SELinuxResourceState, NULL);
 
 XaceDeleteCallback(XACE_EXT_DISPATCH, SELinuxExtension, NULL);
@@ -912,7 +891,7 @@ SELinuxFlaskInit(void)
NULL);
 
 /* Register callbacks */
-ret = AddCallback(ClientStateCallback, SELinuxClientState, NULL);
+ret = AddCallback(GetClientIdsReservedCbs(), SELinuxClientState, NULL);
 ret = AddCallback(ResourceStateCallback, SELinuxResourceState, NULL);
 
 ret = XaceRegisterCallback(XACE_EXT_DISPATCH, SELinuxExtension, NULL);
-- 
1.6.3.3

___
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 v2 4/5] dix: Fix copyrights.

[Rami Ylimäki]

Some ideas and lines of code were taken from the Interactive Server
(IA) extension.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 dix/client.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/dix/client.c b/dix/client.c
index 99152a4..7b46665 100644
--- a/dix/client.c
+++ b/dix/client.c
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies). All
  * rights reserved.
+ * Copyright (c) 1993, 2010, Oracle and/or its affiliates. All rights reserved.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the Software), to 
deal
-- 
1.6.3.3

___
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 v2 5/5] os: Fix copyrights.

[Rami Ylimäki]

Contents of GetCommandFromPid are based on code from
SELinuxLabelClient.

Signed-off-by: Rami Ylimäki rami.ylim...@vincit.fi
---
 os/access.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/os/access.c b/os/access.c
index ce96436..6d03ea4 100644
--- a/os/access.c
+++ b/os/access.c
@@ -55,6 +55,7 @@ SOFTWARE.
 
 /*
  * Copyright © 2004 Sun Microsystems, Inc.  All rights reserved.
+ * Copyright (c) 2005 by Trusted Computer Solutions, Inc. All rights reserved.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the Software),
-- 
1.6.3.3

___
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 0/3] xserver: Client ID tracking

[Rami Ylimäki]

 On 08/30/2010 09:11 PM, Mark Kettenis wrote:

From: =?utf-8?q?Rami=20Ylim=C3=A4ki?=rami.ylim...@vincit.fi
Date: Mon, 30 Aug 2010 16:29:30 +0300

Hi,

These patches add a simple framework for determining client related
IDs such as PID and process name within X server. After these changes
have been reviewed and accepted, the XRes extension will be modified
to use the framework and provide client ID information for clients
with a new protocol request.

How useful is this really, given that information from remote clients
isn't available, or at least cannot be trusted?


We have used this kind of code repeatedly in X server to identify local 
clients that are behaving badly either by allocating too much memory and 
resources in the server or by executing deprecated requests. Maintaining 
private code to do this simply doesn't make sense anymore. Having the 
code in X server by default would speed up development and debugging 
time for us and also ease our maintenance burden.


We want to make also it possible to identify local clients outside of X 
server. That's why we need a new request to query the PID information 
reliably. It should be possible to easily track down problems on a 
release version of a distribution without having to recompile a debug 
version of X server first. Ultimately we want to use cnee to identify 
clients that are executing deprecated requests and xrestop to identify 
clients allocating too much resources. Currently there is no reliable 
way to do this.


You are right in that we mainly care about local clients, because in 
practice on our system all clients will be local. But I think that 
having a good way to identify local clients is still very important even 
though we wouldn't be able to reliably identify remote clients. We don't 
mind if information about clients can't be trusted in a rogue 
environment, because the framework would be used mainly to improve the 
overall system quality and as an development aid, not as a secure client 
framework for finding out which clients are trusted.


-- Rami

___
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