[PATCH v2 2/2] advapi32: Don't read the key name if the root key handle is invalid

[Alex Henrie]

On Thu, Nov 8, 2018 at 9:49 AM Alexandre Julliard <julliard at winehq.org> wrote:
>
> Alex Henrie <alexhenrie24 at gmail.com> writes:
>
> > On Wed, Nov 7, 2018 at 11:57 PM Alexandre Julliard <julliard at winehq.org> wrote:
> >>
> >> Alex Henrie <alexhenrie24 at gmail.com> writes:
> >>
> >> > On Wed, Nov 7, 2018 at 9:11 AM Alexandre Julliard <julliard at winehq.org> wrote:
> >> >>
> >> >> Alex Henrie <alexhenrie24 at gmail.com> writes:
> >> >>
> >> >> > Signed-off-by: Alex Henrie <alexhenrie24 at gmail.com>
> >> >> > ---
> >> >> >  dlls/advapi32/registry.c       | 2 ++
> >> >> >  dlls/advapi32/tests/registry.c | 6 ++++++
> >> >> >  2 files changed, 8 insertions(+)
> >> >> >
> >> >> > diff --git a/dlls/advapi32/registry.c b/dlls/advapi32/registry.c
> >> >> > index 0a8ccfbf20..4b33ade88c 100644
> >> >> > --- a/dlls/advapi32/registry.c
> >> >> > +++ b/dlls/advapi32/registry.c
> >> >> > @@ -313,6 +313,8 @@ static inline HKEY get_special_root_hkey( HKEY hkey, REGSAM access )
> >> >> >  {
> >> >> >      HKEY ret = hkey;
> >> >> >
> >> >> > +    if (hkey == INVALID_HANDLE_VALUE) return 0;
> >> >>
> >> >> There shouldn't be any reason to check for INVALID_HANDLE_VALUE, it
> >> >> doesn't have a special meaning. I expect you'll get the same behavior
> >> >> with any other invalid handle.
> >> >
> >> > Is there a straightforward way to check whether the handle is valid?
> >>
> >> You shouldn't need to add explicit checks, it should fall off from the
> >> normal function behavior. That's probably true for your other patch too.
> >
> > Leaving aside the second patch for the moment, the tests show that
> > RegCreateKeyEx[AW] checks the retkey parameter before checking the
> > hkey parameter. If you don't want retkey to be explicitly checked at
> > the beginning of RegCreateKeyEx[AW], how do you want the check to be
> > done?
>
> Is there an app that depends on this? Adding a lot of error checking
> only to satisfy artificial test cases is usually not desirable.

Considering how commonly this function is used and the fact that it
has a Coverity warning, I think it's very likely that at least one
application depends on at least checking the name parameter. The main
reason why I added checks for retkey and hkey too was for
completeness; if for some reason you think that these parameters
should not be checked then I'm happy to submit a patch that does the
minimum required to fix the Coverity warning.

-Alex