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

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