Skip to content
Snippets Groups Projects

modules: use void* with GetProcAddress()

Closed Steve Lhomme requested to merge robUx4/vlc:getproc-far into master
2 unresolved threads

What matters is the type where we write the function pointer. It has to match the Windows SDK signature otherwise the calls will be bogus.

This is how it's used in most places already in C files.

Merge request reports

Merge request pipeline #560089 passed

Merge request pipeline passed for 0dcdf83b

Test coverage 17.73% (0.03%) from 1 job
Test summary results are loading

Closed by Steve LhommeSteve Lhomme 6 days ago (Feb 4, 2025 7:45am UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Steve Lhomme changed milestone to %4.0

    changed milestone to %4.0

    • That looks dubious. POSIX has some history leading to dlsym() returning void* instead of a function pointer, but there's no reason to have void* otherwise. The C standard only guarantee that casting a function pointer to another function pointer type still allows casting it back, as long as the function is not called from the wrong pointer type.

    • Author Developer

      That's the only way to silence warnings with -Wcast-function-type-strict. (or errors with -Werror=cast-function-type-strict).

      As said in the commit, that type doesn't matter. What matters is the type of the variable that will be used as a function pointer.

      And also "This is how it's used in most places already in C files.".

    • IIRC the C standard doesn't guarantee that you can cast a function pointer into a void* it only guarantees you that you can cast a function pointer to another function pointer.

    • Author Developer

      GCC mentions void (*)(void) for function casting but Clang doesn't like it.

      The function type void (*) (void) is special and matches everything, which can be used to suppress this warning. In a cast involving pointer to member types this warning warns whenever the type cast is changing the pointer to member type.

    • Author Developer

      example (clang 18):

      vlc/modules/video_output/win32/wgl.c:98:58: error: cast from 'PROC' (aka 'long long (*)()') to 'void (*)(void)' converts to incompatible function type [-Werror,-Wcast-function-type-strict]
         98 |     PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = (void(*)(void))wglGetProcAddress("wglCreateAffinityDCNV");
            |                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      vlc/modules/video_output/win32/wgl.c:98:34: error: incompatible function pointer types initializing 'PFNWGLCREATEAFFINITYDCNVPROC' (aka 'struct HDC__ *(*)(struct HGPUNV__ *const *)') with an expression of type 'void (*)(void)' [-Wincompatible-function-pointer-types]
         98 |     PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = (void(*)(void))wglGetProcAddress("wglCreateAffinityDCNV");
            |                                  ^                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      2 errors generated.

      with nothing:

      vlc/modules/video_output/win32/wgl.c:98:34: error: incompatible function pointer types initializing 'PFNWGLCREATEAFFINITYDCNVPROC' (aka 'struct HDC__ *(*)(struct HGPUNV__ *const *)') with an expression of type 'PROC' (aka 'long long (*)()') [-Wincompatible-function-pointer-types]
         98 |     PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = wglGetProcAddress("wglCreateAffinityDCNV");
            |                                  ^                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      1 error generated.

      with the actual target type:

      vlc/modules/video_output/win32/wgl.c:98:58: error: cast from 'PROC' (aka 'long long (*)()') to 'PFNWGLCREATEAFFINITYDCNVPROC' (aka 'struct HDC__ *(*)(struct HGPUNV__ *const *)') converts to incompatible function type [-Werror,-Wcast-function-type-strict]
         98 |     PFNWGLCREATEAFFINITYDCNVPROC fncCreateAffinityDCNV = (PFNWGLCREATEAFFINITYDCNVPROC)wglGetProcAddress("wglCreateAffinityDCNV");
            |                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      1 error generated.

      with (void *) there no error or warning.

    • https://www.frama-c.com/2013/08/24/Function-pointers-in-C.html

      Then you might want to avoid using Wcast-function-type-strict and report a bug to clang? Casting to void* effectively removes the warning, but I very much prefer a warning over a potential UB effect that could break tooling and architectures.

    • Author Developer

      From C17 N2176 6.3.2.3 (Pointers) 8:

      A pointer to a function of one type may be converted to a pointer to a function of another type and back again; the result shall compare equal to the original pointer. If a converted pointer is used to call a function whose type is not compatible with the referenced type, the behavior is undefined

      GetProcAddress() returns a FARPROC which has various function signature based on target Windows version. This means that casting that function pointer to any other signature is Undefined Behavior and Windows programs work by chance.

      From C17 N2176 J.5.7 (Function pointer casts):

      A pointer to an object or to void may be cast to a pointer to a function, allowing data to be invoked as a function (6.5.4).

      A pointer to a function may be cast to a pointer to an object or to void, allowing a function to be inspected or modified (for example, by a debugger) (6.5.4)

      This is a C17 extension that seems to be in GCC and Clang (and MSVC) which both don't complain when using (void*) to transform back and forth from function pointers (and complain with anything else). We should require that extension for Windows target if we don't want to leave GetProcAddress() using an Undefined Behavior.

    • Author Developer

      This becomes trickier with C++ where no such extension exists

      Any pointer to function can be converted to a pointer to a different function type. The result is unspecified, but converting such pointer back to pointer to the original function type yields the pointer to the original function. The resulting pointer can only be called safely if it function type is call-compatible with the original function type.

      And in fact, I have not found a way to convert the FARPROC to anything else without Clang complaining (this MR only contains changes for the C files). In such case we need to explicitly disable the warning (while allowing it for the rest of the code we want to check).

    • GetProcAddress() returns a FARPROC which has various function signature based on target Windows version. This means that casting that function pointer to any other signature is Undefined Behavior and Windows programs work by chance.

      I don't see where the UB is. FARPROC is not the original signature.

    • Author Developer

      I don't see where the UB is. FARPROC is not the original signature.

      It is the only thing the compiler knows from the Windows SDK/mingw-w64. It can't guess the signature (that would be great).

    • Why would it need to guess the signature? The only requirements are that the caller of the function has used the correct signature.

    • Author Developer

      The only requirements are that the caller of the function has used the correct signature.

      Which the compiler doesn't know. It cannot check you're doing it properly or not. You're allowed to do a conversion to another type and back. But if it doesn't know what is the correct "back" it can't verify the code is legit.

    • The compiler doesn't care.

      A pointer to a function of one type may be converted to a pointer to a function of another type and back again; the result shall compare equal to the original pointer

      == function pointer cast to another function pointer is not UB

      If a converted pointer is used to call a function whose type is not compatible with the pointed-to type, the behavior is undefined

      == what matters to qualify the dereferencing call as UB is the pointee, not where the pointer is coming from.

    • Author Developer

      The compiler doesn't care.

      It clearly does. And this is not a bug in Clang. The same thing happens with gcc:

      modules/hw/amf/amf_helper.c:26:39: warning: cast between incompatible function types from ‘FARPROC’ {aka ‘long long int (*)()’} to ‘AMF_RESULT (*)(amf_uint64 *)’ {aka ‘AMF_RESULT (*)(long long unsigned int *)’} [-Wcast-function-type]
         26 |     AMFQueryVersion_Fn queryVersion = (AMFQueryVersion_Fn)GetProcAddress(hLib, AMF_QUERY_VERSION_FUNCTION_NAME);
            |                                       ^

      The only thing the compiler knows is the FARPROC.

    • You're talking about a warning from an optional behavior of the compiler that is, in general, not suitable for dynamic loading of symbol, and confusing it with an undefined behavior.

    • Author Developer

      I do understand perfectly that from the point of view of the compiler the casting from a function pointer type to another function pointer type is an undefined behavior. And then

      If a converted pointer is used to call a function whose type is not compatible with the pointed-to type, the behavior is undefined

      And since

      not suitable for dynamic loading of symbol

      We have to explicitly tell the compiler that this is actually OK.

    • I do understand perfectly that from the point of view of the compiler the casting from a function pointer type to another function pointer type is an undefined behavior.

      The first sentence you quoted from the standard tells the exact opposite, as well as the links I've sent and what Denis said.

    • Author Developer

      A pointer to a function of one type (pt1 of type FARPROC) may be converted to a pointer to a function of another type (pt of type AMFQueryVersion_Fn) and back again (pt2 of type FARPROC); the result (pt1 of type FARPROC) shall compare equal to the original pointer (pt2 of type FARPROC).

      This is how the compiler interprets this sentence (pt1 = pt2).

      Edited by Steve Lhomme
    • It just says that:

      • 1/ you're allowed to cast
      • 2/ if you cast pt1 to AMFQueryVersion_Fn and cast back to FARPROC, the pointer value doesn't change

      I really don't see where you read undefined behaviour in this sentence...

    • The argument here is that if the function is called with the correct arguments, you can cast it to anything you want and re-cast it back. Which is what AMF expects its users to do.

      The clang warning is unsuited for being enabled on the whole codebase for any platform for this specific reason, it should be used to find abnormal casts but does not strictly point at UB. The function calls can be UB if the caller interprets the doc incorrectly.

      We have to explicitly tell the compiler that this is actually OK.

      I suggest disabling the warning for files interacting with these kinds of APIs.

      Edited by Alaric Senat
    • Author Developer

      I suggest disabling the warning for files interacting with these kinds of APIs.

      I agree. In part because there's no cast that can help in C++. And I started working on that. In the end the void* cast should not be needed for C files.

      Function casting to a different signature is one of the nastiest thing there can be in a code after a NULL deference. We should have strong checks against this wherever possible, even in Windows only code.

    • Please register or sign in to reply
  • mentioned in commit robUx4/vlc@81be24da

    • Casting to void * makes sense in generic wrappers that can return any function address. Though perhaps void (*)() would be best, it risks triggering incomplete prototype warnings.

      But I kinda agree with Alex here. It's just silly if we know what the correct fucntion type is at a given site.

    • Author Developer

      IMO we should require the J.5.7 extension. Otherwise dynamic modules don't even work (they use void* for the module callbacks).

    • Please register or sign in to reply
  • mentioned in commit robUx4/vlc@506bf92b

  • mentioned in commit robUx4/vlc@381175ef

  • closed

  • mentioned in commit robUx4/vlc@77f7adb9

Please register or sign in to reply
Loading