New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom error message when incorrect-architecture OCI.DLL is in PATH on Windows #49

Closed
kubo opened this Issue Nov 26, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@kubo
Copy link

kubo commented Nov 26, 2017

When incorrect-architecture OCI.DLL is in the PATH and correct-architecture OCI.DLL isn't there, GetLastError() returns ERROR_BAD_EXE_FORMAT(%1 is not a valid Win32 application). How about custom error message saying "c:\full\path\of\OCI.DLL is 32-bit, which isn't available in x64 programs" or so in this case?
This will reduce issues such as oracle/node-oracledb#752.

  1. Search OCI.DLL as the step 1 in #48 (comment).
  2. If all OCI.DLLs are incorrect-architecture, create a custom error message. Otherwise use message from FormatMessage().

Architecture could be checked by nt_hdr->FileHeader.Machine in #48 (comment) or by the following function.

#include <windows.h>

static void print_arch(const char *name)
{
    FILE *fp = fopen(name, "rb");
    IMAGE_DOS_HEADER dos_hdr;
    IMAGE_NT_HEADERS nt_hdr;

    if (fp == NULL) {
        printf("failed to open: %s\n", name);
        return;
    }
    fread(&dos_hdr, sizeof(dos_hdr), 1, fp);
    if (dos_hdr.e_magic != IMAGE_DOS_SIGNATURE) {
        printf("invalid DOS signature: 0x%x\n", dos_hdr.e_magic);
        fclose(fp);
        return;
    }
    fseek(fp, dos_hdr.e_lfanew, SEEK_SET);
    fread(&nt_hdr, sizeof(nt_hdr), 1, fp);
    fclose(fp);
    if (nt_hdr.Signature != IMAGE_NT_SIGNATURE) {
        printf("invalid NT signature: 0x%x\n", nt_hdr.Signature);
        return;
    }
    switch (nt_hdr.FileHeader.Machine) {
    case IMAGE_FILE_MACHINE_I386:
        printf("architecture is x86.\n");
        break;
    case IMAGE_FILE_MACHINE_AMD64:
        printf("architecture is x64.\n");
        break;
    case IMAGE_FILE_MACHINE_IA64:
        printf("architecture is IA64.\n");
        break;
    default:
        printf("invalid architecture: 0x%x\n", nt_hdr.FileHeader.Machine);
        break;
    }
}

@cjbj cjbj added the enhancement label Nov 26, 2017

anthony-tuininga added a commit that referenced this issue Nov 28, 2017

On Windows, attempt to locate the OCI.dll that failed to load due to …
…an invalid

architecture; if found adjust the message that is returned to include that
location; otherwise, use the default message, as suggested
(#49).
@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Nov 28, 2017

I just pushed a commit to address this. Please take a look and let me know if that addresses your request. Thanks!

@kubo

This comment has been minimized.

Copy link
Author

kubo commented Dec 2, 2017

Thank you! It works fine. But could you fix following?
Check the length of destination of strcpy and strcat to prevent buffer overrun.
When length > _MAX_DIR, it loops infinitely.
Use snprintf instead of sprintf to prevent buffer overrun.

As for this code I prefer predefined macros as follows to checking sizeof(void*).
But it is just my preference. There is no need to fix it.

#if defined _M_AMD64
    if (ntHeaders.FileHeader.Machine == IMAGE_FILE_MACHINE_AMD64)
        return 1;
#elif defined _M_IX86
    if (ntHeaders.FileHeader.Machine == IMAGE_FILE_MACHINE_I386)
        return 1;
#else
#error unsupported architecture
#endif
@anthony-tuininga

This comment has been minimized.

Copy link
Member

anthony-tuininga commented Dec 5, 2017

I just pushed changes based on your suggestions. Thanks. Some of those should be "impossible", but the Windows documentation doesn't state it is, so better safe than sorry!

@kubo

This comment has been minimized.

Copy link
Author

kubo commented Dec 5, 2017

Thank you! It satisfies my request.

@kubo kubo closed this Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment