Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Aurimas Fišeras auri...@gmail.com writes: But what about this and similar situations? There's no single answer, each situation is different, you have to study the code flow to understand what can and cannot happen. What should I do? 1. change get_process_exe_module() to return LIST_ENTRY( ptr, struct process_dll, entry ); 2. change debugger.c:160 to if (exe_module exe_module-file 3. inspect list_head()? 4. ignore it? Ignore it, the process will always have a module at that point. -- Alexandre Julliard julli...@winehq.org
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Alexandre Julliard wrote:
Aurimas Fišeras auri...@gmail.com writes:
How to know when to fix NULL pointer dereferences if in most such cases
code flow can be traced back to a windows API called by other program?
If some other program is really calling it with NULL then you can fix
it. You can't preemptively fix every API that takes a pointer.
OK, I won't fix any windows API if no program calls it, although I
don't believe that is right.
But what about this and similar situations?
Error report:
server/debugger.c:160 red Possible NULL dereference of exe_module
Intraprocedural Null error
143: struct process_dll *exe_module = get_process_exe_module( process );
...
160: if (exe_module-file
process.h:
static inline struct process_dll *get_process_exe_module( struct process
*process )
{
struct list *ptr = list_head( process-dlls );
return ptr ? LIST_ENTRY( ptr, struct process_dll, entry ) : NULL;
}
What should I do?
1. change get_process_exe_module() to
return LIST_ENTRY( ptr, struct process_dll, entry );
2. change debugger.c:160 to
if (exe_module exe_module-file
3. inspect list_head()?
4. ignore it?
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
2009/1/26 Aurimas Fišeras auri...@gmail.com:
Saturn's error report:
(INCONSISTENT USE) Possible null dereference of variable data+(count-1).
This variable is checked for Null at lines: registry.c:1051
Tested on Windows XP
Changelog:
advapi32: Fix potential NULL pointer dereference in RegSetValueExA
[with test] (Saturn)
Excellent, this tool has spotted a corner-case that the code doesn't
handle correctly.
From ea7773cc046992e327030fb99935afc5b25c1b4b Mon Sep 17 00:00:00 2001
From: Aurimas Fischer auri...@gmail.com
Date: Mon, 26 Jan 2009 21:55:05 +0200
Subject: advapi32: Fix potential NULL pointer dereference in RegSetValueExA
[with test] (Saturn)
---
dlls/advapi32/registry.c |1 +
dlls/advapi32/tests/registry.c |4
2 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/dlls/advapi32/registry.c b/dlls/advapi32/registry.c
index 52de6c5..88a89db 100644
--- a/dlls/advapi32/registry.c
+++ b/dlls/advapi32/registry.c
@@ -1055,6 +1055,7 @@ LSTATUS WINAPI RegSetValueExA( HKEY hkey, LPCSTR name,
DWORD reserved, DWORD typ
else if (count is_string(type))
{
/* if user forgot to count terminating null, add it (yes NT does
this) */
+if (!data) return ERROR_NOACCESS;
This should be moved before the comment to avoid the comment relating
to the wrong line.
if (data[count-1] !data[count]) count++;
}
diff --git a/dlls/advapi32/tests/registry.c b/dlls/advapi32/tests/registry.c
index b63b3e2..0e1b673 100644
--- a/dlls/advapi32/tests/registry.c
+++ b/dlls/advapi32/tests/registry.c
@@ -383,6 +383,10 @@ static void test_set_value(void)
test_hkey_main_Value_A(name2A, string2A, sizeof(string2A));
test_hkey_main_Value_W(name2W, string2W, sizeof(string2W));
+/* test RegSetValueExA with invalid parameters*/
+ret = RegSetValueExA(hkey_main, name1A, 0, REG_SZ, NULL, 1);
+ok(ret == ERROR_NOACCESS, got %d (expected ERROR_NOACCESS)\n, ret);
From the source of RegSetValueExA, this appears to return
ERROR_INVALID_PARAMETER on Win9x, so this test case needs to be run on
Win9x and fixed if necessary to take this into account.
--
Rob Shearman
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Rob Shearman robertshear...@gmail.com writes: 2009/1/26 Aurimas Fišeras auri...@gmail.com: Saturn's error report: (INCONSISTENT USE) Possible null dereference of variable data+(count-1). This variable is checked for Null at lines: registry.c:1051 Tested on Windows XP Changelog: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn) Excellent, this tool has spotted a corner-case that the code doesn't handle correctly. I'm not convinced that this is really a bug. If a non-zero count is specified it's quite reasonable to expect data to be valid. Of course Windows has exception handlers all over the place, but that doesn't mean we want to replicate that behavior. -- Alexandre Julliard julli...@winehq.org
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Alexandre Julliard wrote: Rob Shearman robertshear...@gmail.com writes: 2009/1/26 Aurimas Fišeras auri...@gmail.com: Saturn's error report: (INCONSISTENT USE) Possible null dereference of variable data+(count-1). This variable is checked for Null at lines: registry.c:1051 Tested on Windows XP Changelog: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn) Excellent, this tool has spotted a corner-case that the code doesn't handle correctly. I'm not convinced that this is really a bug. If a non-zero count is specified it's quite reasonable to expect data to be valid. It is also quite reasonable to expect that a function won't crash with all legal parameters. Of course Windows has exception handlers all over the place, but that doesn't mean we want to replicate that behavior. But we want to have a bug-for-bug compatibility with Windows? Without this patch windows just returns an error, while wine crashes. There are dozens of similar corner-case errors where Windows crashes as well as wine, but this time only wine crashes.
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Aurimas Fišeras auri...@gmail.com writes: Alexandre Julliard wrote: Of course Windows has exception handlers all over the place, but that doesn't mean we want to replicate that behavior. But we want to have a bug-for-bug compatibility with Windows? Without this patch windows just returns an error, while wine crashes. We only want it when an actual app depends on it, otherwise we'd have to add exception handlers in all functions. Note that the Windows behavior often varies across versions too. -- Alexandre Julliard julli...@winehq.org
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Alexandre Julliard wrote: Aurimas Fišeras auri...@gmail.com writes: Alexandre Julliard wrote: Of course Windows has exception handlers all over the place, but that doesn't mean we want to replicate that behavior. But we want to have a bug-for-bug compatibility with Windows? Without this patch windows just returns an error, while wine crashes. We only want it when an actual app depends on it, otherwise we'd have to add exception handlers in all functions. Note that the Windows behavior often varies across versions too. So why are we fixing various Possible NULL pointer dereference errors reported by Coverity?
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Aurimas Fišeras auri...@gmail.com writes: Alexandre Julliard wrote: We only want it when an actual app depends on it, otherwise we'd have to add exception handlers in all functions. Note that the Windows behavior often varies across versions too. So why are we fixing various Possible NULL pointer dereference errors reported by Coverity? That's different, it's for cases where inspection of the code flow demonstrates that a NULL dereference can actually happen. In this case it would mean that some other part of the Wine code calls RegSetValueExA with a NULL pointer. Most likely the proper fix would then be in the caller. -- Alexandre Julliard julli...@winehq.org
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Alexandre Julliard wrote: Aurimas Fišeras auri...@gmail.com writes: Alexandre Julliard wrote: We only want it when an actual app depends on it, otherwise we'd have to add exception handlers in all functions. Note that the Windows behavior often varies across versions too. So why are we fixing various Possible NULL pointer dereference errors reported by Coverity? That's different, it's for cases where inspection of the code flow demonstrates that a NULL dereference can actually happen. In this case it would mean that some other part of the Wine code calls RegSetValueExA with a NULL pointer. But that some other part of Wine was possibly called from external program via windows API? Most likely the proper fix would then be in the caller. I still don't see the clear difference between these cases. If FunctionA calls FunctionW with (possibly) NULL pointer and FunctionW dereferences it we should fix FunctionA? But since FunctionA is windows API and it is far more likely to be called not from Wine itself but from other programs, we don't fix neither FunctionA nor FunctionW, but expect that other programs will behave and won't call neither FunctionA nor FunctionW (nor FunctionX that calls FunctionA) with NULL pointers? How to know when to fix NULL pointer dereferences if in most such cases code flow can be traced back to a windows API called by other program?
Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
Aurimas Fišeras auri...@gmail.com writes: If FunctionA calls FunctionW with (possibly) NULL pointer and FunctionW dereferences it we should fix FunctionA? If FunctionW requires a valid pointer, then yes of course the caller should be fixed. Just making FunctionW return NOACCESS instead of crashing doesn't fix anything, it just hides the bug. Sadly, Microsoft likes to hide bugs instead of fixing them, and we sometimes have to do the same to remain compatible, but we try to keep it to a minimum. But since FunctionA is windows API and it is far more likely to be called not from Wine itself but from other programs, we don't fix neither FunctionA nor FunctionW, but expect that other programs will behave and won't call neither FunctionA nor FunctionW (nor FunctionX that calls FunctionA) with NULL pointers? Yes. You can't check for NULL before every single dereference, that's madness. How to know when to fix NULL pointer dereferences if in most such cases code flow can be traced back to a windows API called by other program? If some other program is really calling it with NULL then you can fix it. You can't preemptively fix every API that takes a pointer. -- Alexandre Julliard julli...@winehq.org
