I see. Will fix the build issues.

@frankmorgner note that the build fails using gcc 12. So I needed to configure using --disable-strict for the build to pass (probably this is why I did not see the problems you mentioned in my local build).

gcc --version
gcc (Debian 12.2.0-13) 12.2.0

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I../.. -I../../src -I../../src/include -I../../src -Wall -Wextra -Wno-unused-parameter -Werror -Wstrict-aliasing=2 -g -O2 -c sm-eac.c  -fPIC -DPIC -o .libs/libsmeac_la-sm-eac.o
sm-eac.c: In function 'eac_sm_pre_transmit':
sm-eac.c:2228:76: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
 2228 |                                                         (unsigned char **) &eacsmctx->auxiliary_data->data);
      |                                                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Hold on, I may need a few hours to have the build fixed.

@frankmorgner the following change did the trick for me

diff --git a/configure.ac b/configure.ac
index d7d9cb6b..8e9ffd01 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1124,7 +1124,7 @@ if test "${enable_pedantic}" = "yes"; then
        CFLAGS="-pedantic ${CFLAGS}"
 fi
 if test "${enable_strict}" = "yes"; then
-       CFLAGS="-Wall -Wextra -Wno-unused-parameter -Werror -Wstrict-aliasing=2 ${CFLAGS}"
+       CFLAGS="-Wall -Wextra -Wno-unused-parameter -Werror -Wstrict-aliasing ${CFLAGS}"
 fi
 
 AC_CONFIG_FILES([

However, this change has nothing to do with the PR at hand. You should perhaps deal with it elsewhere or correct the type punning in sm-eac.c otherwise. For me this looks like a false positive. At least I have no idea how to handle the char * to unsigned char* "type conversion" differently in this situation (and with this direction - I am pretty sure that the other way around would work).

Strictly speaking, this warning relates to some undefined behavior. Please use this patch:

diff --git a/src/sm/sm-eac.c b/src/sm/sm-eac.c
index 56acdf21..2058e8b6 100644
--- a/src/sm/sm-eac.c
+++ b/src/sm/sm-eac.c
@@ -2223,9 +2223,11 @@ eac_sm_pre_transmit(sc_card_t *card, const struct iso_sm_ctx *ctx,
                                                r = SC_ERROR_OUT_OF_MEMORY;
                                                goto err;
                                        }
+                                       unsigned char *p = (unsigned char *) eacsmctx->auxiliary_data->data;
                                        eacsmctx->auxiliary_data->length = i2d_ASN1_AUXILIARY_DATA(
                                                        msesetat->auxiliary_data,
-                                                       (unsigned char **) &eacsmctx->auxiliary_data->data);
+                                                       &p);
+                                       eacsmctx->auxiliary_data->data = (char *) p;
                                        if ((int) eacsmctx->auxiliary_data->length < 0) {
                                                sc_debug(card->ctx, SC_LOG_DEBUG_VERBOSE, "Error encoding auxiliary data.");
                                                ssl_error(card->ctx);

Note that there should be some more cases of this in sm-eac.c, which you may ignore for now, because they will be fixed with #2674

There is also a memory leak here

There is also a memory leak here

OpenSC/src/libopensc/card-sc-hsm.c

Line 1728 in d21b6ab

keyinfo->gakpresponse = malloc(apdu.resplen);

, see https://github.com/OpenSC/OpenSC/actions/runs/3990200153/jobs/6844166672#step:5:90

Interesting that it occurs here, because the code in question (sc_hsm_generate_keypair) is not modified in this PR.

There is also a memory leak here

OpenSC/src/libopensc/card-sc-hsm.c

Line 1728 in d21b6ab

keyinfo->gakpresponse = malloc(apdu.resplen);

, see https://github.com/OpenSC/OpenSC/actions/runs/3990200153/jobs/6844166672#step:5:90

Sure that you pointed to the correct line in the code?

=================================================================
==30==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 56 byte(s) in 1 object(s) allocated from:
    #0 0x531b4e in __interceptor_calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:77:3
    #1 0x[90](https://github.com/OpenSC/OpenSC/actions/runs/3990200153/jobs/6844166672#step:5:91)b092 in sc_hsm_init /src/opensc/src/libopensc/card-sc-hsm.c:1784:10
    #2 0x57ded1 in sc_connect_card /src/opensc/src/libopensc/card.c:358:8
    #3 0x56f06b in fuzz_connect_card /src/opensc/src/tests/fuzzing/fuzzer_reader.c:205:9
    #4 0x56cacd in LLVMFuzzerTestOneInput /src/opensc/src/tests/fuzzing/fuzz_pkcs15_encode.c:44:6
    #5 0x4407e3 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:611:15
    #6 0x43ffca in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:514:3
    #7 0x441e34 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:826:7
    #8 0x442069 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:857:3
    #9 0x4316cf in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:[91](https://github.com/OpenSC/OpenSC/actions/runs/3990200153/jobs/6844166672#step:5:92)2:6
    #10 0x45ad22 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #11 0x7fe9c1c7b082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51[96](https://github.com/OpenSC/OpenSC/actions/runs/3990200153/jobs/6844166672#step:5:97)9e69ab2d276fae6d1dee)

DEDUP_TOKEN: __interceptor_calloc--sc_hsm_init--sc_connect_card
SUMMARY: AddressSanitizer: 56 byte(s) leaked in 1 allocation(s).

Indeed... this may fix the problem:

diff --git a/src/tests/fuzzing/fuzz_pkcs15_decode.c b/src/tests/fuzzing/fuzz_pkcs15_decode.c
index a83c719cb..e5758ba4d 100644
--- a/src/tests/fuzzing/fuzz_pkcs15_decode.c
+++ b/src/tests/fuzzing/fuzz_pkcs15_decode.c
@@ -108,9 +108,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size)
 
        sc_pkcs15_parse_unusedspace(buf, buf_len, p15card);
 
-       sc_pkcs15_card_free(p15card);
-
 err:
+       sc_pkcs15_card_free(p15card);
        sc_disconnect_card(card);
        sc_release_context(ctx);
        return 0;
diff --git a/src/tests/fuzzing/fuzz_pkcs15_encode.c b/src/tests/fuzzing/fuzz_pkcs15_encode.c
index eb3436dae..a10ecf564 100644
--- a/src/tests/fuzzing/fuzz_pkcs15_encode.c
+++ b/src/tests/fuzzing/fuzz_pkcs15_encode.c
@@ -80,8 +80,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size)
        sc_pkcs15_encode_unusedspace(ctx, p15card, &unused_space, &unused_space_len);
        free(unused_space);
 
-       sc_pkcs15_card_free(p15card);
 err:
+       sc_pkcs15_card_free(p15card);
        sc_disconnect_card(card);
        sc_release_context(ctx);

@frankmorgner rebased, squashed. Hopefully it is now ready for review. Let's see what the CI checks tell us.

appveyor job needs a restart

Build started
git clone -q https://github.com/OpenSC/OpenSC.git C:\projects\opensc
      1 [main] git-remote-https 47[2](https://ci.appveyor.com/project/LudovicRousseau/opensc/builds/46039192/job/m2yxotck7dk340vh#L2) child_info_fork::abort: \??\C:\cygwin\bin\cygcrypto-1.1.dll: Loaded to different address: parent(0x1020000) != child(0xD60000)
error: cannot fork() for fetch-pack: Resource temporarily unavailable
Command exited with code 128

@frankmorgner Hm... Probably this one hits:

It seems that the initialization fails straight away while fuzzing:

The driver allocates some private data

which is never freed due to the direct call to sc_card_free instead of sc_disconnect_card here:

because the internal data is only freed in the operation finish for card-sc-hsm.c.

That is to say, probably card.c might be better off calling sc_disconnect_card (which, in turn calls the finish operation) instead of sc_card_free in

But I cannot tell if this change would have significant negative impact on the other driver implementations.

I simply gave it a try with d868efb

I simply gave it a try with d868efb

In principle, this change is a good idea. However, sc_disconnect_card then requires some more caution. For example, card->reader->ops->disconnect, will always be called regardless of whether the pointers are NULL or not. Also, sc_connect_card can then be simplified to not check whether a reader is connected or not since this is disconnected in sc_disconnect_card anyway.

However, sc_disconnect_card then requires some more caution.

Indeed. Alas, this would also mean refactoring some of the other drivers' finish code, because a call is unsafe in this situation as well. For example

To lock and limit the scope of this PR I will try to find a solution within card-sc-hsm.c only and then open up another issue (and maybe PR) for the proposed refactoring.

To lock and limit the scope of this PR I will try to find a solution within card-sc-hsm.c only and then open up another issue (and maybe PR) for the proposed refactoring.

agreed, thank you

@CardContact Please review this PR.
@frankmorgner Thanks a lot for your intensive support to moving this forward.

mostly nits. Please, squash the fixup and revert commits to have clean commit series before merging.

Ok. Probably we should add a pre-commit linter that enforces the expected code format. Usually, I go for https://pre-commit.com/. Maybe this in combination with https://github.com/pocc/pre-commit-hooks is an option here.

@Jakuje squashed / force pushed again.

Ok. Probably we should add a pre-commit linter that enforces the expected code format. Usually, I go for https://pre-commit.com/. Maybe this in combination with https://github.com/pocc/pre-commit-hooks is an option here.

It is a problem that we are not on the same page what would be the "correct" format and configuration to achieve it, see #2017. I would be all for something like this as soon as we will have the final .clang-format to use.

Rebased to 20899a1, force-pushed.

Thanks!