Fix 64 to 32 conversions

[Jakuje]

This is an attempt to fix #2967 while trying locally with my clang:

CC=clang CFLAGS=-Wshorten-64-to-32 ./configure && make -j8

I tried to rewrite some variables to use more sensible types in the context. Some of them were obvious overlooks, sometimes we get size_t from openssl, but are expected to pass int back to it in some case, where we can not do much other than casting that.

I tried to check also some overflows of the casts in some places where it made sense, but certainly the changeset is too large to do it in all places properly.

The PR also includes strict clang run with many more warnings turned to errors (but I had to disable some others).

Checklist

• PKCS#11 module is tested
• Windows minidriver is tested
• macOS tokend is tested

[frankmorgner]

At least, the macOS build issue is fixed. A build on top of your changes is available here https://github.com/OpenSC/OpenSC/actions/runs/7653681110

[Jakuje]

At least, the macOS build issue is fixed. A build on top of your changes is available here https://github.com/OpenSC/OpenSC/actions/runs/7653681110

Thanks for checking and good to hear that! I have still some changes trying to squash some of the clang-format warnings, but it can be skipped to avoid even more changes. But I would like to get at least the ones on the directly touched lines (and one outstanding comment that I missed).

[frankmorgner]

Yeah, I'm currently reviewing the changes - please don't add more on top for now.

[frankmorgner]

It is unfortunate to see so many return values being casted, because we are mixing the returned length with a status code. In an ideal world, we would maybe try to refactor the functions to only return the error code and modify the signature to add a return value... 🤷​

[Jakuje]

It is unfortunate to see so many return values being casted, because we are mixing the returned length with a status code. In an ideal world, we would maybe try to refactor the functions to only return the error code and modify the signature to add a return value... 🤷​

Correct. But it would require even more changes that I am not sure if they are worth the time spent.

Most of them are casts from the some APDU resplen, which are size_t, but really can not go over the SC_MAX_EXT_APDU_RESP_SIZE (0xFFFF+1) if I see right, so I did not even bother with the bounds checking.

[frankmorgner]

Sorry, I know that this is a lot of refactoring work. I didn't mean to offload this to you. regarding the return values, that may stay as you suggested - same for sc_read/write/update/erase_binary/record. Thanks for taking the time fixing all those warnings.

[frankmorgner]

Do you think it is possible to include the newest releases of gcc/clang in the strict workflow somehow?

[Jakuje]

Do you think it is possible to include the newest releases of gcc/clang in the strict workflow somehow?

In Fedora the latest clang/gcc lands in rawhide (unstable rolling release). The first gcc 14 landed there 12 days ago so there is really not much head-time before the package starts failing and somebody will try to address the newly turned on warning (in this case in #2996). The Fedora change process of these core components usually involves rebuilt of all packages and addressing the more obvious issues or opening bugs:

https://koji.fedoraproject.org/koji/buildinfo?buildID=2344783

There should be fedora:rawhide container image, where we could try to build opensc to get results with the latest compilers. Similarly we could try with debian:sid (unstable debian rolling release), but that one at this moment does not have the gcc14 if I see right:

https://packages.debian.org/sid/gcc

The problem with the rolling/unstable releases is that they are updated at unexpected times and CI starts failing in the worst possible times. But if we keep it in separate workflow file we can treat it as "nice to have". Let me give it a try.

I would probably be for merging the #2885 first. It will shrink this PR a bit if you agree.

[Jakuje]

Rebased on top of the removed drivers with an attempt to run CI against unstable versions of both GCC/clang.

The flags might need some tweaking or we might try to fix some of the whitelisted in there gradually.

[Jakuje]

I think the current version is good enough. One of the strict builds is failing because of clang + strict warnings but I do not see the obvious issue in here:

In file included from iasecc-sm.c:29:
../../src/common/compat_strlcpy.h:43:8: error: conflicting types for 'strlcpy'
   43 | size_t strlcpy(char *dst, const char *src, size_t siz);
      |        ^
/usr/include/string.h:506:15: note: previous declaration is here
  506 | extern size_t strlcpy (char *__restrict __dest,
      |               ^
1 error generated.

Additionally, the update of yubico-piv tool likely broke the PIV tests now (sigh ...). Filled this issue Yubico/yubico-piv-tool#465

[frankmorgner]

I think it is likely the use of the restrict keyword. However, compat_strlcpy.h is included although string.h already has some definition. I think, we stumbled over this (outdated?) problem:

OpenSC/configure.ac

Lines 466 to 476 in 6e2ea36

	# Do not check for strlcpy and strlcat in Linux because it is not implemented
	# and autotools can not detect it in AC_CHECK_DECLS because build does not fail
	# in this test.
	# https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22192
	case "${host_os}" in
	linux*)
	;;
	*)
	AC_CHECK_DECLS([strlcpy, strlcat], [], [], [[#include <string.h>]])
	;;
	esac

[Jakuje]

I think I saw similar issue in some other files that did not include the config.h, but in this case, it did not help:

30228d2

[frankmorgner]

Did you notice that detection of the builtins on linux was excluded? Maybe it is time to enable this now...

In an other PR, we had a similar discussion https://github.com/OpenSC/OpenSC/pull/2904/files#r1367731646. in #2904 the fix was to simply rename the internal function.

[Jakuje]

Did you notice that detection of the builtins on linux was excluded? Maybe it is time to enable this now...

Yes, I saw that but did not dig much deeper as I am not well versed in the linux/others gcc/clang differences.

In an other PR, we had a similar discussion https://github.com/OpenSC/OpenSC/pull/2904/files#r1367731646. in #2904 the fix was to simply rename the internal function.

That could do that. But I would propose in another PR if possible

[Jakuje]

Thanks for review. I will rebase + squash the fixup commits and then we can merge if there will be no other voices against.

[frankmorgner]

great, thank you