[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 Packard<keithp at 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.ylimaki at 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.