microsoft
Android build with curl and SQLite attempts to use "/data/local/tmp" as a writable folder
Describe your environment. Android Version 9. Android NDK 21.3. cpp_client_telemetry built with USE_ROOM=0 and USE_CURL=1. cpp_client_telemetry built at tag "3.5.25".
Steps to reproduce. Call "LogManager::Initialize" with a tenant token.
What is the expected behavior? LogManager::Initialize is successful, no errors written to LogCat, telemetry uploaded.
What is the actual behavior? LogCat reports:
2021-02-24 23:10:16.678 15061-15061/com.microsoft.androidapp E/EventsSDK.SQLiteDB: Failed to open database file: (14) unable to open database file
2021-02-24 23:10:16.679 15061-15061/com.microsoft.androidapp E/EventsSDK.SQLiteDB: Failed to open database file: (14) unable to open database file
2021-02-24 23:10:16.679 15061-15061/com.microsoft.androidapp E/EventsSDK.Storage: No database could be opened
Error code 14 is SQLite's "SQLITE_CANTOPEN". Debugging through the code shows that OfflineStorage_SQLite::Initialize is attempting to create a database file under "/data/local/tmp", which isn't writable for my application. Setting the "CFG_STR_CACHE_FILE_PATH" configuration to be a writable path gets further, but fails with:
2021-02-24 22:48:04.517 13948-13948/com.microsoft.androidapp E/EventsSDK.SQLiteDB: Failed to prepare SQL statement "PRAGMA temp_store_directory = '/data/local/tmp/'": 1 (not a writable directory)
Because OfflineStorage_SQLite::initializeDatabase uses Utils.cpp GetTempDirectory() which - on Android using curl - defaults to /data/local/tmp which isn't writable as an application.
@mschofie @larvacea - Mark, is there a practical reason you don't want to use Rooms and Java layer HTTP stack in your Android app? If you can't use rooms, we can discuss how to pass the path to application-specific cache writable directory via config param.
My team ship an SDK, and would like to encapsulate the implementation details as much as possible. In the past we've taken on Java-side dependencies (specifically an HTTP client stack - OkHttp3) and received feedback that it makes the SDK less 'palatable' to consume. I was hoping to use libcurl and libsqlite3 to link everything into our native .SO and leave our Java-side as minimal as possible.
I've pulled together a workaround where I introduced a CFG_STR_CACHE_TEMP_FOLDER_PATH configuration that OfflineStorage_SQLite prefers over Utils' GetTempDirectory and am currently seeing if that's a viable path forward.
@larvacea @mschofie - this is reasonable solution. However, if there's a mechanism how we can automatically detect the package path (maybe by inspecting /proc/self or using this approach: https://stackoverflow.com/questions/42918762/how-to-get-app-package-name-or-applicationid-using-jni-android ), then that'd be much nicer. Since then we won't be needing a separate config option. What do you guys think? Basically defaulting to in-app package path, either cache or whatever is the standard writable dir used for storing app package files.
To get anything app-related, then LogManager would need the Android Context for the application. At the minute, the Context doesn't appear to be passed in (I could be missing it; I took only a shallow dive into the Java-side) - if it was a supported configuration value then I could see that being incredibly useful. That would allow JNI calls to get other Context details like the package name, package cache path, etc...
@mschofie
There should be a way to obtain the package name (and thus, anticipated /data path for the package) using purely C++ code:
https://stackoverflow.com/questions/34072185/ndk-get-pid-from-package-name
This only requires NDK. But needs to be tested on latest Android, esp. in environments like Knox, which may restrict arbitrary access to /proc filesystem.
However, the approach you are taking with CFG_STR_CACHE_TEMP_FOLDER_PATH is also valid.
The other third option is for Android build without Rooms DB - to extract the path information from existing CFG_STR_CACHE_FILE_PATH config variable. That way the app may supply the absolute path file to DB. And we can assume that the directory containing that DB file should be used as the temporary dir for other DB-related operations.
What's your opinion on that? Would you be ready to contribute a PR with either of these options?
If I remember it right, if you supply absolute path to storage file that starts with / - you should not be needing the new temporary path variable:
https://github.com/microsoft/cpp_client_telemetry/blob/001d97f7c669acf8e386c7fba10216283c9e1b90/lib/api/LogManagerImpl.cpp#L189
e.g. CFG_STR_CACHE_FILE_PATH=/data/data/$packagename/databases/telemetry.db should work, but your SDK code will have to extract the $packagename - either expecting it to be extracted from Context passed down to your SDK (not to 1DS SDK), OR extracted via NDK mechanism above - since the info in /proc should expose the package name by pid.
Correct. I was able to get further by setting CFG_STR_CACHE_FILE_PATH to a fully qualified path to a writable folder (and, yes, I used the Context to call Context.getCacheDir() from native code using JNIEnv* calls). But I fail later, when: OfflineStorage_SQLite sets PRAGMA temp_store_directory to the result of GetTempDirectory():
https://github.com/microsoft/cpp_client_telemetry/blob/001d97f7c669acf8e386c7fba10216283c9e1b90/lib/offline/OfflineStorage_SQLite.cpp#L691
I introduced CFG_STR_CACHE_TEMP_FOLDER_PATH to workaround the PRAGMA temp_store_directory usage.
Sorry, my original wording was confusing. I was trying to highlight that the 'default' was wrong, and that setting CFG_STR_CACHE_FILE_PATH still wasn't enough.
Sorry, I wonder if we can fix this in the OfflineStorage_SQLite.cpp itself, by adding #ifdef ANDROID. Then:
- either do not apply the pragma at all, OR
- assign the
temp_store_directoryto the path extracted fromCFG_STR_CACHE_FILE_PATH?
That way we won't be needing a new config variable.
temp_store_directory seems like a bit of regression introduced by this commit last year:
https://github.com/microsoft/cpp_client_telemetry/commit/349584f6cfb8697b5e19d3de8ecf4a66f42c9c47
Prior to that the pragma was unnecessary. What happens if you comment that with #ifndef ANDROID or alike? it should work...
#ifndef __ANDROID__
// Specify TEMP directory only for Win32 and POSIX, but not Android
std::ostringstream tempPragma;
tempPragma << "PRAGMA temp_store_directory = '" << GetTempDirectory() << "'";
SqliteStatement(*m_db, tempPragma.str().c_str()).select();
const char * result = sqlite3_temp_directory;
LOG_INFO("Set sqlite3 temp_store_directory to '%s'", result);
#endif
I didn't try not specifying the PRAGMA - the SQLite documentation for temporary files calls out the paths it tries, and I was skeptical that they would work on Android, so I figured that the PRAGMA was probably necessary. I'll give it a try and see how it goes...? Unfortunately I'm not an expert on SQLite, so I'm not sure when it would attempt to create temporary files - I don't know what due diligence would be needed to validate that SQLite is working correctly with a 'default' temp_store_directory...
And - presumably - folks are using cpp_client_telemetry on Android already, so are probably being successful with the current implementation of GetTempDirectory() which comes through HttpClient_Android::GetCacheFilePath(). I'd hate to change their behavior. Which is why a new option was nice (although - I admit - increasing complexity) in that it allows optionally overriding the current default behavior.
And - presumably - folks are using cpp_client_telemetry on Android already
Most teams using this SDK on Android, those I am aware of, are relying on Java layer Rooms DB. So applying the patch to disable this pragma for pure native SQLite may work well..
Yeah, specifying working dir under package path instead of temp seems to be safer indeed. I think originally the code worked well (/data/local/tmp), because that is the path always writable on Android dev device and Emulator.