tests: move GSS-API dynamic stub into debug-mode libcurl #17752
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If we make them rely on debug-builds, we could do the stubbed behavior conditional on an environment variable in classic style which they would avoid relying on ld_preload. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
vszakats
changed the title
vszakats
changed the title
|
Right, so the stub also stubs the |
vszakats
force-pushed
the
t-stubgss-to-data
branch
from
4e1cb03 to
5862f57
Compare
1 task
vszakats
force-pushed
the
t-stubgss-to-data
branch
2 times, most recently
from
c98b48d to
6d0ce4e
Compare
|
There were a couple of issues. Once fixed, the in-lib stubs work
The in-lib stub code is smaller, since we don't have to reimplement Speaking of, I'll add a macro to disable this in case gss internals But, there are memleaks reported by valgrind, which need to be fixed valgrindhttps://github.com/curl/curl/actions/runs/15931031764/job/44940047401?pr=17752#step:39:6487 |
vszakats
force-pushed
the
t-stubgss-to-data
branch
from
6d0ce4e to
3e5e6b5
Compare
…TUB_GSS builds" This reverts commit a680275.
Also enable debug mode in a few CI jobs to enable stub-gss. Drop stub-gss from old-linux to avoid enabling debug mode (no strong reason, but doesn't seem to be worth messing with it).
Detection wasn't accurate for cmake builds at least. It was checking if a shared libcurl was built, but that's just an indirect signal to detect if the curl tool was built against that, or a static libcurl. In cmake, there is a separate user option to decide about this.
For symmetry with Curl_gss_init_sec_context.
vszakats
force-pushed
the
t-stubgss-to-data
branch
from
9b1c3fa to
c7fb9d5
Compare
vszakats
changed the title
vszakats
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace the
libstubgss.so-based overload solution with one built intolibcurl at compile-time.
The previous,
LD_PRELOAD-based, solution was non-portable, allowlistedfor Linux, BSD and Solaris. It also required non-debug builds, which
turned out to be an accidental condition:
7d342c7. It also required a curl tool
built against a shared libcurl. Detecting this condition wasn't always
accurate, e.g. with certain cmake configurations.
The overload solution also didn't work on macOS, though it theoretically
should have:
stubgsslibrary for libtests to match autotools #17653Experiments on making the overload solution work in more envs:
That revealed that it also did not work on NetBSD, in CI.
The replacement solution is overloading the necessary GSS-API functions
for test 2056 and 2057 at compile time. It requires a debug-enabled curl
build (due to its insecure nature).
This makes these tests run on all platforms. Including most GSS jobs in
CI, that are running tests. (the exception is old-linux, non-debug jobs,
where it felt overkill to enable debug for this.)
The refactored GSS stub code needs to overload less than before because
it's free to use the official GSS API. (This didn't work with
the overload solution on Alpine for example). It can also use libcurl
functions, allowing to replace
snprintf()withmsnprintf().OS/400 is also overloading GSS API functions. I haven't tested how this
works after this PR. In theory it should, because this PR doesn't rely
on preprocessor overrides.
Note that for future GSS tests, it may be necessary to stub these GSS
API functions:
gss_inquire_context(),gss_unwrap(),gss_wrap().They are on codepaths not (yet) touched by tests.
Also:
sizeof().They leak the same way as seen with 2077 and 2078.
Ref: 7020ba7 tests: re-enable 1510, unignore 2027 2051 in GHA/macos, document heimdal memleak #17462
Ref: 1467597 cmake: fix
pkg-config-based detection inFindGSS.cmake#14430gss_import_name()leaks in valgrind builds.only.
libstubgss.ld_preloadfeature.LD_PRELOADenv in tests.LD_PRELOADenvs from tests.Follow-up to 56d949d #1687
w/o ws https://github.com/curl/curl/pull/17752/files?w=1
CPPFLAGSwith opt-out, or just tie it toDEBUGBUILD?Yes. The stub code only kicks in if env
CURL_STUB_GSS_CREDSis filled.With curl tests this happens in test 2056, 2057. If they cause a problem,
they can be disabled or ignored via
TFLAGS. This seem sufficient as anescape hatch.
https://github.com/curl/curl/actions/runs/15931031764/job/44940047401?pr=17752#step:39:6488
→ Instead of fixing, append the newly tested tests to the existing ignore list.
(I was staring at
lib/vauth/spnego_sspi.cagain, but can't spot the leaks.)is a platform-agnostic way to do this. Perhaps by moving to unit-tests and
remapping these APIs to the test stubs at libcurlu compile-time? Like
done already for two similar (but smaller) stub libs. Or make them
DEBUGBUILDdependent, an keep tests as-is (except for!Debug→Debug).Ref: 56d949d
This needs using macros to call the overridden APIs from curl.