Skip to content
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

schannel: reclassify extra-verbose schannel_recv messages #14826

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Sep 8, 2024

  • Create a new macro SCH_DEV() to manage verbose debug messages that are only useful for debugging Schannel recv decryption.

schannel_recv contains a lot of useful debug messages to help debug the function, however in practice they are not otherwise useful and showing them in debug builds adds a lot of noise.

To show these messages curl must now be built with CURL_SCHANNEL_DEV_DEBUG defined.

Prior to this change many, but not all, extra-verbose messages were wrapped in DEBUGF() so they were only shown in debug builds.

Ref: #14807

Closes #xxxxx

@jay jay added the Windows Windows-specific label Sep 8, 2024
@github-actions github-actions bot added the CI Continuous Integration label Sep 8, 2024
@dfandrich
Copy link
Contributor

Analysis of PR #14826 at 03d9f837:

Test 1540 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Test 677 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Generated by Testclutch

@jay
Copy link
Member Author

jay commented Sep 8, 2024

Test 1540 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

This test failed in the Omni OS ci job and I don't see anything unexpected in the output except the return code is strange lib1540 returned 2006, when expecting 0. As far as I can see that can't be returned (see lib1540.c + first.c) unless curl_easy_perform returns it so I think more likely there must be something strange happening in runtests

edit: oops I missed in the output that there was a crash so that explains it

2024-09-08T07:00:17.9651993Z  Assertion failed: 0, file ../../lib/multi.c, line 1131, function multi_getsock
2024-09-08T07:00:17.9652853Z  sh: 5582: Abort(coredump)

Test 677 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

another unknown error code lib677.exe returned 143, when expecting 0 despite expected output, this one in CI job msvc, CM x64-windows libressl.

edit: looked at this one again and don't see anything wrong in the output

@jay
Copy link
Member Author

jay commented Sep 10, 2024

@vszakats do you have advice for how I should append a definition to cmake CFLAGS for a CI job, I tried -DCMAKE_C_FLAGS_INIT:STRING=-DCURL_SCHANNEL_DEV_DEBUG but CMAKE_C_FLAGS_INIT was overridden by a CMAKE_C_FLAGS which is used for all jobs.

"-DCMAKE_C_FLAGS=${cflags} -DTESTING_CI_BEHAVIOR" \

So then I tried -DCMAKE_C_FLAGS:STRING=-DCURL_SCHANNEL_DEV_DEBUG but it seems cmake doesn't work that way (it only uses the last CMAKE_C_FLAGS as far as I can tell), which means it will add -DCURL_SCHANNEL_DEV_DEBUG overriding the CMAKE_C_FLAGS used for all jobs (shown above with -DTESTING_CI_BEHAVIOR which is what is overridden in this experiment)

- { build: 'cmake' , sys: 'mingw64', env: 'x86_64' , tflags: 'skiprun' , config: '-DENABLE_DEBUG=ON -DBUILD_SHARED_LIBS=ON -DCURL_USE_SCHANNEL=ON -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_C_FLAGS:STRING=-DCURL_SCHANNEL_DEV_DEBUG -DENABLE_UNICODE=ON -DBUILD_EXAMPLES=OFF', type: 'Debug', name: 'schannel dev debug' }

@vszakats
Copy link
Member

vszakats commented Sep 10, 2024

@vszakats do you have advice for how I should append a definition to cmake CFLAGS for a CI job, I tried -DCMAKE_C_FLAGS_INIT:STRING=-DCURL_SCHANNEL_DEV_DEBUG but CMAKE_C_FLAGS_INIT was overridden by a CMAKE_C_FLAGS which is used for all jobs.

"-DCMAKE_C_FLAGS=${cflags} -DTESTING_CI_BEHAVIOR" \

So then I tried -DCMAKE_C_FLAGS:STRING=-DCURL_SCHANNEL_DEV_DEBUG but it seems cmake doesn't work that way (it only uses the last CMAKE_C_FLAGS as far as I can tell), which means it will add -DCURL_SCHANNEL_DEV_DEBUG overriding the CMAKE_C_FLAGS used for all jobs (shown above with -DTESTING_CI_BEHAVIOR which is what is overridden in this experiment)

- { build: 'cmake' , sys: 'mingw64', env: 'x86_64' , tflags: 'skiprun' , config: '-DENABLE_DEBUG=ON -DBUILD_SHARED_LIBS=ON -DCURL_USE_SCHANNEL=ON -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_C_FLAGS:STRING=-DCURL_SCHANNEL_DEV_DEBUG -DENABLE_UNICODE=ON -DBUILD_EXAMPLES=OFF', type: 'Debug', name: 'schannel dev debug' }

Yes, only the last one counts. Would this help?:

diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml
index 9ce3c7650..500e671f2 100644
--- a/.github/workflows/windows.yml
+++ b/.github/workflows/windows.yml
@@ -171,6 +171,7 @@ jobs:
           fi
           [ '${{ matrix.type }}' = 'Debug' ] && options+=' -DCMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG='
           [ '${{ matrix.type }}' = 'Release' ] && options+=' -DCMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE='
+          cflags+=' -DCURL_SCHANNEL_DEV_DEBUG'
           cmake -B bld ${options} \
             "-DCMAKE_C_FLAGS=${cflags}" \
             "-DCMAKE_RC_COMPILE_OBJECT=${rcopts}" \
@@ -389,6 +390,7 @@ jobs:
         if: ${{ matrix.build == 'cmake' }}
         run: |
           cmake -B bld \
+            -DCMAKE_C_FLAGS=-DCURL_SCHANNEL_DEV_DEBUG \
             -DCMAKE_SYSTEM_NAME=Windows \
             -DCMAKE_C_COMPILER_TARGET=${TRIPLET} \
             -DCMAKE_C_COMPILER=${TRIPLET}-gcc \
@@ -502,6 +504,7 @@ jobs:
             options+=" -DOPENSSL_ROOT_DIR=$VCPKG_INSTALLATION_ROOT/installed/${{ matrix.arch }}-${{ matrix.plat }}"
           fi
           cmake -B bld ${options} \
+            -DCMAKE_C_FLAGS=-DCURL_SCHANNEL_DEV_DEBUG \
             "-DCMAKE_TOOLCHAIN_FILE=$VCPKG_INSTALLATION_ROOT/scripts/buildsystems/vcpkg.cmake" \
             "-DVCPKG_INSTALLED_DIR=$VCPKG_INSTALLATION_ROOT/installed" \
             '-DVCPKG_TARGET_TRIPLET=${{ matrix.arch }}-${{ matrix.plat }}' \

@jay
Copy link
Member Author

jay commented Sep 27, 2024

@vszakats i want the define only for job 'schannel dev debug' so i changed the 'cmake configure' step to add the cflags from that particular job like this

-            "-DCMAKE_C_FLAGS=${cflags}" \
+            "-DCMAKE_C_FLAGS=${{ matrix.cflags }} ${cflags}" \

seems to work ok let me know if you see any problem with it

@vszakats
Copy link
Member

It looks fine to me.

Maybe moving the new job one line up, and moving the ENABLE_UNICODE option before the added cmake option would make the new job line up better with existing ones, but it's just a formatting nit.

@jay jay force-pushed the schannel_trace_data branch 2 times, most recently from 8b972b8 to b502ce7 Compare September 28, 2024 19:07
@bagder
Copy link
Member

bagder commented Oct 4, 2024

@jay where are we on this?

@jay
Copy link
Member Author

jay commented Oct 5, 2024

Some Windows CI failures I can't explain and probably unrelated. I rebased on master and force pushed. Assuming @icing is OK with it then it can replace #14810 and I'll land it

@bagder bagder requested a review from icing October 6, 2024 21:20
@jay
Copy link
Member Author

jay commented Oct 16, 2024

@icing

- Create a new macro SCH_DEV() to manage verbose debug messages that are
  only useful for debugging Schannel recv decryption.

schannel_recv contains a lot of useful debug messages to help debug the
function, however in practice they are not otherwise useful and showing
them in debug builds adds a lot of noise.

To show these messages curl must now be built with
CURL_SCHANNEL_DEV_DEBUG defined.

Prior to this change many, but not all, extra-verbose messages were
wrapped in DEBUGF() so they were only shown in debug builds.

Ref: curl#14807

Closes #xxxxx
Copy link
Contributor

@icing icing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed the mention during covid. Looks good to me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

5 participants