Re: [ros-dev] [ros-diffs] [hbelusca] 74550: [USETUP]: Continue implementing the NT OS installation detector. What remains to be done here, besides cleaning up the code from temporary comments and DPRINTs (and fixing potenti...

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

Thanks for your quick review, Thomas.

> >   +/***
> >  +*wchar_t *wcsstr(string1, string2) - search for string2 in string1
> >  +*       (wide strings) 
> Copypasta? 
Yes, from the CRT, where I then put in msconfig_new/stringutils.c and then copied it again
there, as an implementation of search substring with case insensitivity.
I'll have to clean this comment block.

> >   +    /*
> > Finally, unmap and close the file */
> >  +    UnMapFile(SectionHandle, ViewBase);
> >  +    NtClose(FileHandle); 
> This seems clumsy. If there's a single function to open and map the file, why
> isn't there one function to unmap and close? 
Possible. I was keeping (for OpenAndMapFile) the possibility to also make it take (from
the "out phandle" parameter) an already-opened file (via handle) so that it
could just map it in memory, and I wasn't decided whether I would merge or not the
unmapfile + ntclose ... I'll think about it.

>   You know there's DPRINT for exactly this purpose,
> right? 
Correct; actually I didn't place them under comments just to shut them up, but
actually because they wouldn't really compile as they are: FileName isn't a
UNICODE_STRING anymore (it was, in a previous code iteration, where these dprints were
enabled, but I then moved the code into this function, before committing, and preferred to
keep the dprints in place, but I didn't fix them, because I don't know if I want
to have FileName back to UNICODE_STRING, or keep it as it's now, i.e. PCWSTR).

>   Casts from PVOID to other pointer types are
> unnecessary. 
I agree.


Cheers,
Hermès


-----Message d'origine-----
De : Ros-dev [mailto:[e-mail]] De la part de Thomas Faber
Envoyé : mardi 16 mai 2017 10:05
À : ros-dev(a)reactos.org
Objet : Re: [ros-dev] [ros-diffs] [hbelusca] 74550: [USETUP]: Continue implementing the NT
OS installation detector. What remains to be done here, besides cleaning up the code from
temporary comments and DPRINTs (and fixing potenti...

Hi again,

some comments inline.


On 2017-05-15 03:59, hbelusca(a)svn.reactos.org wrote:
>   ---
> branches/setup_improvements/base/setup/usetup/osdetect.c	[iso-8859-1] (original)
>  +++ branches/setup_improvements/base/setup/usetup/osdetect.c	[iso-8859-1] Mon May 15
> 01:59:28 2017
>  @@ -19,6 +19,9 @@
>   /* GLOBALS 
>  ******************************************************************/
>
>   extern PPARTLIST PartitionList;
>  +
>  +/* Language-independent Vendor strings */ static const PCWSTR 
>  +KnownVendors[] = { L"ReactOS", L"Microsoft" };
>
>
>   /* VERSION RESOURCE API 
>  ******************************************************/
>  @@ -361,36 +364,331 @@
>
>   #endif
>
>  +
>  +/***
>  +*wchar_t *wcsstr(string1, string2) - search for string2 in string1
>  +*       (wide strings) 
Copypasta?

>   +*
>  +*Purpose:
>  +*       finds the first occurrence of string2 in string1 (wide strings)
>  +*
>  +*Entry:
>  +*       wchar_t *string1 - string to search in
>  +*       wchar_t *string2 - string to search for
>  +*
>  +*Exit:
>  +*       returns a pointer to the first occurrence of string2 in
>  +*       string1, or NULL if string2 does not occur in string1
>  +*
>  +*Uses:
>  +*
>  +*Exceptions:
>  +*
>  +*********************************************************************
>  +**********/ PWSTR FindSubStrI(PCWSTR str, PCWSTR strSearch) {
>  +    PWSTR cp = (PWSTR)str;
>  +    PWSTR s1, s2;
>  +
>  +    if (!*strSearch)
>  +        return (PWSTR)str;
>  +
>  +    while (*cp)
>  +    {
>  +        s1 = cp;
>  +        s2 = (PWSTR)strSearch;
>  +
>  +        while (*s1 && *s2 && (towupper(*s1) == towupper(*s2)))
>  +            ++s1, ++s2;
>  +
>  +        if (!*s2)
>  +            return cp;
>  +
>  +        ++cp;
>  +    }
>  +
>  +    return NULL;
>  +}
>  +
>   static BOOLEAN
>  -IsRecognizedOS(
>  +CheckForValidPEAndVendor(
>  +    IN HANDLE RootDirectory OPTIONAL,
>  +    IN PCWSTR PathName OPTIONAL,
>  +    IN PCWSTR FileName,             // OPTIONAL
>  +    IN PCWSTR VendorName    // Better would be OUT PCWSTR*, and the function returning
> NTSTATUS ?
>  +    )
>  +{
>  +    BOOLEAN Success = FALSE;
>  +    NTSTATUS Status;
>  +    HANDLE FileHandle, SectionHandle;
>  +    // SIZE_T ViewSize;
>  +    PVOID ViewBase;
>  +    PVOID VersionBuffer = NULL; // Read-only
>  +    PVOID pvData = NULL;
>  +    UINT BufLen = 0;
>  +
>  +    Status = OpenAndMapFile(RootDirectory, PathName, FileName,
>  +                            &FileHandle, &SectionHandle, &ViewBase);
>  +    if (!NT_SUCCESS(Status))
>  +    {
>  +        DPRINT1("Failed to open and map file %wZ, Status 0x%08lx\n",
> &FileName, Status);
>  +        return FALSE; // Status;
>  +    }
>  +
>  +    /* Make sure it's a valid PE file */
>  +    if (!RtlImageNtHeader(ViewBase))
>  +    {
>  +        DPRINT1("File %wZ does not seem to be a valid PE, bail out\n",
> &FileName);
>  +        Status = STATUS_INVALID_IMAGE_FORMAT;
>  +        goto UnmapFile;
>  +    }
>  +
>  +    /*
>  +     * Search for a valid executable version and vendor.
>  +     * NOTE: The module is loaded as a data file, it should be marked as such.
>  +     */
>  +    Status = NtGetVersionResource((PVOID)((ULONG_PTR)ViewBase | 1), &VersionBuffer,
> NULL);
>  +    if (!NT_SUCCESS(Status))
>  +    {
>  +        DPRINT1("Failed to get version resource for file %wZ, Status
> 0x%08lx\n", &FileName, Status);
>  +        goto UnmapFile;
>  +    }
>  +
>  +    Status = NtVerQueryValue(VersionBuffer, L"\\VarFileInfo\\Translation",
> &pvData, &BufLen);
>  +    if (NT_SUCCESS(Status))
>  +    {
>  +        USHORT wCodePage = 0, wLangID = 0;
>  +        WCHAR FileInfo[MAX_PATH];
>  +        UNICODE_STRING Vendor;
>  +
>  +        wCodePage = LOWORD(*(ULONG*)pvData);
>  +        wLangID   = HIWORD(*(ULONG*)pvData);
>  +
>  +        StringCchPrintfW(FileInfo, ARRAYSIZE(FileInfo),
>  +                         L"StringFileInfo\\%04X%04X\\CompanyName",
>  +                         wCodePage, wLangID);
>  +
>  +        Status = NtVerQueryValue(VersionBuffer, FileInfo, &pvData, &BufLen);
>  +        if (NT_SUCCESS(Status) && pvData)
>  +        {
>  +            /* BufLen includes the NULL terminator count */
>  +            RtlInitEmptyUnicodeString(&Vendor, pvData, BufLen * sizeof(WCHAR));
>  +            Vendor.Length = Vendor.MaximumLength - 
>  + sizeof(UNICODE_NULL);
>  +
>  +            DPRINT1("Found version vendor: \"%wZ\" for file %wZ\n",
>
>  + &Vendor, &FileName);
>  +
>  +            Success = !!FindSubStrI(pvData, VendorName);
>  +        }
>  +        else
>  +        {
>  +            DPRINT1("No version vendor found for file %wZ\n", &FileName);
>  +        }
>  +    }
>  +
>  +UnmapFile:
>  +    /* Finally, unmap and close the file */
>  +    UnMapFile(SectionHandle, ViewBase);
>  +    NtClose(FileHandle); 
This seems clumsy. If there's a single function to open and map the file, why
isn't there one function to unmap and close?


>   +
>  +    return Success;
>  +}
>  +
>  +static BOOLEAN
>  +IsValidNTOSInstallation(
>  +    IN HANDLE PartitionHandle,
>  +    IN PCWSTR SystemRoot)
>  +{
>  +    BOOLEAN Success = FALSE;
>  +    USHORT i;
>  +    WCHAR PathBuffer[MAX_PATH];
>  +
>  +    // DoesPathExist(PartitionHandle, SystemRoot, L"System32\\"); etc...
>  +
>  +    /* Check for the existence of \SystemRoot\System32 */
>  +    StringCchPrintfW(PathBuffer, ARRAYSIZE(PathBuffer), L"%s%s", SystemRoot,
> L"System32\\");
>  +    if (!DoesPathExist(PartitionHandle, PathBuffer))
>  +    {
>  +        // DPRINT1("Failed to open directory %wZ, Status 0x%08lx\n", 
>  + &FileName, Status); 
You know there's DPRINT for exactly this purpose, right?


>   +        return FALSE;
>  +    }
>  +
>  +    /* Check for the existence of \SystemRoot\System32\drivers */
>  +    StringCchPrintfW(PathBuffer, ARRAYSIZE(PathBuffer), L"%s%s", SystemRoot,
> L"System32\\drivers\\");
>  +    if (!DoesPathExist(PartitionHandle, PathBuffer))
>  +    {
>  +        // DPRINT1("Failed to open directory %wZ, Status 0x%08lx\n",
> &FileName, Status);
>  +        return FALSE;
>  +    }
>  +
>  +    /* Check for the existence of \SystemRoot\System32\config */
>  +    StringCchPrintfW(PathBuffer, ARRAYSIZE(PathBuffer), L"%s%s", SystemRoot,
> L"System32\\config\\");
>  +    if (!DoesPathExist(PartitionHandle, PathBuffer))
>  +    {
>  +        // DPRINT1("Failed to open directory %wZ, Status 0x%08lx\n",
> &FileName, Status);
>  +        return FALSE;
>  +    }
>  +
>  +#if 0
>  +    /*
>  +     * Check for the existence of SYSTEM and SOFTWARE hives in
> \SystemRoot\System32\config
>  +     * (but we don't check here whether they are actually valid).
>  +     */
>  +    if (!DoesFileExist(PartitionHandle, SystemRoot,
> L"System32\\config\\SYSTEM"))
>  +    {
>  +        // DPRINT1("Failed to open file %wZ, Status 0x%08lx\n", &FileName,
> Status);
>  +        return FALSE;
>  +    }
>  +    if (!DoesFileExist(PartitionHandle, SystemRoot,
> L"System32\\config\\SOFTWARE"))
>  +    {
>  +        // DPRINT1("Failed to open file %wZ, Status 0x%08lx\n", &FileName,
> Status);
>  +        return FALSE;
>  +    }
>  +#endif
>  +
>  +    for (i = 0; i < ARRAYSIZE(KnownVendors); ++i)
>  +    {
>  +        /* Check for the existence of \SystemRoot\System32\ntoskrnl.exe and verify its
> version */
>  +        Success = CheckForValidPEAndVendor(PartitionHandle, 
>  + SystemRoot, L"System32\\ntoskrnl.exe", KnownVendors[i]);
>  +
>  +        /* OPTIONAL: Check for the existence of 
>  + \SystemRoot\System32\ntkrnlpa.exe */
>  +
>  +        /* Check for the existence of \SystemRoot\System32\ntdll.dll */
>  +        Success = CheckForValidPEAndVendor(PartitionHandle, 
>  + SystemRoot, L"System32\\ntdll.dll", KnownVendors[i]);
>  +
>  +        /* We have found a correct vendor combination */
>  +        if (Success)
>  +            break;
>  +    }
>  +
>  +    return Success;
>  +}
>  +
>  +typedef struct _NTOS_INSTALLATION
>  +{
>  +    LIST_ENTRY ListEntry;
>  +    ULONG DiskNumber;
>  +    ULONG PartitionNumber;
>  +// Vendor????
>  +    WCHAR SystemRoot[MAX_PATH];
>  +/**/WCHAR InstallationName[MAX_PATH];/**/ } NTOS_INSTALLATION, 
>  +*PNTOS_INSTALLATION;
>  +
>  +static VOID
>  +ListNTOSInstalls(
>  +    IN PGENERIC_LIST List)
>  +{
>  +    PGENERIC_LIST_ENTRY Entry;
>  +    PNTOS_INSTALLATION NtOsInstall;
>  +    ULONG NtOsInstallsCount = GetNumberOfListEntries(List);
>  +
>  +    DPRINT1("There %s %d installation%s detected:\n",
>  +            NtOsInstallsCount >= 2 ? "are" : "is",
>  +            NtOsInstallsCount,
>  +            NtOsInstallsCount >= 2 ? "s" : "");
>  +
>  +    Entry = GetFirstListEntry(List);
>  +    while (Entry)
>  +    {
>  +        NtOsInstall = 
>  + (PNTOS_INSTALLATION)GetListEntryUserData(Entry); 
Casts from PVOID to other pointer types are unnecessary.


Thanks,
Thomas


_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev