Re: [ros-dev] [ros-diffs] [hbelusca] 74621: [USETUP]: SetupLib: Add an ARC path to (and from) NT path resolver. It allows mapping between an ARC path as specified in freeldr.ini / boot.ini , to its corresponding NT path, if...

[Thomas Faber]

On 2017-05-22 03:09, [e-mail] wrote:
> --- branches/setup_improvements/base/setup/lib/arcname.c	(added)
> +++ branches/setup_improvements/base/setup/lib/arcname.c	[iso-8859-1] Mon May 22 01:09:35 2017
> +PCSTR
> +ArcGetNextTokenA(
> IN  PCSTR ArcPath,
> OUT PANSI_STRING TokenSpecifier,
> OUT PULONG Key)
>
> +{
> HRESULT hr;
> PCSTR p = ArcPath;
> ULONG SpecifierLength;
> ULONG KeyValue;
> /*
> * We must have a valid "specifier(key)" string, where 'specifier'
> * cannot be the empty string, and is followed by '('.
> */
> p = strchr(p, '(');
> if (p == NULL)
>    return NULL; /* No '(' found */
> if (p == ArcPath)
>    return NULL; /* Path starts with '(' and is thus invalid */
> SpecifierLength = (p - ArcPath) * sizeof(CHAR);
> /*
> * The strtoul function skips any leading whitespace.
> *
> * Note that if the token is "specifier()" then strtoul won't perform
> * any conversion and return 0, therefore effectively making the token
> * equivalent to "specifier(0)", as it should be.
> */
> // KeyValue = atoi(p);
> KeyValue = strtoul(p, (PSTR*)&p, 10);
> /* Skip any trailing whitespace */
> while (*p && isspace(*p)) ++p;

isspace(0) is false so you don't need the *p check.
> +PCWSTR
> +ArcGetNextTokenU(
> IN  PCWSTR ArcPath,
> OUT PUNICODE_STRING TokenSpecifier,
> OUT PULONG Key)
>
> +{
> HRESULT hr;
> PCWSTR p = ArcPath;
> ULONG SpecifierLength;
> ULONG KeyValue;
> /*
> * We must have a valid "specifier(key)" string, where 'specifier'
> * cannot be the empty string, and is followed by '('.
> */
> p = wcschr(p, L'(');
> if (p == NULL)
>    return NULL; /* No '(' found */
> if (p == ArcPath)
>    return NULL; /* Path starts with '(' and is thus invalid */
> SpecifierLength = (p - ArcPath) * sizeof(WCHAR);
> ++p;
> /*
> * The strtoul function skips any leading whitespace.
> *
> * Note that if the token is "specifier()" then strtoul won't perform
> * any conversion and return 0, therefore effectively making the token
> * equivalent to "specifier(0)", as it should be.
> */
> // KeyValue = _wtoi(p);
> KeyValue = wcstoul(p, (PWSTR*)&p, 10);
> ASSERT(p);

This assert seems superfluous, you just dereferenced the pointer. Also,
you incremented it just above and also (rightly) assumed it was larger
than ArcPath.
> +static NTSTATUS
> +ResolveArcNameManually(
> OUT PUNICODE_STRING NtName,
> IN OUT PCWSTR* ArcNamePath,
> IN  PPARTLIST PartList OPTIONAL)
>
> +{
> [...]
> +Quit:
> if (FAILED(hr))
> {
>    /*
>     * We can directly cast the HRESULTs into NTSTATUS since the error codes
>     * returned by StringCbPrintfW:
>     *    STRSAFE_E_INVALID_PARAMETER   == 0x80070057,
>     *    STRSAFE_E_INSUFFICIENT_BUFFER == 0x8007007a,
>     * do not have assigned values in the NTSTATUS space.
>     */
>    return (NTSTATUS)hr;

To me that sounds like an argument of why you _can't_ do that.
(You know the Rtl versions of these functions give you an NTSTATUS btw,
  right?)
> --- branches/setup_improvements/base/setup/lib/arcname_tests.c	(added)
> +++ branches/setup_improvements/base/setup/lib/arcname_tests.c	[iso-8859-1] Mon May 22 01:09:35 2017
> @@ -0,0 +1,142 @@
> +/*
> Tests for the arcname.c functions:
> ArcPathNormalize(),
> ArcPathToNtPath().
> You should certainly fix the included headers before being able
> to compile this file (as I didn't bother to have it compilable
> under our (P/N)DK, but just under VS).
> */

Wine's test framework works okay for unit tests. You can make this an
apitest by just #include'ing the C source file, then it can run on
Testbot.

[Hermès BÉLUSCA-MAÏTO]

Hi again,
> isspace(0) is false so you don't need the *p check.

Thanks, I was hesitating about this when I wrote this code.
> > KeyValue = wcstoul(p, (PWSTR*)&p, 10);
> > ASSERT(p);
> This assert seems superfluous, you just dereferenced the pointer. Also, you incremented it just above and also (rightly) assumed it was larger than ArcPath.

If I'm 200% sure wcstoul can never ever ever return a NULL pointer via its second argument, then yes I can remove the assert.
> >    /*
> >     * We can directly cast the HRESULTs into NTSTATUS since the error codes
> >     * returned by StringCbPrintfW:
> >     *    STRSAFE_E_INVALID_PARAMETER   == 0x80070057,
> >     *    STRSAFE_E_INSUFFICIENT_BUFFER == 0x8007007a,
> >     * do not have assigned values in the NTSTATUS space.
> >     */
> >    return (NTSTATUS)hr;
> To me that sounds like an argument of why you _can't_ do that.
> (You know the Rtl versions of these functions give you an NTSTATUS btw,  right?)

Oh great! We do have the RtlStringC(b/ch)* functions! But the header: "ddk/ntstrsafe.h" : in the ddk : lol ...
OK I know that for the purposes of giving a DDK to the users, Microsoft put this header in "ddk", but it doesn't really make sense for us to have them there, when you know it can be used also for user-mode applications that just want to rely on NT Rtl* functions...
> Wine's test framework works okay for unit tests. You can make this an apitest by just #include'ing the C source file, then it can run on Testbot.

Noted :)