Skip to content

Fix SyncRepl parser hang on backslashes and tls_cacertdir parsing - #117

Open
opravil-jan wants to merge 3 commits into
apache:masterfrom
opravil-jan:fix/syncrepl-parser
Open

opravil-jan wants to merge 3 commits into
apache:masterfrom
opravil-jan:fix/syncrepl-parser

Conversation

@opravil-jan

@opravil-jan opravil-jan commented Sep 29, 2026 •

Copy link
Copy Markdown

Depends on #116. This branch is built on #116 so the new unit tests actually run in CI. Until #116 is merged, its commit (build(test): run the plugins' JUnit 5 unit tests) also shows up here. Only the two fix(syncrepl) commits belong to this PR.

This PR fixes two bugs in openldap.syncrepl. Each fix is its own commit and comes with a regression test.

1. Parser hangs on a backslash in a quoted value

getQuotedOrNotQuotedOptionValue only consumed a backslash when it escaped the closing quote. Any other backslash never moved the position forward, so it looped forever. Values that trigger this are common, for example a filter like filter="(cn=a\2ab)", a bind DN like binddn="cn=Smith\, John,dc=example,dc=com", or tls_cacert="C:\certs\ca.pem". The parser is called from DatabasesDetailsPage.getReplicationConsumerText, which is a label provider, so opening an OpenLDAP configuration with such an olcSyncrepl value froze Studio.

The fix keeps such a backslash as part of the value. That matches slapd: values in cn=config / slapd.d are split by strtok_quote_ldif() in servers/slapd/config.c, which passes backslashes through unchanged. (This is not strtok_quote(), the slapd.conf tokenizer, which does treat \ as an escape.) I checked this by compiling slapd's tokenizer functions verbatim and running them on these inputs.

2. tls_cacertdir could not be parsed

tls_cacert was checked first and is a prefix of tls_cacertdir, so the option failed to parse and the whole syncrepl value was rejected. The longer keyword is now checked first. I checked all 35 keywords and found no other prefix conflict.

Tests

3 new tests in SyncReplParserTest. Each one failed before its fix: the two hang tests timed out and the tls_cacertdir test threw a parse exception. The hang tests use @Timeout(threadMode = SEPARATE_THREAD) because the loop never checks for interruption. The round-trip test also checks that toString() writes filter="(cn=a\2ab)" and the bind DN unchanged. openldap.syncrepl: 80 tests; openldap.config.editor, which uses SyncRepl: 34 tests; all pass.

Update

An earlier revision of this PR assumed slapd.conf tokenization. It dropped backslashes and escaped them on write, which would have changed filters and DNs saved to the server. That revision has been replaced. The remaining differences from slapd, all pre-existing, are left for a separate PR:

  • \" is still treated as an escaped quote.
  • '...' is still accepted as quoting.
  • credentials and the tls_* paths are written unquoted, while slapd's own syncrepl_unparse quotes them.

🤖 Generated with Claude Code

junit-platform-runner pulled JUnit 4 onto the test classpath, so
surefire auto-detected the JUnit4 provider and discovered none of the
Jupiter tests: every plugin module reported "Tests run: 0" and the
build stayed green. Nothing uses the runner, so drop it together with
the now unused junit.platform.version property.

Hamcrest was only reachable transitively through JUnit 4; its single
user, EntryEditorWidgetSorterTest, now asserts with JUnit's
assertEquals like the other test classes.

Surefire now runs 360 unit tests across 10 plugins (2 skipped).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@opravil-jan
opravil-jan marked this pull request as draft September 29, 2026 08:59
@opravil-jan

Copy link
Copy Markdown
Author

Converting to draft: commits 1 and 3 are based on the wrong tokenizer. They follow strtok_quote() (slapd.conf), where a backslash escapes the next character. But olcSyncrepl in cn=config / slapd.d is tokenized by strtok_quote_ldif(), which passes backslashes (and all other characters) through unchanged. As it stands, commit 3 would save (cn=a\2ab) as (cn=a\\2ab), which is a regression.

I'll rework it so backslashes are kept verbatim when parsing and are not escaped when writing, then mark the PR ready again. The tls_cacertdir fix (commit 2) is unaffected.

opravil-jan and others added 2 commits September 29, 2026 11:02
A backslash inside a quoted option value was only consumed when it
escaped the closing quote; any other backslash never advanced the
position, so e.g. tls_cacert="C:\certs\ca.pem" or a filter such as
(cn=a\2ab) looped forever. The parser runs from the OpenLDAP config
editor's label provider, so opening a database with such an
olcSyncrepl value froze Studio.

Keep such a backslash as part of the value: cn=config values are split
by slapd's strtok_quote_ldif(), which passes backslashes through
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tls_cacert was matched first and is a prefix of tls_cacertdir, so the
option failed to parse and the whole syncrepl value was rejected.
Check the longer keyword first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@opravil-jan opravil-jan changed the title Fix SyncRepl parser hang on backslashes, tls_cacertdir parsing and value escaping Fix SyncRepl parser hang on backslashes and tls_cacertdir parsing Sep 29, 2026
@opravil-jan
opravil-jan marked this pull request as ready for review September 29, 2026 09:52
@opravil-jan

Copy link
Copy Markdown
Author

Reworked and force-pushed. Backslashes are now kept verbatim, as strtok_quote_ldif() does, and the value-escaping commit is dropped. The PR now contains just the hang fix and the tls_cacertdir fix. Aligning quoting fully with slapd will be a separate PR. Ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant