From babb473682210900578678a3014f2f0301b373e8 Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Thu, 27 Aug 2026 11:05:06 +0200 Subject: [PATCH 1/2] fix(developer): handle leading whitespace correctly in `u16tok()` A string with leading delimiters passed into `u16tok()` would not skip those delimiters. This impacted `&targets` store and other locations. Note that this tightens input parameter checks for delimiters and context as well. Fixes: #13721 Test-bot: skip --- common/cpp/km_u16.cpp | 24 +++++++++- developer/src/kmc-kmn/test/compiler.tests.ts | 36 ++++++++++++-- .../keyboards/targets-with-whitespace.kmn | 11 +++++ developer/src/kmc-kmn/test/helpers/index.ts | 21 ++++++-- developer/src/kmcmplib/src/Compiler.cpp | 31 ++++++++++-- developer/src/kmcmplib/tests/api.tests.cpp | 48 ++++++++++++++----- .../src/kmcmplib/tests/gtest-km_u16.tests.cpp | 34 +++++++++++++ 7 files changed, 180 insertions(+), 25 deletions(-) create mode 100644 developer/src/kmc-kmn/test/fixtures/keyboards/targets-with-whitespace.kmn diff --git a/common/cpp/km_u16.cpp b/common/cpp/km_u16.cpp index 78077202fc4..c3789461566 100644 --- a/common/cpp/km_u16.cpp +++ b/common/cpp/km_u16.cpp @@ -326,10 +326,20 @@ int u16ncmp(const KMX_WCHAR* p, const KMX_WCHAR* q, size_t count) { * @return Pointer to the first token in p */ KMX_WCHAR* u16tok(KMX_WCHAR* p, const KMX_WCHAR ch, KMX_WCHAR** ctx) { + if(ch == 0 || !ctx) { + return NULL; + } + if (!p) { p = *ctx; - if (!p) + if (!p) { return NULL; + } + } + + // skip initial delimiter + while (*p == ch) { + p++; } KMX_WCHAR* q = p; @@ -356,10 +366,20 @@ KMX_WCHAR* u16tok(KMX_WCHAR* p, const KMX_WCHAR ch, KMX_WCHAR** ctx) { * @return Pointer to the first token in p */ KMX_WCHAR* u16tok(KMX_WCHAR* p, const KMX_WCHAR* delimiters, KMX_WCHAR** ctx) { + if(!ctx || !delimiters || !(*delimiters)) { + return NULL; + } + if (!p) { p = *ctx; - if (!p) + if (!p) { return NULL; + } + } + + // skip initial delimiters + while (*p && u16chr(delimiters, *p)) { + p++; } KMX_WCHAR* q = p; diff --git a/developer/src/kmc-kmn/test/compiler.tests.ts b/developer/src/kmc-kmn/test/compiler.tests.ts index db5ea60b064..c4bc0209298 100644 --- a/developer/src/kmc-kmn/test/compiler.tests.ts +++ b/developer/src/kmc-kmn/test/compiler.tests.ts @@ -1,10 +1,15 @@ +/* + * Keyman is copyright (C) SIL Global. MIT License. + */ +import { dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import fs from 'node:fs'; import 'mocha'; import { assert } from 'chai'; -import { KmnCompiler } from '../src/main.js'; -import { dirname } from 'path'; -import { fileURLToPath } from 'url'; -import fs from 'fs'; import { TestCompilerCallbacks } from '@keymanapp/developer-test-helpers'; +import { KmxFileReader } from '@keymanapp/common-types'; +import { compileTestKeyboard } from './helpers/index.js'; +import { KmnCompiler } from '../src/main.js'; const __dirname = dirname(fileURLToPath(import.meta.url)).replace(/\\/g, '/'); const keyboardsDir = __dirname + '/../../../../../common/test/keyboards/'; @@ -13,6 +18,16 @@ const baselineDir = keyboardsDir + 'baseline/'; describe('Compiler class', function() { const callbacks = new TestCompilerCallbacks(this); + this.beforeEach(function() { + callbacks.clear(); + }); + + this.afterEach(function() { + if(this.currentTest?.isFailed()) { + callbacks.printMessages(); + } + }); + it('should throw on failure', async function() { const compiler = new KmnCompiler(); const callbacks : any = null; // ERROR @@ -129,4 +144,17 @@ describe('Compiler class', function() { assert.deepEqual(kvkData, kvkFixtureData); }); + it('should trim all whitespace for `&targets` store', async function() { + const result = await compileTestKeyboard(callbacks, ['keyboards', 'targets-with-whitespace.kmn']); + assert.isNotNull(result); + + // Verifies that targets has 'any' as it contains both JS and KMX targets + assert.isNotNull(result.artifacts.js); + assert.isNotNull(result.artifacts.kmx); + + const reader = new KmxFileReader(); + const keyboard = reader.read(result.artifacts.kmx.data); + assert.equal(keyboard.targets, 'any'); + }); + }); diff --git a/developer/src/kmc-kmn/test/fixtures/keyboards/targets-with-whitespace.kmn b/developer/src/kmc-kmn/test/fixtures/keyboards/targets-with-whitespace.kmn new file mode 100644 index 00000000000..4afe77ecd59 --- /dev/null +++ b/developer/src/kmc-kmn/test/fixtures/keyboards/targets-with-whitespace.kmn @@ -0,0 +1,11 @@ +store(&NAME) 'targets_with_whitespace' +store(&VERSION) '10.0' + +c this used to generate KM0207B: At least one compile target must be specified +store(&TARGETS) ' any' + +begin unicode > use(main) + +group(main) using keys + ++ 'x' > 'y' diff --git a/developer/src/kmc-kmn/test/helpers/index.ts b/developer/src/kmc-kmn/test/helpers/index.ts index 8250dfd101a..ae0ad212687 100644 --- a/developer/src/kmc-kmn/test/helpers/index.ts +++ b/developer/src/kmc-kmn/test/helpers/index.ts @@ -1,8 +1,13 @@ -/** +/* + * Keyman is copyright (C) SIL Global. MIT License. + * * Helpers and utilities for the Mocha tests. */ -import * as path from 'path'; -import { fileURLToPath } from 'url'; +import * as path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { assert } from 'chai'; +import { CompilerCallbacks } from '@keymanapp/developer-utils'; +import { KmnCompiler } from '../../src/compiler/compiler.js'; /** * Builds a path to the fixture with the given path components. @@ -15,3 +20,13 @@ import { fileURLToPath } from 'url'; export function makePathToFixture(...components: string[]): string { return fileURLToPath(new URL(path.join('..', '..', '..', 'test', 'fixtures', ...components), import.meta.url)); } + +export async function compileTestKeyboard(callbacks: CompilerCallbacks, fixture: string[]) { + const compiler = new KmnCompiler(); + assert(await compiler.init(callbacks, {saveDebug: true, shouldAddCompilerVersion: false})); + assert(compiler.verifyInitialized()); + + const kmnPath = makePathToFixture(...fixture); + + return await compiler.run(kmnPath, null); +} \ No newline at end of file diff --git a/developer/src/kmcmplib/src/Compiler.cpp b/developer/src/kmcmplib/src/Compiler.cpp index d9f3c5c9cb0..e73b2b9a452 100644 --- a/developer/src/kmcmplib/src/Compiler.cpp +++ b/developer/src/kmcmplib/src/Compiler.cpp @@ -138,7 +138,7 @@ KMX_BOOL IsSameToken(PKMX_WCHAR *p, KMX_WCHAR const * token); KMX_DWORD GetRHS(PFILE_KEYBOARD fk, PKMX_WCHAR p, PKMX_WCHAR buf, int bufsize, int offset, int IsUnicode); PKMX_WCHAR GetDelimitedString(PKMX_WCHAR *p, KMX_WCHAR const * Delimiters, KMX_WORD Flags); KMX_DWORD GetXString(PFILE_KEYBOARD fk, PKMX_WCHAR str, KMX_WCHAR const * token, PKMX_WCHAR output, int max, int offset, PKMX_WCHAR *newp, int isVKey, int isUnicode); -KMX_BOOL GetCompileTargetsFromTargetsStore(const KMX_WCHAR* store, int &targets); +KMX_BOOL GetCompileTargetsFromTargetsStore(KMX_WCHAR *store, int &targets); int GetGroupNum(PFILE_KEYBOARD fk, PKMX_WCHAR p); @@ -1331,7 +1331,16 @@ KMX_BOOL ProcessSystemStore(PFILE_KEYBOARD fk, KMX_DWORD SystemID, PFILE_STORE s return TRUE; } -KMX_BOOL GetCompileTargetsFromTargetsStore(const KMX_WCHAR* store, int &targets) { +/** + * Extract the compile targets from the &targets store, and rewrite the + * &targets dpString value to remove unnecessary whitespace. Does not + * reallocate sp->dpString, but overwrites its value with a string the + * same length or shorter. + * @param sp store value to rewrite + * @param targets (output) + * @return FALSE if no targets found or invalid targets found + */ +KMX_BOOL GetCompileTargetsFromTargetsStore(KMX_WCHAR *store, int &targets) { // Compile to .kmx const std::vector KMXKeymanTargets{ u"windows", u"macosx", u"linux", u"desktop" @@ -1347,10 +1356,17 @@ KMX_BOOL GetCompileTargetsFromTargetsStore(const KMX_WCHAR* store, int &targets) targets = 0; auto p = new KMX_WCHAR[u16len(store)+1]; - u16cpy(p, store); + auto q = p; + *p = 0; KMX_WCHAR* ctx; - auto token = u16tok(p, u" ", &ctx); + auto token = u16tok(store, u" ", &ctx); while(token) { + if(q > p) { + // Insert a delimiter between tokens, if + // more than one token + *q++ = ' '; + *q = 0; + } bool found = false; if(*token) { if(AnyTarget == token) { @@ -1378,9 +1394,16 @@ KMX_BOOL GetCompileTargetsFromTargetsStore(const KMX_WCHAR* store, int &targets) targets = 0; return FALSE; } + + // Append the token to the output, we know it is long enough + // because the buffer is the same length as the input + u16cpy(q, token); + q = const_cast(u16chr(q, 0)); + *q = 0; } token = u16tok(nullptr, u" ", &ctx); } + u16cpy(store, p); delete[] p; if(targets == 0) { diff --git a/developer/src/kmcmplib/tests/api.tests.cpp b/developer/src/kmcmplib/tests/api.tests.cpp index ea44e3fade0..445c69928bf 100644 --- a/developer/src/kmcmplib/tests/api.tests.cpp +++ b/developer/src/kmcmplib/tests/api.tests.cpp @@ -71,66 +71,90 @@ void test_kmcmp_CompileKeyboard(char *kmn_file) { unlink(kmn_file); } -extern KMX_BOOL GetCompileTargetsFromTargetsStore(const KMX_WCHAR* store, int &targets); +extern KMX_BOOL GetCompileTargetsFromTargetsStore(KMX_WCHAR* buf, int &targets); + +KMX_BOOL do_GetCompileTargetsFromTargetsStore(const KMX_WCHAR* store, int &targets, const KMX_WCHAR* expected = nullptr) { + KMX_WCHAR* buf; + buf = new KMX_WCHAR[u16len(store)+1]; + u16cpy(buf, store); + KMX_BOOL result = GetCompileTargetsFromTargetsStore(buf, targets); + + if(expected) { + test_assert(u16cmp(expected, buf) == 0); + } + + delete[] buf; + return result; +} void test_GetCompileTargetsFromTargetsStore() { int targets = 0; setup(); - test_assert(GetCompileTargetsFromTargetsStore(u"any", targets)); + test_assert(do_GetCompileTargetsFromTargetsStore(u"any", targets, u"any")); test_assert(error_vec.size() == 0); test_assert(targets == (COMPILETARGETS_KMX | COMPILETARGETS_JS)); setup(); - test_assert(GetCompileTargetsFromTargetsStore(u"windows", targets)); + test_assert(do_GetCompileTargetsFromTargetsStore(u"windows", targets, u"windows")); test_assert(error_vec.size() == 0); test_assert(targets == COMPILETARGETS_KMX); setup(); - test_assert(GetCompileTargetsFromTargetsStore(u"desktop", targets)); + test_assert(do_GetCompileTargetsFromTargetsStore(u"desktop", targets, u"desktop")); test_assert(error_vec.size() == 0); test_assert(targets == COMPILETARGETS_KMX); setup(); - test_assert(GetCompileTargetsFromTargetsStore(u"mobile", targets)); + test_assert(do_GetCompileTargetsFromTargetsStore(u"mobile", targets, u"mobile")); test_assert(error_vec.size() == 0); test_assert(targets == COMPILETARGETS_JS); setup(); - test_assert(GetCompileTargetsFromTargetsStore(u"web", targets)); + test_assert(do_GetCompileTargetsFromTargetsStore(u"web", targets, u"web")); test_assert(error_vec.size() == 0); test_assert(targets == COMPILETARGETS_JS); setup(); - test_assert(GetCompileTargetsFromTargetsStore(u"desktop mobile", targets)); + test_assert(do_GetCompileTargetsFromTargetsStore(u"desktop mobile", targets, u"desktop mobile")); + test_assert(error_vec.size() == 0); + test_assert(targets == (COMPILETARGETS_KMX | COMPILETARGETS_JS)); + + setup(); + test_assert(do_GetCompileTargetsFromTargetsStore(u"desktop tablet", targets, u"desktop tablet")); + test_assert(error_vec.size() == 0); + test_assert(targets == (COMPILETARGETS_KMX | COMPILETARGETS_JS)); + + setup(); + test_assert(do_GetCompileTargetsFromTargetsStore(u" desktop tablet", targets, u"desktop tablet")); test_assert(error_vec.size() == 0); test_assert(targets == (COMPILETARGETS_KMX | COMPILETARGETS_JS)); setup(); - test_assert(GetCompileTargetsFromTargetsStore(u"desktop tablet", targets)); + test_assert(do_GetCompileTargetsFromTargetsStore(u" windows androidphone ", targets, u"windows androidphone")); test_assert(error_vec.size() == 0); test_assert(targets == (COMPILETARGETS_KMX | COMPILETARGETS_JS)); setup(); - test_assert(!GetCompileTargetsFromTargetsStore(u"foo bar baz", targets)); + test_assert(!do_GetCompileTargetsFromTargetsStore(u"foo bar baz", targets)); test_assert(error_vec.size() == 1); test_assert(error_vec[0] == KmnCompilerMessages::ERROR_InvalidTarget); test_assert(targets == 0); setup(); - test_assert(!GetCompileTargetsFromTargetsStore(u"windows chromeos", targets)); + test_assert(!do_GetCompileTargetsFromTargetsStore(u"windows chromeos", targets)); test_assert(error_vec.size() == 1); test_assert(error_vec[0] == KmnCompilerMessages::ERROR_InvalidTarget); test_assert(targets == 0); setup(); - test_assert(!GetCompileTargetsFromTargetsStore(u" ", targets)); + test_assert(!do_GetCompileTargetsFromTargetsStore(u" ", targets)); test_assert(error_vec.size() == 1); test_assert(error_vec[0] == KmnCompilerMessages::ERROR_NoTargetsSpecified); test_assert(targets == 0); setup(); - test_assert(!GetCompileTargetsFromTargetsStore(u"", targets)); + test_assert(!do_GetCompileTargetsFromTargetsStore(u"", targets)); test_assert(error_vec.size() == 1); test_assert(error_vec[0] == KmnCompilerMessages::ERROR_NoTargetsSpecified); test_assert(targets == 0); diff --git a/developer/src/kmcmplib/tests/gtest-km_u16.tests.cpp b/developer/src/kmcmplib/tests/gtest-km_u16.tests.cpp index 75d74607cd4..db40baf12d8 100644 --- a/developer/src/kmcmplib/tests/gtest-km_u16.tests.cpp +++ b/developer/src/kmcmplib/tests/gtest-km_u16.tests.cpp @@ -32,6 +32,11 @@ TEST(km_u16_Test, u16tok_char_delim) { KMX_WCHAR str[LINESIZE]; KMX_WCHAR *ctx = nullptr; + // invalid parameters + EXPECT_EQ(nullptr, u16tok(str, ' ', nullptr)); + EXPECT_EQ(nullptr, u16tok(str, (KMX_WCHAR)0, &ctx)); + EXPECT_EQ(nullptr, u16tok(nullptr, ' ', nullptr)); + // sequence of tokens u16cpy(str, u"test a space and two"); ctx = nullptr; @@ -55,6 +60,13 @@ TEST(km_u16_Test, u16tok_char_delim) { EXPECT_TRUE(!u16cmp(u"b", u16tok(nullptr, ' ', &ctx))); EXPECT_EQ(nullptr, u16tok(nullptr, ' ', &ctx)); + // delimiters at start + u16cpy(str, u" a b"); + ctx = nullptr; + EXPECT_TRUE(!u16cmp(u"a", u16tok(str, ' ', &ctx))); + EXPECT_TRUE(!u16cmp(u"b", u16tok(nullptr, ' ', &ctx))); + EXPECT_EQ(nullptr, u16tok(nullptr, ' ', &ctx)); + // no string, no context ctx = nullptr; EXPECT_EQ(nullptr, u16tok(nullptr, ' ', &ctx)); @@ -73,6 +85,12 @@ TEST(km_u16_Test, u16tok_str_delim) { KMX_WCHAR str[LINESIZE]; KMX_WCHAR *ctx = nullptr; + // invalid parameters + EXPECT_EQ(nullptr, u16tok(str, u" ", nullptr)); + EXPECT_EQ(nullptr, u16tok(str, nullptr, &ctx)); + EXPECT_EQ(nullptr, u16tok(str, u"", &ctx)); + EXPECT_EQ(nullptr, u16tok(nullptr, u" ", nullptr)); + // sequence of tokens u16cpy(str, u"test a space and two"); ctx = nullptr; @@ -96,6 +114,13 @@ TEST(km_u16_Test, u16tok_str_delim) { EXPECT_TRUE(!u16cmp(u"b", u16tok(nullptr, u" ", &ctx))); EXPECT_EQ(nullptr, u16tok(nullptr, u" ", &ctx)); + // delimiters at start + u16cpy(str, u" a b"); + ctx = nullptr; + EXPECT_TRUE(!u16cmp(u"a", u16tok(str, u" ", &ctx))); + EXPECT_TRUE(!u16cmp(u"b", u16tok(nullptr, u" ", &ctx))); + EXPECT_EQ(nullptr, u16tok(nullptr, ' ', &ctx)); + // no string, no context ctx = nullptr; EXPECT_EQ(nullptr, u16tok(nullptr, u"", &ctx)); @@ -130,6 +155,9 @@ TEST(km_u16_Test, u16tok_str_compare_to_strtok) { // Compare behaviour of strtok: char str[LINESIZE]; + // Note: strtok behavior is undefined with invalid parameters so we don't do a reference test + // of invalid parameters + // sequence of tokens strcpy(str, "test a space and two"); EXPECT_TRUE(!strcmp("test", strtok(str, " "))); @@ -150,6 +178,12 @@ TEST(km_u16_Test, u16tok_str_compare_to_strtok) { EXPECT_TRUE(!strcmp("b", strtok(nullptr, " "))); EXPECT_EQ(nullptr, strtok(nullptr, " ")); + // delimiters at start + strcpy(str, " a b"); + EXPECT_TRUE(!strcmp("a", strtok(str, " "))); + EXPECT_TRUE(!strcmp("b", strtok(nullptr, " "))); + EXPECT_EQ(nullptr, strtok(nullptr, " ")); + // multiple delimiters strcpy(str, "abcghi"); const char *delim = "<>"; From 2f60544835fbe596ce66984692d3af11bb7b2e4a Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Fri, 28 Aug 2026 17:31:06 +0200 Subject: [PATCH 2/2] docs(developer): tweak comments Co-authored-by: Eberhard Beilharz Co-authored-by: Sabine Schmitt --- developer/src/kmc-kmn/test/compiler.tests.ts | 5 ++++- developer/src/kmcmplib/src/Compiler.cpp | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/developer/src/kmc-kmn/test/compiler.tests.ts b/developer/src/kmc-kmn/test/compiler.tests.ts index c4bc0209298..ce27552585a 100644 --- a/developer/src/kmc-kmn/test/compiler.tests.ts +++ b/developer/src/kmc-kmn/test/compiler.tests.ts @@ -148,10 +148,13 @@ describe('Compiler class', function() { const result = await compileTestKeyboard(callbacks, ['keyboards', 'targets-with-whitespace.kmn']); assert.isNotNull(result); - // Verifies that targets has 'any' as it contains both JS and KMX targets + // Verify implictly that `&targets` store was interpreted correctly as 'any' + // because the compiler generated both JS and KMX targets (#13721) assert.isNotNull(result.artifacts.js); assert.isNotNull(result.artifacts.kmx); + // Then verify directly that the `&targets` store was trimmed by looking at + // the final value in the kmx data const reader = new KmxFileReader(); const keyboard = reader.read(result.artifacts.kmx.data); assert.equal(keyboard.targets, 'any'); diff --git a/developer/src/kmcmplib/src/Compiler.cpp b/developer/src/kmcmplib/src/Compiler.cpp index e73b2b9a452..8204cc2e2ea 100644 --- a/developer/src/kmcmplib/src/Compiler.cpp +++ b/developer/src/kmcmplib/src/Compiler.cpp @@ -1336,7 +1336,7 @@ KMX_BOOL ProcessSystemStore(PFILE_KEYBOARD fk, KMX_DWORD SystemID, PFILE_STORE s * &targets dpString value to remove unnecessary whitespace. Does not * reallocate sp->dpString, but overwrites its value with a string the * same length or shorter. - * @param sp store value to rewrite + * @param store store value to rewrite * @param targets (output) * @return FALSE if no targets found or invalid targets found */