Comment 54 for bug 328035

Revision history for this message
Bryce Harrington (bryce) wrote :

Interesting, it seems my initial analysis in comment #5 was not far off.

For now I've opted to drop the patch with tormod's debdiff in comment #51. I've uploaded this, but defer to slangasek's discretion in including it for beta. (I favor dropping it in beta myself but know that time is short, and while the crashes are serious and may lead to some extraneous bug reports during testing, I don't think it's going to critically inhibit testing activity.) If it doesn't go in for beta, it will go in post-beta.

Meanwhile, I think the patch should be changed to use safer string handling code. snprintf() has been suggested.

mdz has suggested this patch would be useful to include in the release. However, I have several concerns before doing this. First, we've been operating under the assumption that this was just test scaffolding code that would be dropped before the release, so it has not received the level of review that it needs. Second, as they say, "bugs tend to live in clusters", and since we've found several bugs in this patch to date I think it would be safe to assume there are more in there we've not yet found. Third, this patch has been beneficial for debugging purposes during development, but I'm uncertain if that need post-release is as strong; as tormod mentions in comment #50 there are some tradeoffs if we keep it. Possibly this could be addressed best by making it an xorg.conf togglable option.

mdz also suggested it should be posted upstream, which I think would net useful feedback for improving the patch. I would like to wait on doing this until the patch has been reviewed and updated to fix the aforementioned problems we already know about. I can't guess whether or not they will take the patch, but if nothing else perhaps there will be suggestions for improving it that we'd benefit from.

Meanwhile, it's curious that the code is getting negative timestamps. That secondary issue sounds like it may be worth some additional analysis. It sounds to me like a pointer is turning up uninitialized after resume, or there is an unsigned/signed or long/int type conversion going on. Either way, that could hint at further bugs that need to be resolved.