I'd be happy to accept a patch to fix this problem.

Created attachment 95999 [details] [review]
patch to update number of current sessions before decide if we have to reject it

Well, it was pretty easy. We only need to recount sessions in the handle request function. 

I did test it, and I confirm that it fix the bug. 

Glad to hear you were able to figure out a workaround.  However, I'm a bit confused about how this works, and not sure that your patch is the "right" way to fix this.

note in gdm_xdmcp_handle_request, a few lines farther down we call gdm_xdmcp_displays_purge

This function loops over the displays and should dispose of any pending displays.
It calls do_dispose, which calls gdm_xdmcp_recount_sessions.  

So, is the problem that in your case, the call to gdm_xdmcp_displays_purge is happening too late in gdm_xdmcp_handle_request, or is the problem that the gdm_xdmcp_displays_purge isn't catching the case you have identified?

It doesn't seem to make sense to add logic to do the recount, and then call recount again later if there are pending displays.  It seems better to make the logic work so that the existing logic which tries to purge displays works in your case also.  No? 

Would you mind looking into this a bit more and see if you can understand why the existing logic in gdm_xdmcp_displays_purge isn't catching your case and fixing it?

Thanks for the comment. 

The problem is that calling gdm_xdmcp_displays_purge doesn't call do_dispose in every condition, only if there are pending displays that are expired :

static void
gdm_xdmcp_displays_purge (GdmXdmcpManager *manager)
[...]
if (d != NULL &&
    SERVER_IS_XDMCP (d) &&
    d->dispstat == XDMCP_PENDING &&
    curtime > d->acctime + manager->priv->max_wait) {
        g_debug ("gdm_xdmcp_displays_purge: Disposing session id %ld",
                 (long)d->sessionid);
        do_dispose (manager, d);
[...]

For this reason, the call to gdm_xdmcp_recount_sessions is necessary to make sure that num_sessions is updated appropriately before decide to accept or reject the request. 

Since gdm_xdmcp_displays_purge does usualy nothing, then it's not a big performance problem to call gdm_xdmcp_recount_sessions at each xdmcp request. 

Thanks again and have a nice day

My concern is not that it is a performance problem to call gdm_xdmcp_recount_sessions multiple times, but how best to do the cleanup.  It doesn't make a lot of sense to me to do the following:

1. recount
2. check for purged
3. call recount for each purged display found.

At the very least, it might make sense to check for purged, purge them all, and then recount once.

Or perhaps it might make sense if the code in do_purge were able to identify the situation that you are running into, where a recount is necessary and only do the recount when needed rather than all the time.

You don't answer whether the problem situation you encounter is a situation that could be identified.

I hope this is more clear?

The situation encountered can be identified, simply if the length of the displays list is lower than num_sessions. In this situation, num_sessions must be recounted. But the problem is that, num_sessions is not updated when the display list is decreasing.

I look at the code in daemon/display.c, where the display is actualy removed from the list. The function count_session_limits is called when a display is removed by gdm_display_dispose. I think that it could make sens to put the code for updating the display there, but I don't know how to send signal to xdmcp manager for this.

Meanwhile, we have to get gdm work again :) so I attached a patch that may be more conform to suggestions of Brian, in which recount may occurs up to three time maximum. I would be happy to get feedback on it. 

I did test the bug describe here and gdm_xdmcp_display_dispose_check, but I don't know how to test gdm_xdmcp_displays_purge. 

 

Created attachment 96570 [details] [review]
More optimized patch

There is a typo in the comment of the patch : 

update num_sessions only if the length of the string that contain

should be read : 

update num_sessions only if the length of the list that contain

Thanks.  I think this patch is much cleaner.  Now committed to 2.20 and SVN head branch.