Make the database portable and encryptable (#3848) - #5526
Conversation
The database API was five unrelated implementations sharing an interface. Cursors counted from zero on some ports and one on others, iOS reported success on an empty result set and returned null for every blob, the simulator could not seek at all, and no port could encrypt anything. This lands the port-independent half: - package-info.java now carries the normative contract every port must satisfy: zero-based positions, first() lands on a row, execute() runs a whole script while the parameterized forms take exactly one statement, typed parameter binding, flat transactions, IOException with a chained cause, idempotent close. - AbstractDBCursor derives all navigation from two primitives, rewind() and stepForward(), so every port gets identical semantics rather than each reimplementing them. Seeks rewind and re-step, which is what Android's windowed cursor already does on a window miss; buffering rows instead would mean materializing every column of every row stepped past. - SQLStatementSplitter splits a script the way SQLite does, respecting string literals, quoted identifiers, comments and CREATE TRIGGER bodies. - DatabaseConfig, DatabaseEncryptionException and ManagedKeys add keyed opens. Managed keys are resolved in the core so every platform derives identical material from an alias, and a key that cannot be stored is fatal rather than a silent downgrade to plaintext. - db.legacy restores each platform's previous behaviour for the ten changes that alter a previously successful result. It is read lazily, because the generated stubs set it after Display.init. Blob parameters now raise IOException rather than RuntimeException, and the truncated javadoc samples in Database, Cursor and Row are replaced with complete ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The simulator was the weakest database implementation, which mattered more than it sounds: it is where people develop. Its cursor could not seek at all, because the JDBC driver only produces TYPE_FORWARD_ONLY result sets and first(), last(), prev() and position() each threw outright. execute() silently ran the first statement of a script and discarded the rest. rollbackTransaction() left the connection outside autocommit, so every following statement quietly joined a new implicit transaction. Every query leaked its PreparedStatement. - SECursor now extends AbstractDBCursor, rewinding by re-executing the statement. The simulator has working random access for the first time. - execute(String) splits the script and runs each statement, rather than trusting a driver to decide how much of it to run. - The parameterized forms reject a multi-statement script instead of dropping its tail. - Statements are closed on the success path, cursors are closed with the database, close() is idempotent and rollback restores autocommit. - getColumnName reports the result set label, matching getColumnIndex, so an aliased column can be found under the name it was found by. The shaded driver moves from org.xerial to io.github.willena, which is the same driver with SQLite3MC compiled in: same package, same config, verified identical on plaintext databases, plus the SQLCipher-compatible cipher the simulator needs to open a database written on a device. getV4Defaults() is required over getDefault() - the latter selects SQLite3MC's own variant, which real SQLCipher cannot read. That driver also stops being frozen. Freezing assumed the shaded content never changed; it now carries a crypto-bearing engine that has to track upstream security releases. SEDatabaseConformanceTest runs the portable contract against the real SEDatabase headlessly in about two seconds, including both the strict and legacy modes and the encrypt/decrypt round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iOS was the port the "radically different implementations" complaint is
really about, and it had real bugs behind the divergence:
- sqlDbClose called sqlite3_free on the connection handle. That never
closed it, leaked the file descriptor, skipped the WAL checkpoint and
handed the pointer to the wrong allocator. Now sqlite3_close_v2.
- sqlCursorValueAtColumnBlob was { return nil; }, so iOS could not read
a blob at all, in either direction.
- Opening a database called sqlite3_config(SQLITE_CONFIG_SERIALIZED) and,
on failure, sqlite3_shutdown(). That has to run before
sqlite3_initialize() to do anything, and calling shutdown with
connections open is undefined behaviour. Replaced with per-connection
SQLITE_OPEN_FULLMUTEX.
Behaviour now matches the portable contract:
- CursorImpl extends AbstractDBCursor, so last(), prev() and position()
work instead of throwing "Unsupported", and first() lands on a row and
reports false for an empty result set rather than reporting success and
leaving the statement unpositioned.
- Parameters bind by runtime type through new statement natives. They
used to be stringified, which stored an Integer as TEXT, and a comment
conceded it "will probably fail with blobs".
- Parameter count mismatches and multi-statement scripts in the
parameterized forms are rejected rather than silently mis-executed.
- Errors carry sqlite3_errmsg unconditionally; the dead XMLVM branches
that gated error reporting are gone.
- finalize() is removed from the database and cursor. Closing sqlite
handles from the GC thread is the "platform specific nuance" that
defeated ThreadSafeDatabase.
- Custom file:// database paths work, matching Android and the simulator.
Keying is a separate native that reports success rather than throwing, so
the Java side can tell a wrong key from a failure to open the file
without the native layer naming a core exception class.
isDatabaseEncryptionSupported() asks the linked engine via PRAGMA
cipher_version rather than assuming, so it reports honestly on a build
that does not bundle a cipher-capable SQLite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Android was already the most capable port, so this is mostly tightening rather than rebuilding: - A null element in a String[] now binds SQL NULL. bindString rejects null, so passing one used to fail the whole statement. - execute(sql, (Object[]) null) no longer dereferences a null array. - execute(String) runs a whole script. execSQL refuses anything after the first statement, so the script is split and run statement by statement. - executeQuery forces the window fill before returning, so malformed SQL is reported there rather than from the first next(). rawQuery is lazy. - Transactions use the shared flat-transaction guards, so a nested begin is rejected here as it already was everywhere else. - Exceptions carry their cause and are no longer printStackTrace'd on the way out. - Cursors are invalidated when the database closes, close() is idempotent, getRow() off a row throws, getColumnIndex is case insensitive, and wasNull() is false before any value has been read. - Blob query parameters work, bound through a cursor factory, which is the only supported route: rawQuery can carry text arguments only. This is what androidx.sqlite does for the same reason. Encryption lives in a new com/codename1/impl/android/cipher package built on net.zetetic:sqlcipher-android. It compiles against classes that are only on the classpath of app builds that use encryption, so it is excluded from the port's own javac and reached purely by reflection, letting the builder delete it for every app that never touches DatabaseConfig. That gating is why the package is a near copy of AndroidDB rather than a shared supertype: any shared type naming net.zetetic would have to live in the part of the port that must stay deletable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both ports inherited the base openOrCreateDB, which returns null, so Database.openOrCreate() handed back null and calling code failed with a NullPointerException. They now have a full implementation that satisfies the same contract as every other port, encryption included. Neither runs a JVM, so JDBC was never an option; they needed a C binding. That is cheap because both are ParparVM C targets whose CMake project already compiles every .c in the source root. - The engine is SQLite3 Multiple Ciphers, bundled once in the translator and emitted only for applications that use com.codename1.db. iOS shares the same copy, so those three targets run one engine at one version, and the simulator's JDBC driver is built from the same upstream project. - The amalgamation is named .h deliberately. The iOS project generator lists .h but excludes it from the compile phase; CMake globs *.c for sources; and the ParparVM native symbol scanner reads only .c and .m. Named .c it would be compiled twice without its build options, named .inc it would ship inside the .ipa as 13MB of dead weight. - cn1_sqlite3.c is the single translation unit that compiles it, with the build options set immediately before the include so they cannot leak into unrelated sources. It is gated internally, so an emitted but disabled build produces an empty object rather than a link error. - The binding itself is shared. Both ports need identical code but mangle their entry points from different Java classes, so the logic lives once in cn1_db_sqlite_impl.h and each port's .c expands CN1_DB_DEFINE_NATIVES for its own prefix. Verified that every declared native has both its plain and its _R_ symbol in both ports. - iOS stops linking the system libsqlite3 when the bundled engine is used, rather than carrying two SQLite implementations in one process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JavaScript port sat on WebSQL, which Chrome removed in 119 and Firefox never implemented, so its database was dead on every current browser. What it did support was thin: transactions were printlns, getBlob threw, position(n) always returned the first row, close() did nothing, and the bridge busy-waited a CN1 thread on a lock. It now runs the same SQLite build the other ports use, compiled to WebAssembly, inside the application's own worker. Every call after the first is an ordinary synchronous call; only the initial load suspends, through the runtime's existing yield-on-promise support, so the lock and its 200ms poll are gone. Storage uses the opfs-sahpool VFS rather than the default OPFS one. The default needs crossOriginIsolated, which needs COOP/COEP response headers, which we cannot require of the arbitrary static hosting these bundles are deployed to. Browsers without synchronous OPFS access fall back to memory with a console warning, because silently losing every write on reload is not a failure anyone should discover in production. Gating, so nobody pays for what they do not use: - iOS emits the bundled engine, and drops the system libsqlite3, only for applications that reference DatabaseConfig. Everyone else keeps the system SQLite exactly as before. - Windows and Linux emit it for anything referencing com.codename1.db, since they have no system SQLite at all, and its cipher only when encryption is configured. - Android's SQLCipher package is deleted unless DatabaseConfig is referenced, and the AAR arrives through a new PlatformFeatureCatalog entry keyed on that same class. - The JavaScript builder prunes the 1.5MB engine from bundles that never open a database. The catalog entry is keyed on DatabaseConfig rather than the db package on purpose, and two new tests hold that line: every database application references com.codename1.db, so keying it there would bundle SQLCipher for all of them and push the minimum Android SDK from 19 to 23 for people who never asked for encryption. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contract and the encryption are only real if they are checked, and the portability claim in particular is the kind that fails silently: a cipher misconfiguration produces files each platform reads perfectly well on its own and nothing else can touch. - Seven device tests run the shared conformance suite on every port through the existing screenshot harness. They are assertion only, so they take no screenshots and sit before the ordering-sensitive graphics baselines. Ports without a database self-skip, so a port turns green on its own once it has one. - Two of the seven run in legacy mode, which is what makes the compatibility promise testable rather than aspirational: they fail the moment a refactor changes what db.legacy restores. - Two Port Status features expose the results publicly, split so a threading regression cannot blank the whole database row. - scripts/ci/db-cipher-interop.sh checks our encrypted files against the stock sqlcipher client in both directions, with a raw key to isolate the cipher configuration and a passphrase leg to cover the key derivation. Wired into the pull request workflow. The developer guide's SQL section said the iOS SQLite "isn't threadsafe" and warned that the garbage collector closing a connection would crash the app. That was true, and this branch is what fixes it, so the section is rewritten and extended with encryption, key management, threading, cursor cost and the legacy compatibility table. ThreadSafeDatabase is un-deprecated. Its note blamed platform nuances; the nuance was the iOS finalizers, now gone. Its close() was fire and forget, so it returned before the database was closed and a following delete() raced it, which is fixed here too. The cursor inner classes are static: with an explicit owner field the implicit outer reference was dead weight, which SpotBugs flagged on iOS and would eventually have flagged everywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce77b834d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Companion PR with the build-side gating: codenameone/BuildDaemon#172 |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
- The Ant build for the JavaSE port links whichever sqlite-jdbc is pinned in cn1-binaries, which has no org.sqlite.mc, so importing the driver's config builder broke that build for everyone. JavaSEPort now writes the SQLCipher connection properties out literally, which needs no extra class at compile time, and reports isDatabaseEncryptionSupported() by probing for the cipher-capable driver rather than assuming it. The simulator therefore answers honestly under either build. - The Windows cross-compile failed to link. The sample application now uses com.codename1.db, but that integration test drives the translator directly rather than through the builder, so the engine was never emitted and the natives had no definitions. Two fixes: the shared binding header is always emitted and defines every entry point either way, as real bindings or as stubs that raise a clear IOException, so an application always links however the translator was invoked; and the integration tests ask for the engine explicitly, so those ports actually exercise the database instead of only ever self-skipping. Verified that both branches of the header export an identical symbol set. - The developer guide requires snippets to live in docs/demos and be included by tag. Migrated with the repository's own migration script. The snippet harness had no com.codename1.db import, which is why all three failed to compile once moved; added, since it is a core package the guide documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cloudflare Preview
|
The Maven build already excluded it, but the Ant target compiles every source in the port, so it tried to build the package against net.zetetic and failed for anyone building that way -- including BuildDaemon CI, which clones this repo and runs the Ant target. Mirrors the exclusion into both places the ARCore and AI packages already use: the javac in Ports/Android/build.xml and the excludes property in nbproject/project.properties. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d595bd94da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Compared 12 screenshots: 12 matched. |
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Review findings, all eight real: - Database.encrypt() could never work on Android. The system SQLite has no cipher, so a plaintext database opened through it can never be re-keyed. Added openOrCreateDBForRekey(), which Android routes through SQLCipher (an empty key opens an unencrypted file, which can then be re-keyed). - A managed key resolves its keystore alias from the database name, and every port passed null when re-keying, so changeKey(managed()) raised a NullPointerException instead of encrypting. Each Database now retains the name it was opened under. - Two threads first-opening the same managed database could each see nothing stored, generate different keys and overwrite each other, leaving one of them holding data nobody could ever read. The read-generate-store sequence is now serialized. - isKeyHardwareBacked() inferred hardware backing from the API level, but emulators and plenty of real devices back AndroidKeyStore keys in software. It now asks the key itself, via KeyInfo. Applications are told they may use this to refuse to store sensitive data, so it has to be true. - checkEndTransaction() cleared the flag before the engine had ended the transaction, so a failed commit left the transaction open while the API believed it was closed, and the recovering rollback was rejected. Splitting out markTransactionEnded() means the flag drops only on success. A conformance check covers the failed-commit path. - An encrypted Android database opened by file:// URL had no toNativePath() conversion, so java.io.File treated the URL as a literal relative name. - Calling next() past the end repeatedly re-derived the row count each time, inflating it, after which last() would seek to a row that does not exist. Verified the new check fails against the old code (5 became 8). - PRAGMA rekey interpolated the key directly, so a passphrase containing a quote produced a different statement. Both Android and the simulator now go through one helper that quotes text and passes a raw key literal through untouched. CI failures: - Six SpotBugs findings in core-unittests, a module the earlier local runs had not covered: boxed constructors, a default-encoding String, and a Boolean-returning method that could return null. - The arm64 Linux and Windows cross-builds failed compiling the engine's ARM AES intrinsics. Where the compiler defines __ARM_FEATURE_CRYPTO the engine uses them directly, which is what Apple's toolchain does, so iOS is unaffected; otherwise it tags individual functions with __attribute__((target)), which the cross-compiling clang does not honour for these intrinsics. Rather than require ARM crypto extensions of every chip, that path now uses the software implementation. - DatabaseStatementLegacyTest failed on Android because the legacy expectation was wrong, not the code: only iOS ran a whole script before this branch, through sqlite3_exec. Android's execSQL and the simulator's PreparedStatement both dropped everything after the first statement. Corrected in the suite and in both places it is documented. - The migrated guide snippet fixture needed a copyright header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f2f2c70ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Compared 217 screenshots: 217 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0334a8dc1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // generated gradle links them through a fileTree, so encryption used only inside a | ||
| // library is invisible to a scan of the loose class tree -- and the build would then | ||
| // delete the cipher implementation out from under the library that calls it. | ||
| DatabaseUsage libraryUsage = scanForDatabaseUsage(libsDir); |
There was a problem hiding this comment.
When an AAR/JAR supplied through android.gradleDep or gradleDependencies calls DatabaseConfig.managed(), passphrase(), or rawKey() internally while the application only invokes a library API, this scan never sees that bytecode: it inspects only dummyClassesDir and libsDir, whereas those dependencies are added to build.gradle later (around lines 6423-6426). dbCipherSupport therefore remains false, and the builder deletes com/codename1/impl/android/cipher at lines 3583-3595, so the library's encrypted database open reports NOT_SUPPORTED at runtime. Either inspect resolved Gradle dependencies before pruning or provide a build hint that forces cipher inclusion.
Useful? React with 👍 / 👎.
…passes through My previous commit had this backwards, and the run said so: the assertion fired with textPixels=0 after the full thirty second poll, so the form I was waiting for was not a state that arrives -- it is one the panel passes through. The inspector is created and then moved into its own window by showInFrame(), and the docked Component Details panel it leaves behind settles empty. That is what the stored reference holds and what every run captures once it has settled; the failure was a capture taken mid-move, holding the form laid out with no values in it. So the capture now waits for the panel to be empty and the reference goes back to the settled state it always had. The fixed eight second timer is still what made this possible either way, which is why the wait stays: an assertion that names the unsettled panel is a better failure than a screenshot mismatch, whichever way the race goes next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c41c32d06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // process kill there strands it, the next open creates an empty database in its place, | ||
| // and the migration after that removes the stranded copy as stale leftovers. Renaming | ||
| // means there is a complete database under one of the two names at every instant. | ||
| if (!original.renameTo(backup)) { |
There was a problem hiding this comment.
When the Android source database uses journal_mode=PERSIST and has updated or deleted data, closing it intentionally leaves path-journal containing old plaintext pages. This swap renames only the main file to backup, installs the encrypted target at the original path, and later deletes only backup, so encrypt() can report success while recoverable plaintext remains beside the encrypted database. Remove and verify the source sidecars after the clean close and before installing the target.
Useful? React with 👍 / 👎.
| if (slot >= 1 && slot <= params.length && params[slot - 1] instanceof String) { | ||
| reserveAttachmentParameter((String) params[slot - 1]); | ||
| return; |
There was a problem hiding this comment.
When the object overload binds an integer, blob, or other non-string value as the ATTACH filename, SQLite coerces it to text and opens that file—for example, ATTACH ? AS aux with Integer.valueOf(123) opens a file named 123. This branch skips the actual target and the fallback loop ignores it too, so the pre-execution reservation and relative-path rejection are bypassed; a concurrent delete or rekey can therefore claim the attached file before reconciliation. Reject non-string filename values or convert and reserve them using the same representation SQLite uses.
Useful? React with 👍 / 👎.
| // Resolved and then normalized, so two spellings of one path are one registry entry: the | ||
| // claim a key change takes is worth nothing if the other connection is filed under | ||
| // "/a/./b" -- or under a link the engine sees through. | ||
| this.openKey = registryKeyFor(path); |
There was a problem hiding this comment.
When the same Linux database is opened through two hard-link names, getCanonicalPath() preserves the distinct directory entries, so the two handles receive different openKey values even though they share one inode. Each then appears to be the sole connection and may run changeKey(), rekeying the shared file underneath the other handle and leaving it using the old key. Use device/inode identity for the live connection and rekey registry, while retaining the path-derived identity for persistent managed-key aliases.
Useful? React with 👍 / 👎.
The wrapper did engage on protocol-e2e -- three attempts, thirty seconds apart, each rejected with "authorization failed for https://repo.maven.apache.org" for the JUnit BOM and the publishing plugin, before anything compiled. That is about a minute of coverage, and Central refused two different jobs on this branch today, so the window was simply shorter than the incident. Five attempts a minute apart on the three steps that carry the resolution-only pattern. It costs nothing on a real failure, which RETRY_ONLY_MATCHING still returns on the first attempt, and nothing on a genuinely unavailable dependency, which fails the same way five times. The refusal itself is not something this repository can fix: there is no settings.xml here, and the downloads are rejected within a third of a second, so whatever credentials reach Central come from the runner image. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad096aa8f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // container-relative one -- the open-database guard stopped firing and a database was | ||
| // unlinked underneath a live connection. Two spellings of one database still meet here, | ||
| // because the path is resolved before the container is taken off it. | ||
| this.openKey = aliasKey; |
There was a problem hiding this comment.
When the same iOS database is opened through two hard-link paths, managedKeyAliasForPath() uses realpath(), which resolves symlinks but preserves distinct hard-link names, so the two handles receive different openKey values even though they share one inode. Each handle can consequently appear to be the sole connection and run changeKey(), re-keying the file underneath the other handle and causing its later reads or writes to fail. Keep the persistent managed-key alias path-based, but use device/inode identity for the live connection and re-key registry.
Useful? React with 👍 / 👎.
It already retried, with the default three attempts at thirty seconds, and was refused on all three with "authorization failed for https://repo.maven.apache.org" before anything was built -- the fourth job Central has turned away on this branch today. Bounded on the step rather than by raising the default in retry.sh: half the call sites in these workflows have no RETRY_ONLY_MATCHING, and some of those run tests, so a larger default would widen exactly the blanket retrying the pattern exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fifth job in a day to die before touching our code, this time on "Plugin maven-install-plugin:2.5.2 or one of its dependencies could not be resolved". Same wrapper and the same resolution-only pattern as the others, so a failure in what this actually builds still fails on the first attempt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f33c50e9d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tinct Two ways one application's managed key could still be lost. synchronized covers threads in one VM and nothing between two, and an application can run in more than one process -- Android components declared with their own android:process, or two runs of a desktop build. Both could find nothing stored, generate different keys and each overwrite the other, leaving the database encrypted with a key that no longer exists. SecureStorage.setIfAbsent is the operation that was missing: it stores only when there is nothing there and answers with what the store ended up holding, so a caller that lost the race takes the winner's key instead of overwriting it, and both open the database with the same one. iOS implements it through SecItemAdd, which refuses a duplicate inside the keychain daemon and so is atomic between processes; the default is the best a store without that can do, and says so -- the check and the write are still two steps. The namespace sanitizer folded every character it could not carry onto "_", which is not reversible: com.acme.foo$bar and com.acme.foo_bar became one namespace, as did "My App" and "My_App", so two applications shared the store the namespace exists to keep apart. It now escapes those characters the way ManagedKeys.accountName escapes the account half of the same name, the escape character included. The keychain add is verified by building: the port jar rebuilt, the project regenerated from it, the symbol present in the staged IOSNative.m and xcodebuild reporting BUILD SUCCEEDED. Its second symbol also corrected -- I had spelled it _R_int_R_int, which resolves to nothing; the alias this file uses carries no return suffix, and the same mistake in secureStorageEntryStatePlain is fixed with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cf81644f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…cryptable-database # Conflicts: # docs/developer-guide/languagetool-accept.txt
Reading the store, generating a key and writing it back is three operations, and the review is right that reading again afterwards closes nothing: two processes can each complete all four steps and each believe its own key is the one stored. Only iOS was safe, because SecItemAdd refuses a duplicate inside the keychain daemon. The other four now have a gate of their own. Windows and Linux gained fileCreateExclusive, which is CREATE_NEW and O_CREAT|O_EXCL -- decided by the filesystem, so it cannot be won twice. The caller that creates the gate stores the value; the caller that finds it waits briefly for that value to land and never writes. The simulator takes a file lock, which is the mutual exclusion the JVM offers between processes rather than between threads, and re-reads Preferences inside it: that store caches per process, so without the sync the check would consult a copy taken before the other process wrote. Android creates the gate with createNewFile(), which is atomic, but its store is SharedPreferences and there is no way to make one process see another's write -- the cache is per process and MODE_MULTI_PROCESS was withdrawn for being unreliable. So the caller that loses reports nothing rather than writing, which turns a permanent silent corruption into a transient failure: KEY_UNAVAILABLE now, and the next launch reads the file fresh and finds the key. Also merges master, whose only conflict was two additions to the LanguageTool accept list, both kept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3d927401a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| java.io.File dir = new java.io.File(AndroidNativeUtil.getActivity() | ||
| .getApplicationContext().getFilesDir(), "cn1securestorage"); |
There was a problem hiding this comment.
When a managed database is first opened concurrently by components in separate android:process services before any Activity exists, getActivity() returns null, causing gateFile() to return null and setIfAbsent() to fall back to the non-atomic superclass implementation. Both processes can then generate and persist different keys through their independently cached SharedPreferences, potentially leaving the database encrypted with the key that lost the race. Build the gate path from AndroidNativeUtil.getContext().getApplicationContext() instead, as the storage code below already does for background-service support.
Useful? React with 👍 / 👎.
| } | ||
| boolean created; | ||
| try { | ||
| created = gate.createNewFile(); |
There was a problem hiding this comment.
After the first successful setIfAbsent(), this gate file remains permanently. If Database.forgetManagedKey() later removes the corresponding preference, reopening a newly created managed database under that alias finds no value but cannot recreate the gate; the false createNewFile() result leads to a null return and KEY_UNAVAILABLE on every subsequent attempt until app data is manually cleared. Successful removal of an account must also retire its gate, with synchronization that preserves the cross-process exclusion.
Useful? React with 👍 / 👎.
Three jobs on this branch timed out overnight and none of them reached any of our code: vm-tests spent its ninety minutes in "Install native build tools", and the Windows cross-compile and the website build each ran to GitHub's six hour ceiling in their own apt steps. All three had gone through scripts/ci/apt-get-update.sh a moment earlier, at 03:01 to 03:05 UTC, with the azure mirror answering Ign: on every index. Two things were missing. apt had no timeout, so a mirror that accepts the connection and then stalls is waited on forever -- and Acquire::Retries never comes into play, because nothing ever fails. And the settings were passed as options to apt-get update, so the apt-get install that follows in every caller inherited none of them. Both are fixed in one place: the script now drops the timeouts, retries and IPv4 preference into /etc/apt/apt.conf.d, which every later apt call in the job picks up, and runs the update itself under a five minute ceiling with three attempts. The two jobs that ran for six hours also had no timeout-minutes of their own, which is why a hang cost that much; they now have one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
These three do not go through scripts/ci/apt-get-update.sh -- they run as root in the CI container, without sudo -- so the timeouts that script installs never reach them. A stalled mirror there is still a hang rather than a failure, which is what cost three jobs their whole run overnight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
It arrived with the master merge without one, and the copyright gate is diff scoped: merging master pulled the file into this pull request's scope, where it failed. Nothing else about the file changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…test that never ran I added the seven database tests to the manifest and wrote "not-run" beside them in all eleven port reports, which is a statement that they were published and never executed. They do run: every port on this branch reports all seven passing, so the reports are replaced with the real thing -- run 32242868198 and its siblings on this head, not-run 0 across android, both iOS renderers, both Linux architectures, JavaScript, mac-native, tvOS, watchOS and both Windows architectures. The reason a hand-written absence survived is that nothing objected to it. A registered test sitting at "not-run" renders on the page exactly like one that runs and passes, so the contract now rejects it: a port that genuinely cannot do something reports "skip" from the suite itself, which is evidence, while "not-run" is the absence of evidence and the answer to it is to run the suite and check the report in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
VectorMapShapes failed on tvOS with a 4K frame whose basemap covered part of the viewport and whose remainder was the background colour -- 36% of the pixels, two and a half million of them past a quarter of the range, so not sampling noise. The cap never fired: no CN1SS:WARN, and the two sibling map tests matched, so the wait believed the map was rendered. isMapReady() derives the visible tile set from the component's current width and height, which means a run of "ready" answers is only worth anything if every one of them was asked about the same viewport. A layout pass that enlarges the map after the count reaches two leaves the tiles for the new area unrequested and unrendered, and the capture takes the frame in between. The file already carries the sibling of this hazard -- a first ready before the host's final layout pass, which resets the pixel ratio and clears the rendered cache -- and mitigates it with a minimum settle; this is the same fault line at the other end. The poll now resets its counter whenever the map's size differs from the size the last answer was given for, so two consecutive readies mean two readies for the viewport that will be captured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81f14b2311
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…cryptable-database
…a gate Both faults are in the gate I added, and both end the same way: an alias that can never be used again, reporting KEY_UNAVAILABLE long after its database is gone. remove() cleared the entry and left the gate. The next create then found no value of its own and a gate it could not take, so it reported nothing -- for good, since nothing removes that file. Android, Linux and Windows all now delete the gate as part of the removal, after the entry rather than before: a gate dropped first would let a second caller create a key while the old value was still in place. The gate was also named from account.hashCode(), and a hash is not a name: "Aa" and "BB" hash alike, so two aliases shared one file and whichever asked second could never create its key. The name is now derived from the account through the same reversible escape the namespace uses, in one place all four ports call -- the simulator included, where the file is a lock rather than a gate and a collision only costs a wait, but there is no reason for it to be the one place that hashes. Also merges master, and adds the header to the file it brought in without one, which is what the copyright gate was failing on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c483d140f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e it Two ways the gate I added could strand a managed key, and the capture race the JavaScript suite hit on the way past. A gate that is a file's existence outlives the process that made it. A run that died between creating the gate and storing the value left an alias that could never be created again: every later caller read that file as a live writer, waited for a value that was never coming, and reported KEY_UNAVAILABLE. The gate is now a lock -- flock on Linux, an unshared handle on Windows, FileChannel on Android -- which the operating system releases when the process ends however it ends. There is nothing left behind to recover, which is also why remove() no longer deletes that file: it gates nothing by existing, and deleting it under a process that holds the lock would let a second one lock a different file. The migration took the shared entry rather than copying it. Applications that shared an account name under one OS user all depend on that entry, and the first one to upgrade removed it -- so a later one saw nothing, generated a replacement and could no longer read its own database. Adoption now copies and leaves the source for whoever else still needs it, on the desktop ports and in the simulator alike, and marks itself adopted so this application stops consulting it: without that mark a forgotten key would come straight back from the entry it was copied from. Separately, graphics-draw-image-rect was captured with the top half of its grid drawn and the bottom half blank. That test already asks for a longer wait before it is declared ready, but the capture that follows asked for a fixed short one, and a test that draws in stages can be still for three frames between two of them. Both waits now come from one table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolves #3848.
The request was database encryption. Encryption is here, but the reason it took a
whole PR is that
com.codename1.dbwas not one API over SQLite -- it was fiveunrelated implementations that happened to share an interface, and there was no
sensible place to add a key to.
What was actually wrong
Verified in the source, not from memory:
openOrCreatenull, callers NPElast()/prev()/position()IOException("Unsupported")position(n)always gave row 0getPosition()basefirst()trueon an empty set, then reads unset memorygetBlob{ return nil; }execute(sql)multi-statementBEGINprintlnno-opsRuntimeExceptionon every portPlus three defects worth calling out on their own:
sqlDbClosecalledsqlite3_freeon asqlite3*, so no iOS connection was ever closed, the WAL wasnever checkpointed and the handle went to the wrong allocator;
SEDatabaseleakeda
PreparedStatementper query; andThreadSafeDatabase.close()was fire andforget, so a following
delete()raced it.And no device test touched
Databaseat all -- 142 test classes in the screenshotsuite, none of them about databases. That is why Windows and Linux were allowed to
ship with no implementation.
What this does
One contract.
com.codename1.db/package-info.javanow states what every portmust do, and
DatabaseConformanceSuitein the framework checks it. Seven devicetests run that suite on every port in CI; two of them run in legacy mode.
One cursor implementation.
AbstractDBCursorderives all navigation from twoprimitives,
rewind()andstepForward(), so ports stop re-deriving it. Seeksrewind and re-step rather than buffering:
sqlite3_column_*is only valid on thecurrent row, so buffering would mean copying every column of every row stepped
past, blobs included. This is what Android's windowed cursor already does on a
window miss.
Encryption, with a passphrase, a keystore-managed random key, or raw bytes.
Managed keys resolve in the core so every platform derives identical material from
an alias, and a key that cannot be stored is fatal rather than a silent downgrade
to plaintext.
Windows and Linux get a database at all.
JavaScript stops using WebSQL, which Chrome removed in 119 and Firefox never
implemented, in favour of the same SQLite compiled to WebAssembly.
Compatibility
Ten behaviours change in ways an application could depend on. All ten are restored
by the
db.legacybuild hint, per platform, and two device tests assert that itreally does restore them -- so the promise is testable rather than aspirational.
The table is in the developer guide.
The hint deliberately does not cover defects, or capabilities that used to throw
and now work. Nobody can depend on
getBlobreturning null.Cost, when unused
Nothing. iOS keeps the system SQLite unless the app references
DatabaseConfig;Android's SQLCipher package is deleted and its AAR never added; Windows and Linux
compile the engine to an empty object; the JavaScript builder prunes 1.5MB from
bundles that never open a database. Two catalog tests hold that line, because the
entry is keyed on
DatabaseConfigrather than the package -- keying it on thepackage would bundle SQLCipher for every database app and push Android's minimum
SDK from 19 to 23 for people who never asked for encryption.
Verification
SEDatabaseConformanceTestcases, all green.android,ios,codenameone-maven-pluginandByteCodeTranslator.scripts/ci/db-cipher-interop.sh, wired into PR CI, writes an encrypted databasewith our engine and reads it with the stock
sqlcipherclient, and vice versa,with both a raw key and a passphrase. This is the check that matters: a cipher
misconfiguration produces files each platform reads happily and nothing else can
touch, which no single-platform test would catch.
sqlcipher4.17.0 client and the realnet.zetetic:sqlcipher-androidAAR, not against assumed APIs.Three things the spikes caught
Worth recording, because each would have shipped broken:
sqlcipher_export()does not exist in SQLite3MC, so the ATTACH-basedmigration everyone writes would have failed.
PRAGMA rekeyworks, and alsopreserves
user_version, whichsqlcipher_exportdrops.getConnection()on the simulator but on first read onthe device ports, so both paths need handling.
SQLiteMCSqlCipherConfig.getDefault()really does produce files real SQLCiphercannot open;
getV4Defaults()is required. One line, and nothing but across-engine test would have found it.
Review rounds
Nineteen findings from the automated reviewers, all real, all fixed. The ones worth knowing about:
Database.encrypt()could never have worked on Android. The system SQLite has no cipher, so aplaintext database opened through it can never be re-keyed; there is now a platform hook that
routes the migration through SQLCipher.
nullwhen re-keying, so
changeKey(managed())raised aNullPointerExceptionrather than encrypting./,\,:and space all to_, socustomer/dbandcustomer_dbshared one key and forgetting either destroyed the other.
and
sqlite3_close_v2then leaves a zombie connection alive forever.isEncrypted()reported every plaintext JavaScript database as encrypted, because that port hasno readable path and a failed header read is indistinguishable from ciphertext.
PRAGMA rekeyinterpolated the key directly, so a passphrase containing a quote changed thestatement.
Two of the fixes are covered by new conformance checks, including one verified by reinstating the
old code and watching it fail: the exhausted-cursor count went 5 to 8 before the fix.
Two decisions worth a second opinion
maven/sqlite-jdbcis no longer frozen. It was pinned and excluded frompublication because a shade of a fixed driver never changed. It now carries the
engine used to read encrypted databases, so it has to track upstream security
releases. Costs ~13.5MB per release, which is what the freeze was avoiding.
compile. It ships a prebuilt amalgamation where SQLCipher would need its
configure script run per build, and it is what the simulator's JDBC driver is
already built from -- so iOS, Windows, Linux, JavaScript and the simulator all
run one engine at one version. Android still uses the SQLCipher AAR because it
cannot compile C in our build; both write the same format, which is the part
that matters.
Companion PR
The build-side gating is mirrored in codenameone/BuildDaemon#172, which is green.
🤖 Generated with Claude Code