Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)
2009/1/26 Aurimas Fišeras <aurimas(a)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 <aurimas(a)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
Rob Shearman <robertshearman(a)gmail.com> writes:
2009/1/26 Aurimas Fišeras <aurimas(a)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 julliard(a)winehq.org
Alexandre Julliard wrote:
Rob Shearman <robertshearman(a)gmail.com> writes:
2009/1/26 Aurimas Fišeras <aurimas(a)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.
Aurimas Fišeras <aurimas(a)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 julliard(a)winehq.org
Alexandre Julliard wrote:
Aurimas Fišeras <aurimas(a)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?
Aurimas Fišeras <aurimas(a)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 julliard(a)winehq.org
Alexandre Julliard wrote:
Aurimas Fišeras <aurimas(a)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"?
Aurimas Fišeras <aurimas(a)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 julliard(a)winehq.org
Alexandre Julliard wrote:
Aurimas Fišeras <aurimas(a)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?
Aurimas Fišeras <aurimas(a)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 julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Aurimas Fišeras -
Rob Shearman