From 8695be5049294c32c45ef69f8a500dd759f46079 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Mon, 2 Jul 2012 14:41:31 -0400 Subject: [PATCH 1/4] Fix regressions in arch_cpu_user_strlcpy(). - repnz movsb turns out to not actually be a legal instruction, resulting in various strings being copied incorrectly, leading to random crashes in various places. Rework to use loop instead. Thanks to Alex Smith for helping review changes and offering improvements. - Minor cleanups. - Fixes #8650 properly. --- src/system/kernel/arch/x86/arch_x86.S | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/system/kernel/arch/x86/arch_x86.S b/src/system/kernel/arch/x86/arch_x86.S index 58e20fc8cc..f731299222 100644 --- a/src/system/kernel/arch/x86/arch_x86.S +++ b/src/system/kernel/arch/x86/arch_x86.S @@ -276,19 +276,32 @@ FUNCTION(arch_cpu_user_strlcpy): /* Copy at most count - 1 bytes */ dec %ecx - /* move data by bytes */ + /* If count is now 0, skip straight to null terminating + as our loop will otherwise overflow */ + cmp $0,%ecx + jne .L_user_strlcpy_copy_begin + movb $0,(%edi) + jmp .L_user_strlcpy_source_count + +.L_user_strlcpy_copy_begin: cld - repnz +.L_user_strlcpy_copy_loop: + /* move data by bytes */ movsb + cmpb $0,-1(%esi) + je .L_user_strlcpy_copy_loop_done + loop .L_user_strlcpy_copy_loop - /* null terminate string */ - movb $0,(%edi) - dec %esi - +.L_user_strlcpy_copy_loop_done: /* check if we copied the entire source string */ cmp $0,%ecx jne .L_user_strlcpy_source_done +.L_user_strlcpy_zero_terminate: + /* null terminate string */ + movb $0,(%edi) + dec %esi + /* count remaining bytes in src */ .L_user_strlcpy_source_count: not %ecx @@ -297,11 +310,9 @@ FUNCTION(arch_cpu_user_strlcpy): scasb .L_user_strlcpy_source_done: - movl %esi,%eax subl 20(%esp),%eax subl $1,%eax - /* restore the old fault handler */ movl %ebx,(%edx) From dc5a16bb70c95c64b1203682acf5a67e19b2b882 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Mon, 2 Jul 2012 15:42:38 -0400 Subject: [PATCH 2/4] Add TODO note with respect to needed asm implementations. --- src/system/kernel/arch/arm/arch_cpu.cpp | 2 ++ src/system/kernel/arch/m68k/arch_cpu.cpp | 2 ++ src/system/kernel/arch/ppc/arch_cpu.cpp | 3 +++ 3 files changed, 7 insertions(+) diff --git a/src/system/kernel/arch/arm/arch_cpu.cpp b/src/system/kernel/arch/arm/arch_cpu.cpp index e225f9cef8..65d4795a80 100644 --- a/src/system/kernel/arch/arm/arch_cpu.cpp +++ b/src/system/kernel/arch/arm/arch_cpu.cpp @@ -161,6 +161,8 @@ arch_cpu_user_TLB_invalidate(void) } +// TODO: all functions that use fault handlers need to be implemented +// in assembly due to problems passing in label addresses in gcc4. status_t arch_cpu_user_memcpy(void *to, const void *from, size_t size, addr_t *faultHandler) diff --git a/src/system/kernel/arch/m68k/arch_cpu.cpp b/src/system/kernel/arch/m68k/arch_cpu.cpp index 235c5aabaf..488e905c19 100644 --- a/src/system/kernel/arch/m68k/arch_cpu.cpp +++ b/src/system/kernel/arch/m68k/arch_cpu.cpp @@ -171,6 +171,8 @@ arch_cpu_user_TLB_invalidate(void) } +// TODO: all functions that use fault handlers need to be implemented +// in assembly due to problems passing in label addresses in gcc4. status_t arch_cpu_user_memcpy(void *to, const void *from, size_t size, addr_t *faultHandler) diff --git a/src/system/kernel/arch/ppc/arch_cpu.cpp b/src/system/kernel/arch/ppc/arch_cpu.cpp index 79c62967f0..80a76d4294 100644 --- a/src/system/kernel/arch/ppc/arch_cpu.cpp +++ b/src/system/kernel/arch/ppc/arch_cpu.cpp @@ -172,6 +172,9 @@ arch_cpu_user_TLB_invalidate(void) } +// TODO: all functions that use fault handlers need to be implemented +// in assembly due to problems passing in label addresses in gcc4. + status_t arch_cpu_user_memcpy(void *to, const void *from, size_t size, addr_t *faultHandler) From 29291c8c924ada62bf015aaf70d04616cc2e7314 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Mon, 2 Jul 2012 19:42:20 -0400 Subject: [PATCH 3/4] Cleanups/optimizations. --- src/system/kernel/arch/x86/arch_x86.S | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/system/kernel/arch/x86/arch_x86.S b/src/system/kernel/arch/x86/arch_x86.S index f731299222..d5c51507f7 100644 --- a/src/system/kernel/arch/x86/arch_x86.S +++ b/src/system/kernel/arch/x86/arch_x86.S @@ -278,9 +278,8 @@ FUNCTION(arch_cpu_user_strlcpy): /* If count is now 0, skip straight to null terminating as our loop will otherwise overflow */ - cmp $0,%ecx - jne .L_user_strlcpy_copy_begin - movb $0,(%edi) + jnz .L_user_strlcpy_copy_begin + movb $0,(%edi) jmp .L_user_strlcpy_source_count .L_user_strlcpy_copy_begin: @@ -289,30 +288,24 @@ FUNCTION(arch_cpu_user_strlcpy): /* move data by bytes */ movsb cmpb $0,-1(%esi) - je .L_user_strlcpy_copy_loop_done + je .L_user_strlcpy_source_done loop .L_user_strlcpy_copy_loop -.L_user_strlcpy_copy_loop_done: - /* check if we copied the entire source string */ - cmp $0,%ecx - jne .L_user_strlcpy_source_done - -.L_user_strlcpy_zero_terminate: /* null terminate string */ movb $0,(%edi) - dec %esi + dec %esi /* count remaining bytes in src */ .L_user_strlcpy_source_count: not %ecx - movb $0,%al + xor %al,%al repnz scasb .L_user_strlcpy_source_done: movl %esi,%eax subl 20(%esp),%eax - subl $1,%eax + dec %eax /* restore the old fault handler */ movl %ebx,(%edx) From 7f7b659ec5eb4911c6a62e86af3a379227c3eeb0 Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Mon, 2 Jul 2012 20:17:03 -0400 Subject: [PATCH 4/4] Use safer but slower approach to copying the string. --- src/system/kernel/arch/x86/arch_x86.S | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/system/kernel/arch/x86/arch_x86.S b/src/system/kernel/arch/x86/arch_x86.S index d5c51507f7..1ebcae0e82 100644 --- a/src/system/kernel/arch/x86/arch_x86.S +++ b/src/system/kernel/arch/x86/arch_x86.S @@ -286,9 +286,10 @@ FUNCTION(arch_cpu_user_strlcpy): cld .L_user_strlcpy_copy_loop: /* move data by bytes */ - movsb - cmpb $0,-1(%esi) - je .L_user_strlcpy_source_done + lodsb + stosb + test %al,%al + jz .L_user_strlcpy_source_done loop .L_user_strlcpy_copy_loop /* null terminate string */