fix crashing when Xnamespace is enabled and a minor improvement #1077

mahiro21h · 2025-09-17T09:36:42Z

currently, the xserver can crash when running with xnamespace extension enabled when closing clients. this is due to the client's namespace being cleared when the client closes but before other code is done using its value. this pr solves this by adding a new client state before ClientStateGone and having all other code use this new state. this way xnamespace can clear the client's namespace after the other code is done using its value.

this pr also changes code in xnamespace to print everything into the xserver's log by having all XNS log macros call LogMessage() instead and replacing all instances of printf with XNS_LOG().

closes: #486

metux · 2025-09-17T09:59:56Z

include/dixstruct.h

    unsigned int clientGone:1;
    unsigned int closeDownMode:2;
-    unsigned int clientState:2;
+    unsigned int clientState:3;


oh, damn, I'm not sure whether we can do that, because it might change ABI.

maybe we can get away with this in that particular case, because we don't have more of those bits-limited fields after it - smart_priority is a usual char, so I'd guess it's going into next storage unit. but I'm not entirely sure right now.

@HaplessIdiot @algrid @dec05eba @X11Libre/reviewers @X11Libre/wranglers how do you think about this ?

Posix says CHAR_BIT == 8.
Before this change, the struct layout was:

char unsigned int :1; unsigned int :1; unsigned int :1; unsigned int :1; unsigned int :2; unsigned int :2; signed char

Now, it's:

char unsigned int :1; unsigned int :1; unsigned int :1; unsigned int :1; unsigned int :2; unsigned int :3; /* 1 bit more than before */ unsigned int :7; /* extra padding */ signed char

This breaks abi.

Do X clients need to care about this abi, or is this just for modules?

Damn that painful and to make problem worse these fields count up to 8 bits (char) :/ And testing on linux 64 bit platform is very bad: smart_priority moves to next alignment unit... :

#clientState:2 cc base=0x7fffffffd490 majorop=0x7fffffffd4c6 minorop=0x7fffffffd4c7 smart_priority=0x7fffffffd4c9 #clientState3: cc base=0x7fffffffd490 majorop=0x7fffffffd4c6 minorop=0x7fffffffd4c7 smart_priority=0x7fffffffd4ca

it generated from code:

diff --git a/dix/main.c b/dix/main.c index 5a9e515cc..3da4a31f8 100644 --- a/dix/main.c +++ b/dix/main.c @@ -130,6 +130,12 @@ CallbackListPtr PostInitRootWindowCallback = NULL; int dix_main(int argc, char *argv[], char *envp[]) { + + struct _Client c1; + + printf("cc base=%p majorop=%p minorop=%p smart_priority=%p\n",&c1,&c1.majorOp,&c1.minorOp,&c1.smart_priority); + fflush(stdout); + int i; HWEventQueueType alwaysCheckForInput[2];

Only one my suggestion is to add field at end of struct and when ClientStateGone is set, then check by last field state, for example ClientStateGone_Gone or ClientStateGone_Closing (it will complicate code but leave ABI intact)

Maybe the dixprivate api can be used to avoid breaking abi?

because it might change ABI

i can't believe i didn't think of that, you're right. it's probably safer to find another alternative.

just saw you guys' replies. metux's solution of adding a new filed to NewClientInfoRec appears to work fine for now. it doesn't require that many changes too, so i'm going to go with it. the new client state can be added at a later time.

This breaks abi.

Do X clients need to care about this abi, or is this just for modules?

Not the clients, but drivers. Especially Nvidia. I have no idea whether they're accessing any field down below, but that sounds like an actual risk.

metux · 2025-09-17T10:05:04Z

hmm, maybe it's better adding action field to NewClientInfoRec telling what really happens, so we look at this instead of the current state. And then add another call right at the place where the client is destroyed

mahiro21h · 2025-09-17T10:39:08Z

sounds good to me for now. i'll try and see what happens.

mahiro21h · 2025-09-17T12:15:58Z

@metux is it all good now?

metux · 2025-09-17T14:42:15Z

Xext/namespace/config.c

            for (int i=0; i<at->authTokenLen; i++)
-                printf("%02X", (unsigned char)at->authTokenData[i]);
-            printf("\"\n");
+                XNS_LOG("%02X", (unsigned char)at->authTokenData[i]);


this would move each single output byte into a new line ... is that really intended ?

no, it's not. i noticed the bytes being outputted incorrectly and was going to undo this change while rebasing but i forgot. i'll undo it later.

metux · 2025-09-17T14:44:27Z

include/dixstruct.h

    ClientPtr client;
    xConnSetupPrefix *prefix;
    xConnSetup *setup;
+    Bool action; /* added temporarily for use by xnamespace only */


why not an enum with all possible things that could have happened ?
callees then wouldn't look at the client's state bits anymore, but instead this enum.

i did this because it's supposed to be a temporary fix until we can add a new client state. it's not supposed to replace the client state. did i misunderstand what you wanted when you suggested adding action field?

my idea was adding an field in here telling the actual reason for the hook to be called.

ok i'll fix it

xnamespace is currently the only code that needs to do its work at a later time because other code relies on it when sending window unmap events for example. so we will add a special field `state` in NewClientInfoRec for it. Signed-off-by: Raysan Al-Awami <raysanalawami@gmail.com>

check param->state instead of checking the client's state. also, only clear namespace when param->state is NewClientInfoStateFreedResources. Signed-off-by: Raysan Al-Awami <raysanalawami@gmail.com>

Signed-off-by: Raysan Al-Awami <raysanalawami@gmail.com>

metux · 2025-09-18T08:00:51Z

include/dixstruct.h

    ClientPtr client;
    xConnSetupPrefix *prefix;
    xConnSetup *setup;
+    unsigned int state:3; /* added temporarily for use by xnamespace only */


why not the enum ?

i wanted to do it, but i decided to remain consistent with the rest of the server's code. afaik the server's code defines fields that are supposed to be set using enums as bit-fields.

metux · 2025-09-18T08:00:52Z

include/dixstruct.h

+    NewClientInfoStateRetained,
+    NewClientInfoStateGone,
+    NewClientInfoStateFreedResources
+} NewClientInfoState;


let's better call it something with "action" or "event", etc.
the idea: this shall tell what's happening right now - IOW: what's the reason for this hook to be called at all.

OR: add an entirely new hook that's called just right before a clientrec is actually destroyed. maybe that's the even better way to do this.

metux · 2025-09-18T08:03:16Z

after sleeping over this, I'm thinking we should do it a much simpler way: just add new new callback that's fired when a client is actually destroyed, and leave the existing hook alone.

Existing client-state hook isn't sufficient for this, and so easy to be extended cleanly (*1). Adding a new callback is trivial and cheap, so preferring this way, instead of trying to tweak the existing hook for something it's never been designed for. *1) see discussion here: #1077 Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>

metux · 2025-09-18T16:33:20Z

Here's a very simple and straightforward approach: just adding a new callback.

#1090

Once that's merged, we easily use it for fixing this bug - no need to ever tinker with client-state's

metux · 2025-09-18T17:57:14Z

Here's a very simple and straightforward approach: just adding a new callback.

#1090

Once that's merged, we easily use it for fixing this bug - no need to ever tinker with client-state's

Hacked up a quick fix based on this in branch wip/client-destroy-hook - haven't had the time to really test it yet.

Existing client-state hook isn't sufficient for this, and so easy to be extended cleanly (*1). Adding a new callback is trivial and cheap, so preferring this way, instead of trying to tweak the existing hook for something it's never been designed for. *1) see discussion here: #1077 Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>

metux · 2025-09-22T11:02:07Z

Here's a very simple and straightforward approach: just adding a new callback.

#1090

Once that's merged, we easily use it for fixing this bug - no need to ever tinker with client-state's

It's merged now, so we can use it.

Existing client-state hook isn't sufficient for this, and so easy to be extended cleanly (*1). Adding a new callback is trivial and cheap, so preferring this way, instead of trying to tweak the existing hook for something it's never been designed for. *1) see discussion here: #1077 Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>

metux · 2025-09-26T15:11:45Z

@mahiro21h needs rebase and resolving conflicts.
perhaps the issue already is resolved by recent commits (new callback)

github-actions · 2025-09-29T09:19:27Z

Merge Conflict found

metux reviewed Sep 17, 2025

View reviewed changes

mahiro21h force-pushed the xnamespace-fixes branch from 8689316 to d64225d Compare September 17, 2025 12:14

metux reviewed Sep 17, 2025

View reviewed changes

mahiro21h added 4 commits September 17, 2025 21:36

Xext: use NewClientInfoState and change logic in hookClientState()

19a4182

check param->state instead of checking the client's state. also, only clear namespace when param->state is NewClientInfoStateFreedResources. Signed-off-by: Raysan Al-Awami <raysanalawami@gmail.com>

dix: use state field in CloseDownClient()

e8cee56

Signed-off-by: Raysan Al-Awami <raysanalawami@gmail.com>

Xext: use LogMessage() in XNS logging macros

702d1eb

Signed-off-by: Raysan Al-Awami <raysanalawami@gmail.com>

mahiro21h force-pushed the xnamespace-fixes branch from d64225d to 702d1eb Compare September 17, 2025 19:20

metux reviewed Sep 18, 2025

View reviewed changes

metux mentioned this pull request Sep 18, 2025

dix: add callback before ClientRec is being destroyed #1090

Merged

fix crashing when Xnamespace is enabled and a minor improvement #1077

Are you sure you want to change the base?

fix crashing when Xnamespace is enabled and a minor improvement #1077

Uh oh!

Conversation

mahiro21h commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cepelinas9000 Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metux commented Sep 17, 2025

Uh oh!

mahiro21h commented Sep 17, 2025

Uh oh!

mahiro21h commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metux commented Sep 18, 2025

Uh oh!

metux commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metux commented Sep 18, 2025

Uh oh!

metux commented Sep 22, 2025

Uh oh!

metux commented Sep 26, 2025

Uh oh!

github-actions bot commented Sep 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cepelinas9000 Sep 17, 2025 •

edited

Loading

metux commented Sep 18, 2025 •

edited

Loading