December 2004 - Wine-Devel - list.winehq.org

Re: ptrace single-stepping change breaks Wine

by Jesse Allen

On Fri, 31 Dec 2004 09:30:04 -0800 (PST), Linus Torvalds <torvalds(a)osdl.org> wrote: > > > On Fri, 31 Dec 2004, Davide Libenzi wrote: > > > > I don't think that the Wine problem resolution is due to the POPF > > instruction handling. Basically Linus patch does a nice cleanup plus POPF > > handling, so maybe the patch can be split. > > The popf part is very nice in that it allows you to single-step and debug > this thing. > > The fact is, I can't debug Wine. The code-base is just too alien for me, > so to debug this thing I needed to come up with a silly example of TF > usage, and try to debug _that_ instead as if it were something unknown (ie > debugging by knowing what the program does is a no-no, since that would > have defeated the whole exercise). > > And handlign "popf" correctly really was the only sane way to debug it, > anything else would never have worked in a real-life debugging situation. > It's easy enough to say "well, just do it by hand", but that's not > practical when you debug with "si 1000" to try to pinpoint the behaviour a > bit. > > And clearly my debuggability exercise seem to have worked, since the end > result apparently ended up doing the right thing for Wine. I honestly think there may have been no other way. There was a reason why I was very vague at first. I could not pin-point an exact location of failure. Just grepping a good log shows over 500,000 calls to RaiseException. trap_handler ran over 300,000 times and as many TF clears. My brother told me to set watches or break points to see if I could find something wrong, but I simply told him there is too much going on. The only thing I could think that can be done is to mentally find debugging scenarios and hope to find where it could be breaking. But the only people that could have thought of a scenario are people that know linux well enough what the kernel is doing in the first place. > > This is why debuggability is important. I realize that people may think > I'm inconsistent (since I abhor kernel debuggers), but while _I_ abhor > debuggers, I also think that the primary objective of an operating system > is to make easy things easy, and hard things possible, so while I don't > much like debuggers, I consider it a fundamental failure if the kernel > doesn't have proper support for them. I think that copy protection routines are particually nasty. They purposely make debugging hard (again see above, no sane program would be like that). And the program's reason for failing is not the reason at all -- "please insert disc" -- the disc is already in there! So they don't say the real reason why it failed, leaving the user totally hopelessly lost on what they should do. It's really hard to file a bug report on that alone! Had I not placed my guess on ptrace early on, this problem may have gone undiscovered for a long time. I have checked transgaming and they seem to be not aware about this, but it would have been a rude awakening for them when they find that when most distros had updated to 2.6.9, that all the SecuRom protected games silently broke, and they would have had a heck of a time debugging them to find the reason, I'm sure, even with the specs on it! Though who knows if cedega is affected because their code-base is diverging, and I'm sure their copy protection support is very different. > > So I think it's worth splitting out the "popf" part of the patch, but even > if that one doesn't actually matter for Warcraft, I'd put it in just so > that we have a state where we're _supposed_ to be able to debug things > with TF in them. Just having the mental expectation that things like that > should work is important - otherwise we'll eventually end up having some > other subtle problem with TF handling. > > Linus > > Sure. I've tried the game and it doesn't require is_at_popf(). I thought it did because I thought it was required for handling TF correctly, but maybe this is another scenario. I think it's a good idea to keep it in too. I actually don't care about debugging the copy protection, I just want correct behavoir. If doing all this has pointed out these other issues too, then it's all well worth it, because next time it will be /easier/ and we won't have subtle problems. Jesse

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Davide Libenzi wrote: > > I don't think that the Wine problem resolution is due to the POPF > instruction handling. Basically Linus patch does a nice cleanup plus POPF > handling, so maybe the patch can be split. The popf part is very nice in that it allows you to single-step and debug this thing. The fact is, I can't debug Wine. The code-base is just too alien for me, so to debug this thing I needed to come up with a silly example of TF usage, and try to debug _that_ instead as if it were something unknown (ie debugging by knowing what the program does is a no-no, since that would have defeated the whole exercise). And handlign "popf" correctly really was the only sane way to debug it, anything else would never have worked in a real-life debugging situation. It's easy enough to say "well, just do it by hand", but that's not practical when you debug with "si 1000" to try to pinpoint the behaviour a bit. And clearly my debuggability exercise seem to have worked, since the end result apparently ended up doing the right thing for Wine. This is why debuggability is important. I realize that people may think I'm inconsistent (since I abhor kernel debuggers), but while _I_ abhor debuggers, I also think that the primary objective of an operating system is to make easy things easy, and hard things possible, so while I don't much like debuggers, I consider it a fundamental failure if the kernel doesn't have proper support for them. So I think it's worth splitting out the "popf" part of the patch, but even if that one doesn't actually matter for Warcraft, I'd put it in just so that we have a state where we're _supposed_ to be able to debug things with TF in them. Just having the mental expectation that things like that should work is important - otherwise we'll eventually end up having some other subtle problem with TF handling. Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Daniel Jacobowitz wrote: > > Lots, I like it. The syscall trap will always be delivered before the > single-step trap, right, because signal delivery won't run until we > return to userspace? Yes. Although I've not actually tested it. Before, it used to show up as one event, and basically the "0x80" marker got lost, so effectively the "system call exit" part would have got lost. Now, it _may_ DTRT, with the caveat that the system call ptrace_notify() thing still has the same problem with restarting-with-a-signal. That's basically a "don't do that then", and is the status quo, of course, so this is at least not a regression. It's still pretty ugly, but apparently nobody really cares ;) Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Andi Kleen wrote: > > > - you couldn't even debug signal handlers, because they were _really_ > > hard to get into unless you knew where they were and put a breakpoint > > on them. > > Ok I see this as being a problem. But I bet it could be fixed > much simpler without doing all this complicated and likely-to-be-buggy > popf parsing you added. And that is _exactly_ what we did. Guess what broke Wine? Uhhuh. Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Thomas Sailer

On Freitag 31 Dezember 2004 14.31, Mike Hearn wrote: > What about this patch? This works now. Happy new year... Tom

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Davide Libenzi

On Fri, 31 Dec 2004, Andi Kleen wrote: > > - you couldn't even debug signal handlers, because they were _really_ > > hard to get into unless you knew where they were and put a breakpoint > > on them. > > Ok I see this as being a problem. But I bet it could be fixed > much simpler without doing all this complicated and likely-to-be-buggy > popf parsing you added. I don't think that the Wine problem resolution is due to the POPF instruction handling. Basically Linus patch does a nice cleanup plus POPF handling, so maybe the patch can be split. > > - you couldn't see the instruction after a system call. > > Are you sure? Yes, this was true with 2.4. Than it has been fixed some time ago. But handling that revealed a fragile handling of ptrace event delivery we had in do_syscall_trace(). Part of the Linus patch tries to solve the fact that on one side strace wants things to happen in a certain way, way that seem to break Wine. I think Linus cleanup of the ptrace event delivery can get strace, Wine and single-step-after-syscall right, w/out POPF handling. Then you guys can flame each other over the POPF handling ;) - Davide

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Daniel Jacobowitz

On Thu, Dec 30, 2004 at 09:47:42PM -0800, Linus Torvalds wrote: > So I looked at just sharing the code with the debug trap handler, and the > result is appended. strace works, as does all the TF tests I've thrown at > it, and the code actually looks better anyway (the old do_debug code looks > like it got the EIP wrong in VM86 mode, for example, this just cleans > that up too). Just use a common "send_sigtrap()" routine. > > Does this look saner? Lots, I like it. The syscall trap will always be delivered before the single-step trap, right, because signal delivery won't run until we return to userspace? -- Daniel Jacobowitz

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Mike Hearn

On Fri, 2004-12-31 at 14:13 +0100, Thomas Sailer wrote: > No this doesn't work. The decision which address space layout to use is done > in arch/i386/mm/mmap.c:arch_pick_mmap_layout, and this function is called by > the elf loader in fs/binfmt_elf.c:load_elf_binary, i.e. the decision which > address space layout to use for the current wine process is already done long > time before your personality syscall takes effect. > > I hoped there was some ELF section magic to turn this off (like execshield), > but there doesn't seem to be. So it has to be done before an exec then? I had assumed changing the personality would affect all mmaps from that point onwards, too bad. We do some execs as part of the Wine startup code so it shouldn't be too much of an issue. Anyway, all the setarch program does is do this syscall then exec the program so it shouldn't be too hard to do the equivalent in Wine. What about this patch? --- libs/wine/config.c (revision 14) +++ libs/wine/config.c (local) @@ -35,6 +35,10 @@ #endif #include "wine/library.h" +#ifdef HAVE_SYSCALL_H +#include <syscall.h> +#endif + static const char server_config_dir[] = "/.wine"; /* config dir relative to $HOME */ static const char server_root_prefix[] = "/tmp/.wine-"; /* prefix for server root dir */ static const char server_dir_prefix[] = "/server-"; /* prefix for server dir */ @@ -289,8 +293,13 @@ static void preloader_exec( char **argv, new_argv = xmalloc( (last_arg - argv + 2) * sizeof(*argv) ); memcpy( new_argv + 1, argv, (last_arg - argv + 1) * sizeof(*argv) ); new_argv[0] = full_name; + + /* set the abi personality */ + syscall(136, 0x8 /* PER_LINUX32 */ | 0x0200000 /* ADDR_COMPAT_LAYOUT */); + if (envp) execve( full_name, new_argv, envp ); else execv( full_name, new_argv ); + free( new_argv ); free( full_name ); return;

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Thomas Sailer

On Donnerstag 30 Dezember 2004 14.06, you wrote: > Tom, does this patch against Wine help? It should do the same thing as > the setarch program, so if that fixes it then this should also (if I've > understood how this mechanism works of course). No this doesn't work. The decision which address space layout to use is done in arch/i386/mm/mmap.c:arch_pick_mmap_layout, and this function is called by the elf loader in fs/binfmt_elf.c:load_elf_binary, i.e. the decision which address space layout to use for the current wine process is already done long time before your personality syscall takes effect. I hoped there was some ELF section magic to turn this off (like execshield), but there doesn't seem to be. Tom

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Andi Kleen

On Thu, Dec 30, 2004 at 04:38:21PM -0800, Linus Torvalds wrote: > > Can someone repeat again what was wrong with the old ptrace > > semantics before the initial change that caused all these complex > > changes? It seemed to work well for years. How about we just > > go back to the old state, revert all the recent ptrace changes > > and skip all that? > > Let me count the ways that were wrong before the changes: > - you couldn't debug any code that set TF. Really. ptrace would totally > destroy the TF state in the controlled process, so it would do > something totally different when debugged. Well, tough. I assume people who play with TF themselves know how to handle it without debuggers. Really adding instruction parsing for such a corner case seems like extreme overkill to me. I agree it is not nice, but is it really worth all that complexity to hide it? > - you couldn't even debug signal handlers, because they were _really_ > hard to get into unless you knew where they were and put a breakpoint > on them. Ok I see this as being a problem. But I bet it could be fixed much simpler without doing all this complicated and likely-to-be-buggy popf parsing you added. > - you couldn't see the instruction after a system call. Are you sure? > - ptrace returned bogus TF state after a single-step I am sure all debuggers in existence deal with that (and they will need to continue doing so because there are so many old kernels around) > descriptors, and we actually do that (badly) in another place: the AMD > "prefetch" check does exactly the same thing except it seems to get a few > details wrong (looks like it cannot handle 16-bit code), and only works > for the current process. Yes, it was intentional to simplify it. 16bit code is not supposed to use prefetches (and even if they do the probability of faulting is pretty small) Also we needed several iterations to get all the subtle bugs out (and I bet there are some issues left) -Andi

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Jesse Allen

On Thu, 30 Dec 2004 21:47:42 -0800 (PST), Linus Torvalds <torvalds(a)osdl.org> wrote: > > > > So I looked at just sharing the code with the debug trap handler, and the > result is appended. strace works, as does all the TF tests I've thrown at > it, and the code actually looks better anyway (the old do_debug code looks > like it got the EIP wrong in VM86 mode, for example, this just cleans > that up too). Just use a common "send_sigtrap()" routine. > > Does this look saner? > Yeah. I've tested and this one works. I don't have any other copy protection schemes that are broken. Of the cd-rom based, older safedisc still worked, and the newer one needs a special device driver that wine cannot load properly yet. It's more likely if there is a problem with one that it's a problem with wine at this point. Jesse

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Daniel Jacobowitz wrote: > > Well, you put SIGTRAP|0x80 in si_code. Coincidentally, 0x80 is > SI_KERNEL. So testing for SI_KERNEL | 0x80 is probably OK in the > signal path, since most of its other arbitrary values would be either > negative or not include SI_KERNEL. Somebody would need to validate that no user can fool the kernel into doing something stupid... That said, I think I solved the problem a different way: the TIF_SINGLESTEP code really fundamentally doesn't want to have _any_ of that SIGTRAP | 0x80 nonsense in its path, it actually wants to look like a debug trap - which sets si_code to TRAP_BRKPT. Which makes more sense anyway. So I looked at just sharing the code with the debug trap handler, and the result is appended. strace works, as does all the TF tests I've thrown at it, and the code actually looks better anyway (the old do_debug code looks like it got the EIP wrong in VM86 mode, for example, this just cleans that up too). Just use a common "send_sigtrap()" routine. Does this look saner? Linus ---- ===== include/asm-i386/ptrace.h 1.7 vs edited ===== --- 1.7/include/asm-i386/ptrace.h 2004-09-03 16:55:27 -07:00 +++ edited/include/asm-i386/ptrace.h 2004-12-30 21:27:39 -08:00 @@ -55,6 +55,7 @@ #define PTRACE_SET_THREAD_AREA 26 #ifdef __KERNEL__ +extern void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs, int error_code); #define user_mode(regs) ((VM_MASK & (regs)->eflags) || (3 & (regs)->xcs)) #define instruction_pointer(regs) ((regs)->eip) #if defined(CONFIG_SMP) && defined(CONFIG_FRAME_POINTER) ===== arch/i386/kernel/signal.c 1.49 vs edited ===== --- 1.49/arch/i386/kernel/signal.c 2004-11-22 09:47:16 -08:00 +++ edited/arch/i386/kernel/signal.c 2004-12-30 11:40:35 -08:00 @@ -270,7 +270,6 @@ struct pt_regs *regs, unsigned long mask) { int tmp, err = 0; - unsigned long eflags; tmp = 0; __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp)); @@ -292,16 +291,7 @@ err |= __put_user(current->thread.error_code, &sc->err); err |= __put_user(regs->eip, &sc->eip); err |= __put_user(regs->xcs, (unsigned int __user *)&sc->cs); - - /* - * Iff TF was set because the program is being single-stepped by a - * debugger, don't save that information on the signal stack.. We - * don't want debugging to change state. - */ - eflags = regs->eflags; - if (current->ptrace & PT_DTRACE) - eflags &= ~TF_MASK; - err |= __put_user(eflags, &sc->eflags); + err |= __put_user(regs->eflags, &sc->eflags); err |= __put_user(regs->esp, &sc->esp_at_signal); err |= __put_user(regs->xss, (unsigned int __user *)&sc->ss); @@ -424,11 +414,9 @@ * The tracer may want to single-step inside the * handler too. */ - if (regs->eflags & TF_MASK) { - regs->eflags &= ~TF_MASK; - if (current->ptrace & PT_DTRACE) - ptrace_notify(SIGTRAP); - } + regs->eflags &= ~TF_MASK; + if (test_thread_flag(TIF_SINGLESTEP)) + ptrace_notify(SIGTRAP); #if DEBUG_SIG printk("SIG deliver (%s:%d): sp=%p pc=%p ra=%p\n", @@ -519,11 +507,9 @@ * The tracer may want to single-step inside the * handler too. */ - if (regs->eflags & TF_MASK) { - regs->eflags &= ~TF_MASK; - if (current->ptrace & PT_DTRACE) - ptrace_notify(SIGTRAP); - } + regs->eflags &= ~TF_MASK; + if (test_thread_flag(TIF_SINGLESTEP)) + ptrace_notify(SIGTRAP); #if DEBUG_SIG printk("SIG deliver (%s:%d): sp=%p pc=%p ra=%p\n", ===== arch/i386/kernel/traps.c 1.92 vs edited ===== --- 1.92/arch/i386/kernel/traps.c 2004-12-28 11:07:48 -08:00 +++ edited/arch/i386/kernel/traps.c 2004-12-30 21:26:14 -08:00 @@ -718,23 +718,21 @@ */ if ((regs->xcs & 3) == 0) goto clear_TF_reenable; - if ((tsk->ptrace & (PT_DTRACE|PT_PTRACED)) == PT_DTRACE) - goto clear_TF; + + /* + * Was the TF flag set by a debugger? If so, clear it now, + * so that register information is correct. + */ + if (tsk->ptrace & PT_DTRACE) { + regs->eflags &= ~TF_MASK; + tsk->ptrace &= ~PT_DTRACE; + if (!tsk->ptrace & PT_DTRACE) + goto clear_TF; + } } /* Ok, finally something we can handle */ - tsk->thread.trap_no = 1; - tsk->thread.error_code = error_code; - info.si_signo = SIGTRAP; - info.si_errno = 0; - info.si_code = TRAP_BRKPT; - - /* If this is a kernel mode trap, save the user PC on entry to - * the kernel, that's what the debugger can make sense of. - */ - info.si_addr = ((regs->xcs & 3) == 0) ? (void __user *)tsk->thread.eip - : (void __user *)regs->eip; - force_sig_info(SIGTRAP, &info, tsk); + send_sigtrap(tsk, regs, error_code); /* Disable additional traps. They'll be re-enabled when * the signal is delivered. ===== arch/i386/kernel/ptrace.c 1.28 vs edited ===== --- 1.28/arch/i386/kernel/ptrace.c 2004-11-22 09:44:52 -08:00 +++ edited/arch/i386/kernel/ptrace.c 2004-12-30 21:26:16 -08:00 @@ -42,6 +42,12 @@ */ #define EFL_OFFSET ((EFL-2)*4-sizeof(struct pt_regs)) +static inline struct pt_regs *get_child_regs(struct task_struct *task) +{ + void *stack_top = (void *)task->thread.esp0; + return stack_top - sizeof(struct pt_regs); +} + /* * this routine will get a word off of the processes privileged stack. * the offset is how far from the base addr as stored in the TSS. @@ -138,24 +144,119 @@ return retval; } +#define LDT_SEGMENT 4 + +static unsigned long convert_eip_to_linear(struct task_struct *child, struct pt_regs *regs) +{ + unsigned long addr, seg; + + addr = regs->eip; + seg = regs->xcs & 0xffff; + if (regs->eflags & VM_MASK) { + addr = (addr & 0xffff) + (seg << 4); + return addr; + } + + /* + * We'll assume that the code segments in the GDT + * are all zero-based. That is largely true: the + * TLS segments are used for data, and the PNPBIOS + * and APM bios ones we just ignore here. + */ + if (seg & LDT_SEGMENT) { + u32 *desc; + unsigned long base; + + down(&child->mm->context.sem); + desc = child->mm->context.ldt + (seg & ~7); + base = (desc[0] >> 16) | ((desc[1] & 0xff) << 16) | (desc[1] & 0xff000000); + + /* 16-bit code segment? */ + if (!((desc[1] >> 22) & 1)) + addr &= 0xffff; + addr += base; + up(&child->mm->context.sem); + } + return addr; +} + +static inline int is_at_popf(struct task_struct *child, struct pt_regs *regs) +{ + int i, copied; + unsigned char opcode[16]; + unsigned long addr = convert_eip_to_linear(child, regs); + + copied = access_process_vm(child, addr, opcode, sizeof(opcode), 0); + for (i = 0; i < copied; i++) { + switch (opcode[i]) { + /* popf */ + case 0x9d: + return 1; + /* opcode and address size prefixes */ + case 0x66: case 0x67: + continue; + /* irrelevant prefixes (segment overrides and repeats) */ + case 0x26: case 0x2e: + case 0x36: case 0x3e: + case 0x64: case 0x65: + case 0xf0: case 0xf2: case 0xf3: + continue; + + /* + * pushf: NOTE! We should probably not let + * the user see the TF bit being set. But + * it's more pain than it's worth to avoid + * it, and a debugger could emulate this + * all in user space if it _really_ cares. + */ + case 0x9c: + default: + return 0; + } + } + return 0; +} + static void set_singlestep(struct task_struct *child) { - long eflags; + struct pt_regs *regs = get_child_regs(child); + /* + * Always set TIF_SINGLESTEP - this guarantees that + * we single-step system calls etc.. This will also + * cause us to set TF when returning to user mode. + */ set_tsk_thread_flag(child, TIF_SINGLESTEP); - eflags = get_stack_long(child, EFL_OFFSET); - put_stack_long(child, EFL_OFFSET, eflags | TRAP_FLAG); + + /* + * If TF was already set, don't do anything else + */ + if (regs->eflags & TRAP_FLAG) + return; + + /* Set TF on the kernel stack.. */ + regs->eflags |= TRAP_FLAG; + + /* + * ..but if TF is changed by the instruction we will trace, + * don't mark it as being "us" that set it, so that we + * won't clear it by hand later. + */ + if (is_at_popf(child, regs)) + return; + child->ptrace |= PT_DTRACE; } static void clear_singlestep(struct task_struct *child) { - if (child->ptrace & PT_DTRACE) { - long eflags; + /* Always clear TIF_SINGLESTEP... */ + clear_tsk_thread_flag(child, TIF_SINGLESTEP); - clear_tsk_thread_flag(child, TIF_SINGLESTEP); - eflags = get_stack_long(child, EFL_OFFSET); - put_stack_long(child, EFL_OFFSET, eflags & ~TRAP_FLAG); + /* But touch TF only if it was set by us.. */ + if (child->ptrace & PT_DTRACE) { + struct pt_regs *regs = get_child_regs(child); + regs->eflags &= ~TRAP_FLAG; child->ptrace &= ~PT_DTRACE; } } @@ -553,6 +654,29 @@ return ret; } +void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs, int error_code) +{ + struct siginfo info; + + tsk->thread.trap_no = 1; + tsk->thread.error_code = error_code; + + memset(&info, 0, sizeof(info)); + info.si_signo = SIGTRAP; + info.si_code = TRAP_BRKPT; + + /* + * If this is a kernel mode trap, save the user PC on entry to + * the kernel, that's what the debugger can make sense of. + */ + info.si_addr = user_mode(regs) ? + (void __user *) regs->eip : + (void __user *)tsk->thread.eip; + + /* Send us the fakey SIGTRAP */ + force_sig_info(SIGTRAP, &info, tsk); +} + /* notification of system call entry/exit * - triggered by current->work.syscall_trace */ @@ -568,15 +692,16 @@ audit_syscall_exit(current, regs->eax); } - if (!test_thread_flag(TIF_SYSCALL_TRACE) && - !test_thread_flag(TIF_SINGLESTEP)) - return; if (!(current->ptrace & PT_PTRACED)) return; - /* the 0x80 provides a way for the tracing parent to distinguish - between a syscall stop and SIGTRAP delivery */ - ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD) && - !test_thread_flag(TIF_SINGLESTEP) ? 0x80 : 0)); + + /* Fake a debug trap */ + if (test_thread_flag(TIF_SINGLESTEP)) + send_sigtrap(current, regs, 0); + + if (!test_thread_flag(TIF_SYSCALL_TRACE)) + return; + ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD) ? 0x80 : 0)); /* * this isn't the same as continuing with a signal, but it will do

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Daniel Jacobowitz

On Thu, Dec 30, 2004 at 09:05:01PM -0800, Linus Torvalds wrote: > > > On Thu, 30 Dec 2004, Jesse Allen wrote: > > > > Well I tried this patch and it works. > > Goodie. Are there other known problems with silly copy-protection > schemes? It migth be worth testing. > > However: > > > Since I cannot spot any issue, the patch looks good. Are there any > > other test cases? > > Yes. It seems I broke "strace" with it. Probably the difference in system > call trace reporting that Dan Jacobowitz already pointed out. > > Now, that should be easily handled by just separating out the cases of > system call tracing and debug trap handling, and using the old silly code > for system calls. I'd prefer a cleaner approach, but that seems to be the > sane thing to do for now. Strace doesn't use PTRACE_SETOPTIONS as far as I can tell... so it must be something different. -- Daniel Jacobowitz

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Daniel Jacobowitz

On Thu, Dec 30, 2004 at 03:17:01PM -0800, Linus Torvalds wrote: > > > On Thu, 30 Dec 2004, Daniel Jacobowitz wrote: > > > > does not look right to me. Before, we'd get an 0x80|SIGTRAP result > > from wait. Now, you've moved the 0x80 to live only inside the siginfo. > > This is accessible to the debugger via ptrace, but only very recently > > (late 2.5.x). So this will probably break users of PT_TRACESYSGOOD. > > I don't see how to easily emulate the old behaviour 100% - see > ptrace_notify. We always sent the signal SIGTRAP to the process, and then > set "exit_code" tp have the 0x80 marker by calling ptrace_stop() by hand. > However, if we make it a real signal (which we need to do to get the > continue semantics right), we no longer have that out-of-band info > available to us. > > We could make "get_signal_to_deliver()" pass in some other exit_code to > ptrace_stop() than just signr. It would have to pick it up from the > siginfo structure, but then we'd have to make sure that _other_ signal > users do that properly.. > > Patches/suggestions welcome. Well, you put SIGTRAP|0x80 in si_code. Coincidentally, 0x80 is SI_KERNEL. So testing for SI_KERNEL | 0x80 is probably OK in the signal path, since most of its other arbitrary values would be either negative or not include SI_KERNEL. -- Daniel Jacobowitz

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Thu, 30 Dec 2004, Jesse Allen wrote: > > Well I tried this patch and it works. Goodie. Are there other known problems with silly copy-protection schemes? It migth be worth testing. However: > Since I cannot spot any issue, the patch looks good. Are there any > other test cases? Yes. It seems I broke "strace" with it. Probably the difference in system call trace reporting that Dan Jacobowitz already pointed out. Now, that should be easily handled by just separating out the cases of system call tracing and debug trap handling, and using the old silly code for system calls. I'd prefer a cleaner approach, but that seems to be the sane thing to do for now. Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Jesse Allen

On Thu, 30 Dec 2004 14:46:17 -0800 (PST), Linus Torvalds <torvalds(a)osdl.org> wrote: > > > Ok, here's a patch that may or may not make Wine happier. It's a _lot_ > more careful about TF handling, and in particular it's trying really > really hard to make sure that a controlling process does not change the > trap flag as it is modified or used by the process. > > This hopefully fixes: > > - single-step over "popf" should work - we won't clear TF after the popf, > but instead let the popf results remain. > > NOTE! I tried to make sure that it does the right thing for segments > with non-zero bases, but I never actually tested that code. It's fairly > obvious, but it's also fairly likely to have some silly problems. Wine > may well show effects of this, although I don't know how common > non-zero bases are with any kind of half-modern windows binaries. > > - ptrace reporting of "eflags" register after a single-step (we used to > report TF as being set because the debugger set it - even though the > "native state" without debugging had it cleared). > > This also hopefully means that all the conditional TF clearing games > etc aren't necessary, since arch/i386/kernel/traps.c should now be > taking care of hiding the debugger for most cases ("pushf" still > remains, and is hard. See comment in ptrace.c part of the patch) > > It's a bit more involved than I'd like, since especially the "popf" case > just is pretty complex, but I'd love to hear whether it works. > > NOTE NOTE NOTE! I've tested it, but only on one small test-case, so it > might be totally broken in many ways. I'd love to have people who are x86 > and ptrace-aware give this a second look, and I'm confident Jesse will > find that it won't work, but can hopefully try to debug it a bit with > this.. > Well I tried this patch and it works. I captured a log showing the signals and eflags again when running the program. I compared it to the working version. There are differences, but none seem to be the scenario TF was not set when it should have been. Both log files are just about the same size too. I captured a second log in a row, and compared the previous. Again there are differences, so there is some unavoidable randomness. Since I cannot spot any issue, the patch looks good. Are there any other test cases? Jesse

Dec. 31, 2004

Re: Remove heap.h

by Filip Navara

Jon Griffiths wrote: >+ docA.lpszDocName = WtoA( doc->lpszDocName ); >+ docA.lpszOutput = WtoA( doc->lpszOutput ); >+ docA.lpszDatatype = WtoA( doc->lpszDatatype; > Isn't there a missing parenthesis?

Dec. 31, 2004

Re: ITSS: DllCanUnloadNow implementation

by Robert Shearman

Jacek Caban wrote: > Changelog: > Implemented DllCanUnloadNow > > BOOL WINAPI ITSS_DllCanUnloadNow(void) > { >- FIXME("\n"); >- >- return FALSE; >+ TRACE("dll_count = %lu\n", dll_count); >+ return !dll_count; > > This function should return an HRESULT, not a BOOL. This error would have been caught if it weren't for the silly ITSS_ prefix. Consequently, the return value should be either S_OK or S_FALSE depending on whether there are no or some remaining references respectively. Rob

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Fri, 31 Dec 2004, Andi Kleen wrote: > > Just looking at all this complexiy and thinking about > making it work on x86-64 too doesn't exactly give a good > feeling in my spine. > > Not to belittle your archivement Linus but it all looks > very overengineered to me. Ehh, do you have any _alternatives_? > I think such complex instruction emulation games will be > hard to maintain and there are very surely bugs in so > much subtle code. There is no complexity anywhere, and we don't actually emulate any instructions at all. The only thing we do is to check _whether_ the instruction is a "popf" - we let the CPU do all the work, we just say "ok, the instruction will set TF, so we should not touch it afterwards. > Can someone repeat again what was wrong with the old ptrace > semantics before the initial change that caused all these complex > changes? It seemed to work well for years. How about we just > go back to the old state, revert all the recent ptrace changes > and skip all that? Let me count the ways that were wrong before the changes: - you couldn't debug any code that set TF. Really. ptrace would totally destroy the TF state in the controlled process, so it would do something totally different when debugged. - you couldn't even debug signal handlers, because they were _really_ hard to get into unless you knew where they were and put a breakpoint on them. - you couldn't see the instruction after a system call. - ptrace returned bogus TF state after a single-step > I would love to skip this all on x86-64, but I would prefer > to not make the behaviour incompatible to i386. I suspect all the code can be shared. In fact, the change to send a SIGTRAP directly rather than play around with "ptrace_notify()" etc is likely totally architecture-independent apart from the calling convention magic, so all of "do_syscall_trace()" could probably be moved into kernel/ptrace.c. The _only_ real complexity is actually following the silly LDT descriptors, and we actually do that (badly) in another place: the AMD "prefetch" check does exactly the same thing except it seems to get a few details wrong (looks like it cannot handle 16-bit code), and only works for the current process. I assume you have that same prefetch thing on x86-64 already, so if anything, you could look at my replacement and see if it would be workable to do the prefetch thing too.. IOW, none of the issues involved are new. Linus

Dec. 31, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Thu, 30 Dec 2004, Daniel Jacobowitz wrote: > > does not look right to me. Before, we'd get an 0x80|SIGTRAP result > from wait. Now, you've moved the 0x80 to live only inside the siginfo. > This is accessible to the debugger via ptrace, but only very recently > (late 2.5.x). So this will probably break users of PT_TRACESYSGOOD. I don't see how to easily emulate the old behaviour 100% - see ptrace_notify. We always sent the signal SIGTRAP to the process, and then set "exit_code" tp have the 0x80 marker by calling ptrace_stop() by hand. However, if we make it a real signal (which we need to do to get the continue semantics right), we no longer have that out-of-band info available to us. We could make "get_signal_to_deliver()" pass in some other exit_code to ptrace_stop() than just signr. It would have to pick it up from the siginfo structure, but then we'd have to make sure that _other_ signal users do that properly.. Patches/suggestions welcome. Linus

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Andi Kleen

Linus Torvalds <torvalds(a)osdl.org> writes: > It's a bit more involved than I'd like, since especially the "popf" case > just is pretty complex, but I'd love to hear whether it works. > > NOTE NOTE NOTE! I've tested it, but only on one small test-case, so it > might be totally broken in many ways. I'd love to have people who are x86 > and ptrace-aware give this a second look, and I'm confident Jesse will > find that it won't work, but can hopefully try to debug it a bit with > this.. Just looking at all this complexiy and thinking about making it work on x86-64 too doesn't exactly give a good feeling in my spine. Not to belittle your archivement Linus but it all looks very overengineered to me. I think such complex instruction emulation games will be hard to maintain and there are very surely bugs in so much subtle code. Can someone repeat again what was wrong with the old ptrace semantics before the initial change that caused all these complex changes? It seemed to work well for years. How about we just go back to the old state, revert all the recent ptrace changes and skip all that? e.g. reporting TF after popf in ptrace doesnt really seem like a big issue to me that is worth fixing with that much code. It is more an unimportant corner case, isnt it? Same thing with forcing TF after popf. I bet most debuggers in existence get this special case wrong and so far the world hasn't collapsed because of that. I would love to skip this all on x86-64, but I would prefer to not make the behaviour incompatible to i386. -Andi

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Daniel Jacobowitz

i386 architecture details are really not my thing, so I'm going to trust you on most of this, but this bit: On Thu, Dec 30, 2004 at 02:46:17PM -0800, Linus Torvalds wrote: > /* the 0x80 provides a way for the tracing parent to distinguish > between a syscall stop and SIGTRAP delivery */ > - ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD) && > - !test_thread_flag(TIF_SINGLESTEP) ? 0x80 : 0)); > + info.si_code = SIGTRAP; > + if ((current->ptrace & PT_TRACESYSGOOD) && !test_thread_flag(TIF_SINGLESTEP)) > + info.si_code = SIGTRAP | 0x80; > + info.si_pid = current->tgid; > + info.si_uid = current->uid; > > - /* > - * this isn't the same as continuing with a signal, but it will do > - * for normal use. strace only continues with a signal if the > - * stopping signal is not SIGTRAP. -brl > - */ > - if (current->exit_code) { > - send_sig(current->exit_code, current, 1); > - current->exit_code = 0; > - } > + /* Send us the fakey SIGTRAP */ > + send_sig_info(SIGTRAP, &info, current); > } does not look right to me. Before, we'd get an 0x80|SIGTRAP result from wait. Now, you've moved the 0x80 to live only inside the siginfo. This is accessible to the debugger via ptrace, but only very recently (late 2.5.x). So this will probably break users of PT_TRACESYSGOOD. -- Daniel Jacobowitz

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Thu, 30 Dec 2004, Linus Torvalds wrote: > > Working on a patch for this right now, I'll send something out soonish > (and I'll test it on my test-case before sending it, so that it at least > has some chance of working). Ok, here's a patch that may or may not make Wine happier. It's a _lot_ more careful about TF handling, and in particular it's trying really really hard to make sure that a controlling process does not change the trap flag as it is modified or used by the process. This hopefully fixes: - single-step over "popf" should work - we won't clear TF after the popf, but instead let the popf results remain. NOTE! I tried to make sure that it does the right thing for segments with non-zero bases, but I never actually tested that code. It's fairly obvious, but it's also fairly likely to have some silly problems. Wine may well show effects of this, although I don't know how common non-zero bases are with any kind of half-modern windows binaries. - ptrace reporting of "eflags" register after a single-step (we used to report TF as being set because the debugger set it - even though the "native state" without debugging had it cleared). This also hopefully means that all the conditional TF clearing games etc aren't necessary, since arch/i386/kernel/traps.c should now be taking care of hiding the debugger for most cases ("pushf" still remains, and is hard. See comment in ptrace.c part of the patch) It's a bit more involved than I'd like, since especially the "popf" case just is pretty complex, but I'd love to hear whether it works. NOTE NOTE NOTE! I've tested it, but only on one small test-case, so it might be totally broken in many ways. I'd love to have people who are x86 and ptrace-aware give this a second look, and I'm confident Jesse will find that it won't work, but can hopefully try to debug it a bit with this.. Linus ----- ===== arch/i386/kernel/signal.c 1.49 vs edited ===== --- 1.49/arch/i386/kernel/signal.c 2004-11-22 09:47:16 -08:00 +++ edited/arch/i386/kernel/signal.c 2004-12-30 11:40:35 -08:00 @@ -270,7 +270,6 @@ struct pt_regs *regs, unsigned long mask) { int tmp, err = 0; - unsigned long eflags; tmp = 0; __asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp)); @@ -292,16 +291,7 @@ err |= __put_user(current->thread.error_code, &sc->err); err |= __put_user(regs->eip, &sc->eip); err |= __put_user(regs->xcs, (unsigned int __user *)&sc->cs); - - /* - * Iff TF was set because the program is being single-stepped by a - * debugger, don't save that information on the signal stack.. We - * don't want debugging to change state. - */ - eflags = regs->eflags; - if (current->ptrace & PT_DTRACE) - eflags &= ~TF_MASK; - err |= __put_user(eflags, &sc->eflags); + err |= __put_user(regs->eflags, &sc->eflags); err |= __put_user(regs->esp, &sc->esp_at_signal); err |= __put_user(regs->xss, (unsigned int __user *)&sc->ss); @@ -424,11 +414,9 @@ * The tracer may want to single-step inside the * handler too. */ - if (regs->eflags & TF_MASK) { - regs->eflags &= ~TF_MASK; - if (current->ptrace & PT_DTRACE) - ptrace_notify(SIGTRAP); - } + regs->eflags &= ~TF_MASK; + if (test_thread_flag(TIF_SINGLESTEP)) + ptrace_notify(SIGTRAP); #if DEBUG_SIG printk("SIG deliver (%s:%d): sp=%p pc=%p ra=%p\n", @@ -519,11 +507,9 @@ * The tracer may want to single-step inside the * handler too. */ - if (regs->eflags & TF_MASK) { - regs->eflags &= ~TF_MASK; - if (current->ptrace & PT_DTRACE) - ptrace_notify(SIGTRAP); - } + regs->eflags &= ~TF_MASK; + if (test_thread_flag(TIF_SINGLESTEP)) + ptrace_notify(SIGTRAP); #if DEBUG_SIG printk("SIG deliver (%s:%d): sp=%p pc=%p ra=%p\n", ===== arch/i386/kernel/traps.c 1.92 vs edited ===== --- 1.92/arch/i386/kernel/traps.c 2004-12-28 11:07:48 -08:00 +++ edited/arch/i386/kernel/traps.c 2004-12-30 12:36:30 -08:00 @@ -718,8 +718,17 @@ */ if ((regs->xcs & 3) == 0) goto clear_TF_reenable; - if ((tsk->ptrace & (PT_DTRACE|PT_PTRACED)) == PT_DTRACE) - goto clear_TF; + + /* + * Was the TF flag set by a debugger? If so, clear it now, + * so that register information is correct. + */ + if (tsk->ptrace & PT_DTRACE) { + regs->eflags &= ~TF_MASK; + tsk->ptrace &= ~PT_DTRACE; + if (!tsk->ptrace & PT_DTRACE) + goto clear_TF; + } } /* Ok, finally something we can handle */ ===== arch/i386/kernel/ptrace.c 1.28 vs edited ===== --- 1.28/arch/i386/kernel/ptrace.c 2004-11-22 09:44:52 -08:00 +++ edited/arch/i386/kernel/ptrace.c 2004-12-30 14:35:45 -08:00 @@ -42,6 +42,12 @@ */ #define EFL_OFFSET ((EFL-2)*4-sizeof(struct pt_regs)) +static inline struct pt_regs *get_child_regs(struct task_struct *task) +{ + void *stack_top = (void *)task->thread.esp0; + return stack_top - sizeof(struct pt_regs); +} + /* * this routine will get a word off of the processes privileged stack. * the offset is how far from the base addr as stored in the TSS. @@ -138,24 +144,119 @@ return retval; } +#define LDT_SEGMENT 4 + +static unsigned long convert_eip_to_linear(struct task_struct *child, struct pt_regs *regs) +{ + unsigned long addr, seg; + + addr = regs->eip; + seg = regs->xcs & 0xffff; + if (regs->eflags & VM_MASK) { + addr = (addr & 0xffff) + (seg << 4); + return addr; + } + + /* + * We'll assume that the code segments in the GDT + * are all zero-based. That is largely true: the + * TLS segments are used for data, and the PNPBIOS + * and APM bios ones we just ignore here. + */ + if (seg & LDT_SEGMENT) { + u32 *desc; + unsigned long base; + + down(&child->mm->context.sem); + desc = child->mm->context.ldt + (seg & ~7); + base = (desc[0] >> 16) | ((desc[1] & 0xff) << 16) | (desc[1] & 0xff000000); + + /* 16-bit code segment? */ + if (!((desc[1] >> 22) & 1)) + addr &= 0xffff; + addr += base; + up(&child->mm->context.sem); + } + return addr; +} + +static inline int is_at_popf(struct task_struct *child, struct pt_regs *regs) +{ + int i, copied; + unsigned char opcode[16]; + unsigned long addr = convert_eip_to_linear(child, regs); + + copied = access_process_vm(child, addr, opcode, sizeof(opcode), 0); + for (i = 0; i < copied; i++) { + switch (opcode[i]) { + /* popf */ + case 0x9d: + return 1; + /* opcode and address size prefixes */ + case 0x66: case 0x67: + continue; + /* irrelevant prefixes (segment overrides and repeats) */ + case 0x26: case 0x2e: + case 0x36: case 0x3e: + case 0x64: case 0x65: + case 0xf0: case 0xf2: case 0xf3: + continue; + + /* + * pushf: NOTE! We should probably not let + * the user see the TF bit being set. But + * it's more pain than it's worth to avoid + * it, and a debugger could emulate this + * all in user space if it _really_ cares. + */ + case 0x9c: + default: + return 0; + } + } + return 0; +} + static void set_singlestep(struct task_struct *child) { - long eflags; + struct pt_regs *regs = get_child_regs(child); + /* + * Always set TIF_SINGLESTEP - this guarantees that + * we single-step system calls etc.. This will also + * cause us to set TF when returning to user mode. + */ set_tsk_thread_flag(child, TIF_SINGLESTEP); - eflags = get_stack_long(child, EFL_OFFSET); - put_stack_long(child, EFL_OFFSET, eflags | TRAP_FLAG); + + /* + * If TF was already set, don't do anything else + */ + if (regs->eflags & TRAP_FLAG) + return; + + /* Set TF on the kernel stack.. */ + regs->eflags |= TRAP_FLAG; + + /* + * ..but if TF is changed by the instruction we will trace, + * don't mark it as being "us" that set it, so that we + * won't clear it by hand later. + */ + if (is_at_popf(child, regs)) + return; + child->ptrace |= PT_DTRACE; } static void clear_singlestep(struct task_struct *child) { - if (child->ptrace & PT_DTRACE) { - long eflags; + /* Always clear TIF_SINGLESTEP... */ + clear_tsk_thread_flag(child, TIF_SINGLESTEP); - clear_tsk_thread_flag(child, TIF_SINGLESTEP); - eflags = get_stack_long(child, EFL_OFFSET); - put_stack_long(child, EFL_OFFSET, eflags & ~TRAP_FLAG); + /* But touch TF only if it was set by us.. */ + if (child->ptrace & PT_DTRACE) { + struct pt_regs *regs = get_child_regs(child); + regs->eflags &= ~TRAP_FLAG; child->ptrace &= ~PT_DTRACE; } } @@ -559,6 +660,8 @@ __attribute__((regparm(3))) void do_syscall_trace(struct pt_regs *regs, int entryexit) { + struct siginfo info; + if (unlikely(current->audit_context)) { if (!entryexit) audit_syscall_entry(current, regs->orig_eax, @@ -573,18 +676,18 @@ return; if (!(current->ptrace & PT_PTRACED)) return; + + memset(&info, 0, sizeof(info)); + info.si_signo = SIGTRAP; + /* the 0x80 provides a way for the tracing parent to distinguish between a syscall stop and SIGTRAP delivery */ - ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD) && - !test_thread_flag(TIF_SINGLESTEP) ? 0x80 : 0)); + info.si_code = SIGTRAP; + if ((current->ptrace & PT_TRACESYSGOOD) && !test_thread_flag(TIF_SINGLESTEP)) + info.si_code = SIGTRAP | 0x80; + info.si_pid = current->tgid; + info.si_uid = current->uid; - /* - * this isn't the same as continuing with a signal, but it will do - * for normal use. strace only continues with a signal if the - * stopping signal is not SIGTRAP. -brl - */ - if (current->exit_code) { - send_sig(current->exit_code, current, 1); - current->exit_code = 0; - } + /* Send us the fakey SIGTRAP */ + send_sig_info(SIGTRAP, &info, current); }

Dec. 30, 2004

Re: Looking for Software which uses the MS Crypto API

by Michael Jung

On Thursday 30 December 2004 18:39, Jesse Allen wrote: > Blizzard's game patching software seems to use it to authenticate the > patch archive. Your changes broke the patcher: "unable to > authenticate", last time I checked, but I am unable to update to the > current cvs at the moment. No source, sorry. I'll check up on it at > a later time. I've downloaded the current Diablo II patch and tricked it into running up to the authentication step by patching my registry. It crashed and allowed me to find and fix a bug. This is exactly what I need. Thanks! Ciao, Michael

Dec. 30, 2004

Re: Looking for Software which uses the MS Crypto API

by Scott Ritchie

I'm pretty sure Steam does. And, coincidentally, Steam happens to be broken at the moment, although it does work in Crossover. Also, I think the open source eMule uses that DLL a bit, IIRC to generate a crypt key for each userid. Previous hacks to get eMule working in Wine involved generating the key in a seperate program and then merging it in - there's an entire thread about it in the eMule forums. -Scott Ritchie On Thu, 2004-12-30 at 17:50 +0100, Michael Jung wrote: > Hello, > > In order to test rsaenh.dll, I'm looking for software, which applies the > Microsoft Crypto-API. Any suggestions? It would be ideal, if the source is > available. > > Ciao, > Michael >

Dec. 30, 2004

Re: Remove heap.h

by Dimitrie O. Paun

On Thu, Dec 30, 2004 at 09:08:58PM +0000, Mike Hearn wrote: > I don't really understand why we can't make this an inline or something, > it seems that it's a lot more convenient than the direct win32 > equivalents. Making it an inline would achieve the same effect as simply > replacing each usage manually, and allow us to improve DLL separation as > well. Because in 99.99% of cases you don't want to convert from W->A. If all our functions are Unicode, a W->A conversion is a warning sign, and for sure you wouldn't want to encourage people to do so by making it any easier. -- Dimi.

Dec. 30, 2004

Re: Remove heap.h

by Mike Hearn

On Thu, 30 Dec 2004 15:41:06 -0500, Dimitrie O. Paun wrote: > To be honest, I'm not too happy with this patch, HEAP_strdupWtoA > was a good marker for code that needed fixing, this patch just > makes those places harder to find. I don't really understand why we can't make this an inline or something, it seems that it's a lot more convenient than the direct win32 equivalents. Making it an inline would achieve the same effect as simply replacing each usage manually, and allow us to improve DLL separation as well.

Dec. 30, 2004

Re: Remove heap.h

by Dimitrie O. Paun

On Thu, Dec 30, 2004 at 07:57:31AM -0800, Jon Griffiths wrote: > Hi, > > After applying, include/heap.h can be removed from cvs. To be honest, I'm not too happy with this patch, HEAP_strdupWtoA was a good marker for code that needed fixing, this patch just makes those places harder to find. -- Dimi.

Dec. 30, 2004

Re: Get rid of DPRINTF in x11drv/opengl.c

by Lionel Ulmer

On Thu, Dec 30, 2004 at 11:10:19AM -0500, Dimitrie O. Paun wrote: > The way to do it is to prepare the output in a memory buffer, and > output it at once. This can be done with the wine_dbg_sprintf() > function. In cases where you need logic to construct the output > (like testing for flags, etc), you can do it by first printing > to a local buffer, and then printing the content of the buffer > to a debug buffer via wine_dbg_sprintf(). A simple example of this > technique can be found in the dlls/kernel/locale.c file, in the > debugstr_lang() function. Yeah, this is another janitorial task that could be put on the list: transform all 'TRACEing helper functions' (a lot in DDraw) to function returning strings to really only have TRACEs calls in 'real' functions and not logging functions. Lionel -- Lionel Ulmer - http://www.bbrox.org/

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Thu, 30 Dec 2004, Jesse Allen wrote: > > Using the latest version of the patch on do_syscall_trace(), it still > doesn't work unless I remove this test. If indeed it's supposed to > fall through to receive the proper signal, (ie to single step properly > after an int op), then removing it is wrong, and I won't consider it > anymore. I also have to use the patch shown below, with a typo-fixed, > to fix the other problem. I broke it apart from the other because we > might want to consider it seperately right now. I'm actually able to now re-create some problems with TF handling with a simple non-wine test, and I think I'm zeroing in on the reason for Wine breaking. And I think it just happened to work by luck before. Not only do we have problems single-stepping over a system call, we _also_ have problems single-stepping over a "popf" - we'll set TF (to single-step), and then afterwards we'll _clear_ TF, even if the "popf" also was supposed to set it. Working on a patch for this right now, I'll send something out soonish (and I'll test it on my test-case before sending it, so that it at least has some chance of working). Linus

Dec. 30, 2004

Re: Wine regression caused by 12/02 18:05:37 UTC commit: breaks Total Annihilation

by Lionel Ulmer

> Word on the street is that we need an implementation of the DirectDraw > clipper, but I have no idea if that's correct or if so, how much of it we > need. Last time I checked, it was not clipper related at all. It's basically (from what I remember - I could be wrong) QuickTime that displays on the desktop window using DirectX (just at the start, after it uses clipper and windows). This means that if we want to make this work, we need first to lock the X server then take a screenshot of the root window then put it in the buffer to give back to QT to draw on it and then re-copy this in what is currently the black window and finally unlock the X server... In other means a bad hack :-) In my opinion, the cleanest would just to not support at all 'window-less' DirectX as the only one using it is QT and it uses it badly :-) Lionel -- Lionel Ulmer - http://www.bbrox.org/

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Jesse Allen

On Thu, 30 Dec 2004 09:59:27 -0800 (PST), Davide Libenzi <davidel(a)xmailserver.org> wrote: > On Wed, 29 Dec 2004, Linus Torvalds wrote: > > > On Wed, 29 Dec 2004, Davide Libenzi wrote: > > > > > > I think same. My test simply let the function processing to let thru and > > > reach the fake signal sending point. > > > > No, your test-case doesn't even send a signal at all, because your > > test-program just uses a PTRACE_SINGLESTEP without any "send signal" - so > > "exit_code" will be zero after the debuggee gets released from the > > "ptrace_notify()", and the suspect code will never be executed.. > > No no, my test case has nothing to do with the extra signal sent in > do_syscall_trace(). But the test I put at the time: > > - if (!test_thread_flag(TIF_SYSCALL_TRACE)) > + if (!test_thread_flag(TIF_SYSCALL_TRACE) && > + !test_thread_flag(TIF_SINGLESTEP)) > return; > > will make the code to not execute the "return" (in the single step case) > and to fall through the point where the signal is sent. Using the latest version of the patch on do_syscall_trace(), it still doesn't work unless I remove this test. If indeed it's supposed to fall through to receive the proper signal, (ie to single step properly after an int op), then removing it is wrong, and I won't consider it anymore. I also have to use the patch shown below, with a typo-fixed, to fix the other problem. I broke it apart from the other because we might want to consider it seperately right now. I spent some time speaking to my brother about this. He has done his own kernel before for an embedded system. He was rather frustrated with me trying to find a reason for why a rather simple change broke my program. He told me I couldn't have it both ways. However I believe that I don't understand the linux code well enough to make that conclusion. > > > > The problem I think I see (and which the comment alludes to) is that if > > the controlling process continues the debuggee with a signal, we'll be > > doing the wrong thing: the code in do_syscall_trace() will take that > > signal (in "current->exit_code") and send it as a real signal to the > > debuggee, but since it is debugged, it will be caught (again) by the > > controlling process, which apparently in the case of Wine gets very > > confused. > > > > So I _think_ the problem happens for the following schenario: > > - wine for some reason does a PTRACE_SINGLESTEP over a system call > > - after the single-step, wine ends up trying to get the sub-process to > > enter a signal handler with ptrace( PTRACE_CONT, ... sig) > > - the normal ptrace path (where we trap a signal - see kernel/signal.c > > and the "ptrace_stop()" logic) handles this correctly, but > > do_syscall_trace() will do a "send_sig()" > > - we'll try to handle the signal when returning to the traced process > > - now "get_signal_to_deliver()" will invoce the ptrace logic AGAIN, and > > call ptrace_stop() for this fake signal, and we'll report a new event > > to the controlling process. > > > > Does this make sense? > > This might explain what they were seeing, but OTOH it seems that the real > cause of their problems is related to something else (according to other > emails on this thread). > > Actually, I don't think the vanilla kernel has the code that breaks those other wine programs. I just learned of this yesterday and it's not related. I believe it's only in fedora core 3 or -ac kernels and I use vanilla kernels. Jesse --- linux/arch/i386/kernel/ptrace.c 2004-12-29 14:10:34.000000000 -0700 +++ linux-mod/arch/i386/kernel/ptrace.c 2004-12-29 20:50:00.000000000 -0700 @@ -142,18 +142,31 @@ { long eflags; + /* + * Always set TIF_SINGLESTEP - this guarantees that + * we single-step system calls etc.. + */ set_tsk_thread_flag(child, TIF_SINGLESTEP); + + /* + * If TF was already set, don't do anything else + */ eflags = get_stack_long(child, EFL_OFFSET); + if (eflags & TRAP_FLAG) + return; put_stack_long(child, EFL_OFFSET, eflags | TRAP_FLAG); child->ptrace |= PT_DTRACE; } static void clear_singlestep(struct task_struct *child) { + /* Always clear TIF_SINGLESTEP... */ + clear_tsk_thread_flag(child, TIF_SINGLESTEP); + + /* But touch TF only if it was set by us.. */ if (child->ptrace & PT_DTRACE) { long eflags; - clear_tsk_thread_flag(child, TIF_SINGLESTEP); eflags = get_stack_long(child, EFL_OFFSET); put_stack_long(child, EFL_OFFSET, eflags & ~TRAP_FLAG); child->ptrace &= ~PT_DTRACE;

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Davide Libenzi

On Wed, 29 Dec 2004, Linus Torvalds wrote: > On Wed, 29 Dec 2004, Davide Libenzi wrote: > > > > I think same. My test simply let the function processing to let thru and > > reach the fake signal sending point. > > No, your test-case doesn't even send a signal at all, because your > test-program just uses a PTRACE_SINGLESTEP without any "send signal" - so > "exit_code" will be zero after the debuggee gets released from the > "ptrace_notify()", and the suspect code will never be executed.. > > The problem I think I see (and which the comment alludes to) is that if > the controlling process continues the debuggee with a signal, we'll be > doing the wrong thing: the code in do_syscall_trace() will take that > signal (in "current->exit_code") and send it as a real signal to the > debuggee, but since it is debugged, it will be caught (again) by the > controlling process, which apparently in the case of Wine gets very > confused. > > So I _think_ the problem happens for the following schenario: > - wine for some reason does a PTRACE_SINGLESTEP over a system call > - after the single-step, wine ends up trying to get the sub-process to > enter a signal handler with ptrace( PTRACE_CONT, ... sig) > - the normal ptrace path (where we trap a signal - see kernel/signal.c > and the "ptrace_stop()" logic) handles this correctly, but > do_syscall_trace() will do a "send_sig()" > - we'll try to handle the signal when returning to the traced process > - now "get_signal_to_deliver()" will invoce the ptrace logic AGAIN, and > call ptrace_stop() for this fake signal, and we'll report a new event > to the controlling process. Make sense to me. What about the one below? The problem is though, that we have two different events here. One is the single step trap and the other one is the signal. And according to the comment, strace want something different from SIGTRAP to continue with signal. So it wants the double event we are actually sending (ptrace_notify + send_sig). OTOH Wine does not seem to like the double event thingy. Hmm ...? - Davide --- ptrace.c.orig 2004-12-30 10:29:11.000000000 -0800 +++ ptrace.c 2004-12-30 11:11:23.000000000 -0800 @@ -573,18 +573,14 @@ return; if (!(current->ptrace & PT_PTRACED)) return; - /* the 0x80 provides a way for the tracing parent to distinguish - between a syscall stop and SIGTRAP delivery */ - ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD) && - !test_thread_flag(TIF_SINGLESTEP) ? 0x80 : 0)); - /* - * this isn't the same as continuing with a signal, but it will do - * for normal use. strace only continues with a signal if the - * stopping signal is not SIGTRAP. -brl - */ if (current->exit_code) { - send_sig(current->exit_code, current, 1); + ptrace_notify(current->exit_code); current->exit_code = 0; + } else { + /* the 0x80 provides a way for the tracing parent to distinguish + between a syscall stop and SIGTRAP delivery */ + ptrace_notify(SIGTRAP | ((current->ptrace & PT_TRACESYSGOOD) && + !test_thread_flag(TIF_SINGLESTEP) ? 0x80 : 0)); } }

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Linus Torvalds

On Thu, 30 Dec 2004, Davide Libenzi wrote: > > This might explain what they were seeing, but OTOH it seems that the real > cause of their problems is related to something else (according to other > emails on this thread). There's two different problems: the one seen by Thomas (the Xilinx FPGA synthesizer), which is apparently just due to Wine (or, more likely, the Windows app itself) depending on a certain memory layout for the stack and/or other allocations. That one I think we can consider solved, and indeed had nothing to do with TF. The other one is the copy-protection code breaking for some game (Warcraft) for Jesse Allen, and that one is definitely TF-related.. Jesse can fix it with patches, but those patches aren't acceptable for other uses, so that's why I'm trying to find something that DTRT both for Wine and for a regular debugging session.. Linus

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Davide Libenzi

On Wed, 29 Dec 2004, Linus Torvalds wrote: > On Wed, 29 Dec 2004, Davide Libenzi wrote: > > > > I think same. My test simply let the function processing to let thru and > > reach the fake signal sending point. > > No, your test-case doesn't even send a signal at all, because your > test-program just uses a PTRACE_SINGLESTEP without any "send signal" - so > "exit_code" will be zero after the debuggee gets released from the > "ptrace_notify()", and the suspect code will never be executed.. No no, my test case has nothing to do with the extra signal sent in do_syscall_trace(). But the test I put at the time: - if (!test_thread_flag(TIF_SYSCALL_TRACE)) + if (!test_thread_flag(TIF_SYSCALL_TRACE) && + !test_thread_flag(TIF_SINGLESTEP)) return; will make the code to not execute the "return" (in the single step case) and to fall through the point where the signal is sent. > The problem I think I see (and which the comment alludes to) is that if > the controlling process continues the debuggee with a signal, we'll be > doing the wrong thing: the code in do_syscall_trace() will take that > signal (in "current->exit_code") and send it as a real signal to the > debuggee, but since it is debugged, it will be caught (again) by the > controlling process, which apparently in the case of Wine gets very > confused. > > So I _think_ the problem happens for the following schenario: > - wine for some reason does a PTRACE_SINGLESTEP over a system call > - after the single-step, wine ends up trying to get the sub-process to > enter a signal handler with ptrace( PTRACE_CONT, ... sig) > - the normal ptrace path (where we trap a signal - see kernel/signal.c > and the "ptrace_stop()" logic) handles this correctly, but > do_syscall_trace() will do a "send_sig()" > - we'll try to handle the signal when returning to the traced process > - now "get_signal_to_deliver()" will invoce the ptrace logic AGAIN, and > call ptrace_stop() for this fake signal, and we'll report a new event > to the controlling process. > > Does this make sense? This might explain what they were seeing, but OTOH it seems that the real cause of their problems is related to something else (according to other emails on this thread). - Davide

Dec. 30, 2004

Re: Looking for Software which uses the MS Crypto API

by Jesse Allen

On Thu, 30 Dec 2004 17:50:09 +0100, Michael Jung <mjung(a)iss.tu-darmstadt.de> wrote: > Hello, > > In order to test rsaenh.dll, I'm looking for software, which applies the > Microsoft Crypto-API. Any suggestions? It would be ideal, if the source is > available. > > Ciao, > Michael > > Blizzard's game patching software seems to use it to authenticate the patch archive. Your changes broke the patcher: "unable to authenticate", last time I checked, but I am unable to update to the current cvs at the moment. No source, sorry. I'll check up on it at a later time.

Dec. 30, 2004

Re: Get rid of DPRINTF in wininet

by Diego 'Flameeyes' Pettenò

On Thursday 30 December 2004 17:15, Dimitrie O. Paun wrote: > I'm afraid it is <g>. This is a case where the wine_dbg_sprintf() > business is in order. This time at least I know what I did wrong ^^ I'll fix these up as soon as I understand how wine_dbg_sprintf() works :) -- Diego "Flameeyes" Pettenò flameeyes(a)users.berlios.de - http://flameeyes.web.ctonet.it/

Dec. 30, 2004

Looking for Software which uses the MS Crypto API

by Michael Jung

Hello, In order to test rsaenh.dll, I'm looking for software, which applies the Microsoft Crypto-API. Any suggestions? It would be ideal, if the source is available. Ciao, Michael

Dec. 30, 2004

Re: Get rid of DPRINTF in opengl32

by Dimitrie O. Paun

On Thu, Dec 30, 2004 at 12:37:12PM +0100, Diego 'Flameeyes' Pettenò wrote: > Hi, > this time is a very small patch, it simply replace DPRINTF with TRACE without > oher changes. Once again, a job for wine_dbg_sprintf(). -- Dimi.

Dec. 30, 2004

Re: Get rid of DPRINTF in wininet

by Dimitrie O. Paun

On Thu, Dec 30, 2004 at 12:33:52PM +0100, Diego 'Flameeyes' Pettenò wrote: > I hope only my usual karma is not here this time or I'll make everybody lost > time ^^ I'm afraid it is <g>. This is a case where the wine_dbg_sprintf() business is in order. -- Dimi.

Dec. 30, 2004

Re: Get rid of DPRINTF in winedos

by Dimitrie O. Paun

On Thu, Dec 30, 2004 at 12:22:18PM +0100, Diego 'Flameeyes' Pettenò wrote: > Hi, > The attached patch replaces DPRINTF -> TRACE in winedos dll, joining the near > DPRINTF used to tace dos interrupts. This looks good, but do we still need the explicit 'if (TRACE_ON(relay))'? I think we can use the explicit one from TRACE(), no? -- Dimi.

Dec. 30, 2004

Re: Get rid of DPRINTF in x11drv/opengl.c

by Dimitrie O. Paun

On Thu, Dec 30, 2004 at 12:04:23PM +0100, Diego 'Flameeyes' Pettenò wrote: > I had some spare time, and I wanted to do some mostly non-designing code, so I > took the time to look at wine's janitorial projects, and I took a look at the > DPRINTF -> TRACE conversion. > > The attached patch removes DPRINTFs from x11drv/opengl.c (which is the only > file in x11drv which uses them), and replacing the 6 different following > calls with a single call for bits and buffer counters. This is a good project to work on, but the reason it's not done yet it's because it is not a simple replacement of DPRINTF with TRACE. Doing stuff like: + TRACE(" - size / version : %d / %d\n", ppfd->nSize, ppfd->nVersion); + TRACE(" - dwFlags : "); Will not work correctly in a multitreaded environment, where the lines may get intertwinded on output with statements from other threads. The way to do it is to prepare the output in a memory buffer, and output it at once. This can be done with the wine_dbg_sprintf() function. In cases where you need logic to construct the output (like testing for flags, etc), you can do it by first printing to a local buffer, and then printing the content of the buffer to a debug buffer via wine_dbg_sprintf(). A simple example of this technique can be found in the dlls/kernel/locale.c file, in the debugstr_lang() function. -- Dimi. -- Dimi.

Dec. 30, 2004

Re: Wine regression caused by 12/02 18:05:37 UTC commit: breaks Total Annihilation

by Mike Hearn

On Thu, 30 Dec 2004 15:25:20 +0100, Michael Stefaniuc wrote: > Now i get the stupid "make the whole display black" Quicktime thingy That's a bug in DirectDraw, we have a bad hack in CrossOver for it but it needs to be fixed properly. Word on the street is that we need an implementation of the DirectDraw clipper, but I have no idea if that's correct or if so, how much of it we need. thanks -mike

Dec. 30, 2004

Re: Wine regression caused by 12/02 18:05:37 UTC commit: breaks Total Annihilation

by Michael Stefaniuc

Eric Pouech wrote: >> Is anyone still looking into this? >> >> I'm seeing the same problem here (below messages repeated and 100% CPU >> usage) >> on RH 9 with the direct sound regression test and some games. > > does this (new) patch help ? Well, i don't know if it fixes their problem but at least it makes Quicktime stop looping (wineserver and a wine-preloader process are maxing out the CPU) for me. Now i get the stupid "make the whole display black" Quicktime thingy and then it crashes: wine: Unhandled exception (thread 001d), starting debugger... WineDbg starting on pid 0x1c Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00000000). In 32 bit mode. Register dump: CS:0073 SS:007b DS:007b ES:007b FS:003b GS:0033 EIP:00000000 ESP:7791fdd5 EBP:7791fe34 EFLAGS:00010202( - 00 - -RI1) EAX:6e4f8a00 EBX:00000000 ECX:00000000 EDX:77c86284 ESI:7791fe58 EDI:7791fdf5 Stack dump: 0x7791fdd5: 6e31fbec 00000000 06000000 00000002 0x7791fde5: 7174696d 7791feb0 0046d24e 00000000 0x7791fdf5: bb7791fe 0077c511 00030000 05000000 0x7791fe05: 00c00000 00004000 1f000000 00000000 0x7791fe15: 00000000 f8030000 b97791fd 947791f9 0x7791fe25: 587791fe 686e35c6 006e43ad a0000000 Backtrace: =>1 0x00000000 (0x7791fe34) 2 0x00448e51err:dbghelp_msc:pdb_process_internal -Unable to peruse .PDB file C:\qtml\buildresults\nosym\QuickTimePlayer.pdb in quicktimeplayer (+0x48e51) (0x7791fea0) 3 0x0046d32e EntryPoint+0xe0 in quicktimeplayer (0x7791ff2c) 4 0x77baf49b start_process+0xc3(arg=0x0) [/home/michi/work/wine/dlls/kernel/process.c:1046] in kernel32 (0x7791fff4) 5 0xb7fcb69d wine_pthread_abort_thread+0x29(status=0x0) [/home/michi/work/wine/libs/wine/port.c:85] in libwine.so.1 (0x00000000) 0x00000000: addb %al,0x0(%eax) Wine-dbg> Hope this helps (if it's related). bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 System Administration Fax.: +49-711-96437-111 Red Hat GmbH Email: mstefani(a)redhat.com Hauptstaetterstr. 58 http://www.redhat.de/ D-70178 Stuttgart

Dec. 30, 2004

Re: another build problem on current cvs

by Ferenc Wagner

Vincent Béron <vberon(a)mecano.gme.usherb.ca> writes: > Le mer 29/12/2004 à 05:38, Joris Huizer a écrit : > >> In debian the package is called libasound2-dev, version >> "0.9.0beta10.9.0beta1" which should be the 0.9 branch of alsa > > Either update to alsa 1.0, downgrade Wine to prior > 20040505, or switch to the wineoss driver. For Debian stable I've got some unofficial ALSA packages (among others) at http://afavant.elte.hu/~wferi/debian/ which may come handy. -- Feri.

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Mike Hearn

On Thu, 2004-12-30 at 11:09 +0100, Thomas Sailer wrote: > On Wed, 2004-12-29 at 20:15 -0800, Andrew Morton wrote: > > You can globally disable flex-mmap with > > > > echo 1 > /proc/sys/vm/legacy_va_layout > > > > Does it fix things? > > Haven't tried. But setarch i386 -L /usr/bin/wine ... did fix it. > > Tom Tom, does this patch against Wine help? It should do the same thing as the setarch program, so if that fixes it then this should also (if I've understood how this mechanism works of course). Mike Hearn <mh(a)codeweavers.com> Set the Linux ABI personality to get a legacy VM layout

Dec. 30, 2004

NLS/IME functions

by Diego 'Flameeyes' Pettenò

Hi, I'm still trying to figure out how nls/ime functions are placed in windows systems to fix up the patch I sent some days ago. I haven't had the time to find out if YuGiOh calls GetProcAddress (because isn't mine, it's of a friend of mine which is trying to get it working, so I need him to test), so I'm fooling around checking for a way to fix this up. At least Win98's and Win2k's user32.dll have the functions defined into winnls32.h, also if they are defined as compatibility with winnt3. At the moment on wine these functions are commented out also as stubs so they are completely missing. On Windows I'm not able to find a winnls32.dll which is instead present on wine, and seems to define similar functions (but they are quite all spec-stubs, but three functions which are defined (stubbing), without a STUB trace. How should be this done? Implement stub-functions for every nls/ime function? Or simply ignore them for now? Thanks for the time, -- Diego "Flameeyes" Pettenò flameeyes(a)users.berlios.de - http://flameeyes.web.ctonet.it/

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Mike Hearn

On Wed, 2004-12-29 at 18:10 -0800, Linus Torvalds wrote: > Some wine person would need to inform us about what the seh exception > thing means.. "code c0000005"? STATUS_ACCESS_VIOLATION, or the Win32 equivalent of a segfault. It would appear that the ptrace changes are not responsible here, though if changing the kernel really does change this crash then maybe there is another issue we haven't uncovered yet. thanks -mike

Dec. 30, 2004

Re: ptrace single-stepping change breaks Wine

by Thomas Sailer

On Wed, 2004-12-29 at 20:15 -0800, Andrew Morton wrote: > You can globally disable flex-mmap with > > echo 1 > /proc/sys/vm/legacy_va_layout > > Does it fix things? Haven't tried. But setarch i386 -L /usr/bin/wine ... did fix it. Tom

Dec. 30, 2004

Re: Wine regression caused by 12/02 18:05:37 UTC commit: breaks Total Annihilation

by Eric Pouech

> Is anyone still looking into this? > > I'm seeing the same problem here (below messages repeated and 100% CPU > usage) > on RH 9 with the direct sound regression test and some games. does this (new) patch help ? A+

Dec. 30, 2004