Re: advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Rob Shearman]

2009/1/26 Aurimas Fišeras :
> 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 
> 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.

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Alexandre Julliard]

Rob Shearman  writes:
> 2009/1/26 Aurimas Fišeras :
> > 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.

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Aurimas Fišeras]

Alexandre Julliard wrote:
> Rob Shearman  writes:
> > 2009/1/26 Aurimas Fišeras :
> > > 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.

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Alexandre Julliard]

Aurimas Fišeras  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.

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Aurimas Fišeras]

Alexandre Julliard wrote:
> Aurimas Fišeras  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?

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Alexandre Julliard]

Aurimas Fišeras  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.

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Aurimas Fišeras]

Alexandre Julliard wrote:
> Aurimas Fišeras  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"?

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Alexandre Julliard]

Aurimas Fišeras  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.

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Aurimas Fišeras]

Alexandre Julliard wrote:
> Aurimas Fišeras  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?

advapi32: Fix potential NULL pointer dereference in RegSetValueExA [with test] (Saturn)

[Alexandre Julliard]

Aurimas Fišeras  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?
> - change get_process_exe_module() to
> return LIST_ENTRY( ptr, struct process_dll, entry );
> - change debugger.c:160 to
> if (exe_module && exe_module->file &&
> - inspect list_head()?
> - ignore it?

Ignore it, the process will always have a module at that point.