Skip to content

Commit 7a9a468

Browse files
Siddhesh2377claude
andcommitted
fix(lora): address remaining CodeRabbit PR #414 review comments
- C++: use new(std::nothrow) to prevent std::bad_alloc across C boundary - C++: fix dangling pointer on OOM during lora re-registration - C++: add empty-path checks to load_lora and remove_lora - Kotlin SDK: replace fragile regex JSON parsing with org.json - Android: add HTTPS-only URL validation for adapter downloads - Android: validate downloaded file size against catalog entry - Android: fix concurrent refreshLoraState race condition - Android: move LoraViewModel out of composable body in ChatScreen - Android: wrap Timber.i in LaunchedEffect in MainActivity - Docs: add per-function error codes to rac_lora_registry.h Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 6b28b21 commit 7a9a468

File tree

8 files changed

+106
-72
lines changed

8 files changed

+106
-72
lines changed

examples/android/RunAnywhereAI/app/src/main/java/com/runanywhere/runanywhereai/MainActivity.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import androidx.compose.foundation.layout.fillMaxSize
99
import androidx.compose.material3.MaterialTheme
1010
import androidx.compose.material3.Surface
1111
import androidx.compose.runtime.Composable
12+
import androidx.compose.runtime.LaunchedEffect
1213
import androidx.compose.runtime.collectAsState
1314
import androidx.compose.runtime.getValue
1415
import androidx.compose.runtime.rememberCoroutineScope
@@ -76,7 +77,7 @@ class MainActivity : ComponentActivity() {
7677
}
7778

7879
is SDKInitializationState.Ready -> {
79-
Timber.i("App is ready to use!")
80+
LaunchedEffect(Unit) { Timber.i("App is ready to use!") }
8081
AppNavigation()
8182
}
8283
}

examples/android/RunAnywhereAI/app/src/main/java/com/runanywhere/runanywhereai/presentation/chat/ChatScreen.kt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,11 +73,13 @@ import java.util.Locale
7373

7474
@OptIn(ExperimentalMaterial3Api::class)
7575
@Composable
76-
fun ChatScreen(viewModel: ChatViewModel = viewModel()) {
76+
fun ChatScreen(
77+
viewModel: ChatViewModel = viewModel(),
78+
loraViewModel: LoraViewModel = viewModel(),
79+
) {
7780
val uiState by viewModel.uiState.collectAsStateWithLifecycle()
7881
val listState = rememberLazyListState()
7982
val scope = rememberCoroutineScope()
80-
val loraViewModel: LoraViewModel = viewModel()
8183

8284
var showingConversationList by remember { mutableStateOf(false) }
8385
var showingModelSelection by remember { mutableStateOf(false) }

examples/android/RunAnywhereAI/app/src/main/java/com/runanywhere/runanywhereai/presentation/chat/ChatViewModel.kt

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,9 @@ import com.runanywhere.sdk.public.extensions.LLM.ToolCallingOptions
3131
import com.runanywhere.sdk.public.extensions.LLM.ToolCallFormat
3232
import com.runanywhere.sdk.public.extensions.LLM.RunAnywhereToolCalling
3333
import com.runanywhere.runanywhereai.presentation.settings.ToolSettingsViewModel
34+
import kotlinx.coroutines.Dispatchers
3435
import kotlinx.coroutines.Job
36+
import kotlinx.coroutines.withContext
3537
import kotlinx.coroutines.flow.MutableStateFlow
3638
import kotlinx.serialization.json.Json
3739
import kotlinx.serialization.json.JsonElement
@@ -801,10 +803,12 @@ class ChatViewModel(application: Application) : AndroidViewModel(application) {
801803
}
802804

803805
/** Refresh LoRA loaded state for the active adapters indicator. */
806+
private var loraRefreshJob: Job? = null
804807
fun refreshLoraState() {
805-
viewModelScope.launch {
808+
loraRefreshJob?.cancel()
809+
loraRefreshJob = viewModelScope.launch {
806810
try {
807-
val loaded = RunAnywhere.getLoadedLoraAdapters()
811+
val loaded = withContext(Dispatchers.IO) { RunAnywhere.getLoadedLoraAdapters() }
808812
_uiState.value = _uiState.value.copy(hasActiveLoraAdapter = loaded.isNotEmpty())
809813
} catch (e: Exception) {
810814
Timber.e(e, "Failed to refresh LoRA state")

examples/android/RunAnywhereAI/app/src/main/java/com/runanywhere/runanywhereai/presentation/lora/LoraViewModel.kt

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,11 @@ import kotlinx.coroutines.flow.asStateFlow
2424
import kotlinx.coroutines.flow.update
2525
import kotlinx.coroutines.ensureActive
2626
import kotlinx.coroutines.launch
27+
import kotlinx.coroutines.sync.Mutex
28+
import kotlinx.coroutines.sync.withLock
2729
import kotlinx.coroutines.withContext
2830
import java.io.File
29-
import java.net.URL
31+
import java.net.URI
3032

3133
data class LoraUiState(
3234
val registeredAdapters: List<LoraAdapterCatalogEntry> = emptyList(),
@@ -47,6 +49,7 @@ class LoraViewModel(application: Application) : AndroidViewModel(application) {
4749
private val _uiState = MutableStateFlow(LoraUiState())
4850
val uiState: StateFlow<LoraUiState> = _uiState.asStateFlow()
4951
private var downloadJob: Job? = null
52+
private val downloadMutex = Mutex()
5053

5154
private val loraDir: File by lazy {
5255
File(application.filesDir, "lora_adapters").also { it.mkdirs() }
@@ -177,13 +180,42 @@ class LoraViewModel(application: Application) : AndroidViewModel(application) {
177180
private fun scanDownloadedAdapters(adapters: List<LoraAdapterCatalogEntry>): Map<String, String> {
178181
return adapters.mapNotNull { entry ->
179182
val file = File(loraDir, entry.filename)
183+
// Validate resolved path stays under loraDir to prevent path traversal
184+
if (!file.canonicalPath.startsWith(loraDir.canonicalPath + File.separator)) {
185+
Timber.w("Skipping adapter with invalid filename (path traversal): ${entry.filename}")
186+
return@mapNotNull null
187+
}
180188
if (file.exists()) entry.id to file.absolutePath else null
181189
}.toMap()
182190
}
183191

184192
/** Download a LoRA adapter GGUF file. */
185193
fun downloadAdapter(entry: LoraAdapterCatalogEntry) {
186-
if (_uiState.value.downloadingAdapterId != null) return
194+
viewModelScope.launch {
195+
// Mutex ensures only one download starts even under concurrent calls
196+
if (!downloadMutex.tryLock()) return@launch
197+
try {
198+
if (_uiState.value.downloadingAdapterId != null) return@launch
199+
startDownload(entry)
200+
} finally {
201+
downloadMutex.unlock()
202+
}
203+
}
204+
}
205+
206+
private fun startDownload(entry: LoraAdapterCatalogEntry) {
207+
val destFile = File(loraDir, entry.filename)
208+
// Validate resolved path stays under loraDir
209+
if (!destFile.canonicalPath.startsWith(loraDir.canonicalPath + File.separator)) {
210+
_uiState.update { it.copy(error = "Invalid adapter filename") }
211+
return
212+
}
213+
// Only allow HTTPS downloads to prevent MITM attacks
214+
val uri = try { URI(entry.downloadUrl) } catch (_: Exception) { null }
215+
if (uri == null || uri.scheme?.lowercase() != "https") {
216+
_uiState.update { it.copy(error = "Only HTTPS download URLs are allowed") }
217+
return
218+
}
187219

188220
_uiState.update {
189221
it.copy(
@@ -194,12 +226,11 @@ class LoraViewModel(application: Application) : AndroidViewModel(application) {
194226
}
195227

196228
downloadJob = viewModelScope.launch {
197-
val destFile = File(loraDir, entry.filename)
198229
val tmpFile = File(loraDir, "${entry.filename}.tmp")
199230
var downloadComplete = false
200231
try {
201232
withContext(Dispatchers.IO) {
202-
val connection = URL(entry.downloadUrl).openConnection().apply {
233+
val connection = URI(entry.downloadUrl).toURL().openConnection().apply {
203234
connectTimeout = 30_000
204235
readTimeout = 60_000
205236
}
@@ -222,6 +253,13 @@ class LoraViewModel(application: Application) : AndroidViewModel(application) {
222253
}
223254
}
224255
}
256+
// Validate downloaded file size if catalog provides one
257+
if (entry.fileSize > 0 && tmpFile.length() != entry.fileSize) {
258+
tmpFile.delete()
259+
throw Exception(
260+
"Downloaded file size (${tmpFile.length()}) does not match expected size (${entry.fileSize})"
261+
)
262+
}
225263
destFile.delete()
226264
if (!tmpFile.renameTo(destFile)) {
227265
tmpFile.delete()
@@ -272,6 +310,11 @@ class LoraViewModel(application: Application) : AndroidViewModel(application) {
272310
viewModelScope.launch {
273311
try {
274312
val file = File(loraDir, entry.filename)
313+
// Validate resolved path stays under loraDir
314+
if (!file.canonicalPath.startsWith(loraDir.canonicalPath + File.separator)) {
315+
_uiState.update { it.copy(error = "Invalid adapter filename") }
316+
return@launch
317+
}
275318
withContext(Dispatchers.IO) {
276319
// Always try to unload — ignore errors if not loaded
277320
try {

sdk/runanywhere-commons/include/rac/infrastructure/model_management/rac_lora_registry.h

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ typedef struct rac_lora_registry* rac_lora_registry_handle_t;
4444
/**
4545
* @brief Create a new LoRA adapter registry
4646
* @param out_handle Output: handle to the newly created registry
47-
* @return RAC_SUCCESS or error code
47+
* @return RAC_SUCCESS, RAC_ERROR_INVALID_ARGUMENT (NULL out_handle),
48+
* or RAC_ERROR_OUT_OF_MEMORY
4849
*/
4950
RAC_API rac_result_t rac_lora_registry_create(rac_lora_registry_handle_t* out_handle);
5051

@@ -60,10 +61,12 @@ RAC_API void rac_lora_registry_destroy(rac_lora_registry_handle_t handle);
6061
* @brief Register a LoRA adapter entry in the registry
6162
*
6263
* The entry is deep-copied; the caller retains ownership of the original.
64+
* If an entry with the same id already exists, it is replaced.
6365
*
6466
* @param handle Registry handle
6567
* @param entry Adapter entry to register (must have a non-NULL id)
66-
* @return RAC_SUCCESS or error code
68+
* @return RAC_SUCCESS, RAC_ERROR_INVALID_ARGUMENT (NULL handle/entry/id),
69+
* or RAC_ERROR_OUT_OF_MEMORY
6770
*/
6871
RAC_API rac_result_t rac_lora_registry_register(rac_lora_registry_handle_t handle,
6972
const rac_lora_entry_t* entry);
@@ -84,7 +87,8 @@ RAC_API rac_result_t rac_lora_registry_remove(rac_lora_registry_handle_t handle,
8487
* @param handle Registry handle
8588
* @param out_entries Output: array of deep-copied entries (caller must free with rac_lora_entry_array_free)
8689
* @param out_count Output: number of entries
87-
* @return RAC_SUCCESS or error code
90+
* @return RAC_SUCCESS, RAC_ERROR_INVALID_ARGUMENT (NULL params),
91+
* or RAC_ERROR_OUT_OF_MEMORY
8892
*/
8993
RAC_API rac_result_t rac_lora_registry_get_all(rac_lora_registry_handle_t handle,
9094
rac_lora_entry_t*** out_entries,
@@ -96,7 +100,8 @@ RAC_API rac_result_t rac_lora_registry_get_all(rac_lora_registry_handle_t handle
96100
* @param model_id Model ID to match against each entry's compatible_model_ids
97101
* @param out_entries Output: array of matching deep-copied entries (caller must free with rac_lora_entry_array_free)
98102
* @param out_count Output: number of matching entries
99-
* @return RAC_SUCCESS or error code
103+
* @return RAC_SUCCESS, RAC_ERROR_INVALID_ARGUMENT (NULL params),
104+
* or RAC_ERROR_OUT_OF_MEMORY
100105
*/
101106
RAC_API rac_result_t rac_lora_registry_get_for_model(rac_lora_registry_handle_t handle,
102107
const char* model_id,
@@ -108,7 +113,8 @@ RAC_API rac_result_t rac_lora_registry_get_for_model(rac_lora_registry_handle_t
108113
* @param handle Registry handle
109114
* @param adapter_id ID of the adapter to look up
110115
* @param out_entry Output: deep-copied entry (caller must free with rac_lora_entry_free)
111-
* @return RAC_SUCCESS or RAC_ERROR_NOT_FOUND
116+
* @return RAC_SUCCESS, RAC_ERROR_INVALID_ARGUMENT (NULL params),
117+
* RAC_ERROR_NOT_FOUND, or RAC_ERROR_OUT_OF_MEMORY
112118
*/
113119
RAC_API rac_result_t rac_lora_registry_get(rac_lora_registry_handle_t handle,
114120
const char* adapter_id,

sdk/runanywhere-commons/src/features/llm/llm_component.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -782,7 +782,7 @@ extern "C" rac_result_t rac_llm_component_load_lora(rac_handle_t handle,
782782
float scale) {
783783
if (!handle)
784784
return RAC_ERROR_INVALID_HANDLE;
785-
if (!adapter_path)
785+
if (!adapter_path || adapter_path[0] == '\0')
786786
return RAC_ERROR_INVALID_ARGUMENT;
787787

788788
auto* component = reinterpret_cast<rac_llm_component*>(handle);
@@ -805,7 +805,7 @@ extern "C" rac_result_t rac_llm_component_remove_lora(rac_handle_t handle,
805805
const char* adapter_path) {
806806
if (!handle)
807807
return RAC_ERROR_INVALID_HANDLE;
808-
if (!adapter_path)
808+
if (!adapter_path || adapter_path[0] == '\0')
809809
return RAC_ERROR_INVALID_ARGUMENT;
810810

811811
auto* component = reinterpret_cast<rac_llm_component*>(handle);

sdk/runanywhere-commons/src/infrastructure/model_management/lora_registry.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <cstring>
1111
#include <map>
1212
#include <mutex>
13+
#include <new>
1314
#include <string>
1415
#include <vector>
1516

@@ -21,6 +22,7 @@ struct rac_lora_registry {
2122
std::mutex mutex;
2223
};
2324

25+
// Forward declaration — needed by deep_copy_lora_entry for OOM cleanup
2426
static void free_lora_entry(rac_lora_entry_t* entry);
2527

2628
static rac_lora_entry_t* deep_copy_lora_entry(const rac_lora_entry_t* src) {
@@ -85,7 +87,8 @@ static void free_lora_entry(rac_lora_entry_t* entry) {
8587

8688
rac_result_t rac_lora_registry_create(rac_lora_registry_handle_t* out_handle) {
8789
if (!out_handle) return RAC_ERROR_INVALID_ARGUMENT;
88-
rac_lora_registry* registry = new rac_lora_registry();
90+
rac_lora_registry* registry = new (std::nothrow) rac_lora_registry();
91+
if (!registry) return RAC_ERROR_OUT_OF_MEMORY;
8992
RAC_LOG_INFO("LoraRegistry", "LoRA registry created");
9093
*out_handle = registry;
9194
return RAC_SUCCESS;
@@ -106,10 +109,11 @@ rac_result_t rac_lora_registry_register(rac_lora_registry_handle_t handle,
106109
if (!handle || !entry || !entry->id) return RAC_ERROR_INVALID_ARGUMENT;
107110
std::lock_guard<std::mutex> lock(handle->mutex);
108111
std::string adapter_id = entry->id;
109-
auto it = handle->entries.find(adapter_id);
110-
if (it != handle->entries.end()) { free_lora_entry(it->second); }
111112
rac_lora_entry_t* copy = deep_copy_lora_entry(entry);
112113
if (!copy) return RAC_ERROR_OUT_OF_MEMORY;
114+
// Free old entry AFTER successful deep_copy to avoid dangling pointer on OOM
115+
auto it = handle->entries.find(adapter_id);
116+
if (it != handle->entries.end()) { free_lora_entry(it->second); }
113117
handle->entries[adapter_id] = copy;
114118
RAC_LOG_DEBUG("LoraRegistry", "LoRA adapter registered: %s", entry->id);
115119
return RAC_SUCCESS;

sdk/runanywhere-kotlin/src/jvmAndroidMain/kotlin/com/runanywhere/sdk/foundation/bridge/extensions/CppBridgeLoraRegistry.kt

Lines changed: 27 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
package com.runanywhere.sdk.foundation.bridge.extensions
1010

1111
import com.runanywhere.sdk.native.bridge.RunAnywhereBridge
12+
import org.json.JSONArray
13+
import org.json.JSONObject
1214

1315
object CppBridgeLoraRegistry {
1416
private const val TAG = "CppBridge/CppBridgeLoraRegistry"
@@ -49,20 +51,26 @@ object CppBridgeLoraRegistry {
4951
return parseLoraEntryArrayJson(json)
5052
}
5153

52-
// JSON Parsing
54+
// JSON Parsing — uses org.json for correctness with special characters
5355

54-
private fun parseLoraEntryJson(json: String): LoraEntry? {
55-
if (json == "null" || json.isBlank()) return null
56+
private fun parseLoraEntryJson(obj: JSONObject): LoraEntry? {
5657
return try {
58+
val id = obj.optString("id", "").takeIf { it.isNotEmpty() } ?: return null
59+
val modelIds = mutableListOf<String>()
60+
obj.optJSONArray("compatible_model_ids")?.let { arr ->
61+
for (i in 0 until arr.length()) {
62+
arr.optString(i)?.takeIf { it.isNotEmpty() }?.let { modelIds.add(it) }
63+
}
64+
}
5765
LoraEntry(
58-
id = extractString(json, "id") ?: return null,
59-
name = extractString(json, "name") ?: "",
60-
description = extractString(json, "description") ?: "",
61-
downloadUrl = extractString(json, "download_url") ?: "",
62-
filename = extractString(json, "filename") ?: "",
63-
compatibleModelIds = extractStringArray(json, "compatible_model_ids"),
64-
fileSize = extractLong(json, "file_size"),
65-
defaultScale = extractFloat(json, "default_scale"),
66+
id = id,
67+
name = obj.optString("name", ""),
68+
description = obj.optString("description", ""),
69+
downloadUrl = obj.optString("download_url", ""),
70+
filename = obj.optString("filename", ""),
71+
compatibleModelIds = modelIds,
72+
fileSize = obj.optLong("file_size", 0L),
73+
defaultScale = obj.optDouble("default_scale", 0.0).toFloat(),
6674
)
6775
} catch (e: Exception) {
6876
log(LogLevel.ERROR, "Failed to parse LoRA entry JSON: ${e.message}")
@@ -71,50 +79,16 @@ object CppBridgeLoraRegistry {
7179
}
7280

7381
private fun parseLoraEntryArrayJson(json: String): List<LoraEntry> {
74-
if (json == "[]" || json.isBlank()) return emptyList()
75-
val entries = mutableListOf<LoraEntry>()
76-
var depth = 0; var objectStart = -1
77-
for (i in json.indices) {
78-
when (json[i]) {
79-
'{' -> { if (depth == 0) objectStart = i; depth++ }
80-
'}' -> {
81-
depth--
82-
if (depth == 0 && objectStart >= 0) {
83-
parseLoraEntryJson(json.substring(objectStart, i + 1))?.let { entries.add(it) }
84-
objectStart = -1
85-
}
86-
}
82+
if (json.isBlank() || json == "[]") return emptyList()
83+
return try {
84+
val array = JSONArray(json)
85+
(0 until array.length()).mapNotNull { i ->
86+
parseLoraEntryJson(array.getJSONObject(i))
8787
}
88+
} catch (e: Exception) {
89+
log(LogLevel.ERROR, "Failed to parse LoRA entry array JSON: ${e.message}")
90+
emptyList()
8891
}
89-
return entries
90-
}
91-
92-
private fun extractString(json: String, key: String): String? {
93-
val regex = Regex(""""$key"\s*:\s*"((?:[^"\\]|\\.)*)"""")
94-
return regex.find(json)?.groupValues?.get(1)?.takeIf { it.isNotEmpty() }
95-
}
96-
97-
private fun extractLong(json: String, key: String): Long {
98-
val regex = Regex(""""$key"\s*:\s*(-?\d+)""")
99-
return regex.find(json)?.groupValues?.get(1)?.toLongOrNull() ?: 0L
100-
}
101-
102-
private fun extractFloat(json: String, key: String): Float {
103-
val regex = Regex(""""$key"\s*:\s*(-?[\d.]+)""")
104-
return regex.find(json)?.groupValues?.get(1)?.toFloatOrNull() ?: 0f
105-
}
106-
107-
private fun extractStringArray(json: String, key: String): List<String> {
108-
val keyMatch = Regex(""""$key"\s*:\s*\[""").find(json) ?: return emptyList()
109-
val arrayStart = keyMatch.range.last + 1
110-
var depth = 1; var pos = arrayStart
111-
while (pos < json.length && depth > 0) {
112-
when (json[pos]) { '[' -> depth++; ']' -> depth-- }; pos++
113-
}
114-
if (depth != 0) return emptyList()
115-
val arrayContent = json.substring(arrayStart, pos - 1).trim()
116-
if (arrayContent.isEmpty()) return emptyList()
117-
return Regex(""""((?:[^"\\]|\\.)*)"""").findAll(arrayContent).map { it.groupValues[1] }.toList()
11892
}
11993

12094
private enum class LogLevel { DEBUG, INFO, WARN, ERROR }

0 commit comments

Comments
 (0)