Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maybe a memory leak using static check found #5619

Open
yihong0618 opened this issue Feb 11, 2025 · 1 comment
Open

maybe a memory leak using static check found #5619

yihong0618 opened this issue Feb 11, 2025 · 1 comment

Comments

@yihong0618
Copy link
Contributor

I just used rapx to check opendal

about rapx: https://github.com/Artisan-Lab/RAPx

doc: https://artisan-lab.github.io/RAPx-Book/1-intro.html

using:

first install rapx as doc

cd opendal/core
# then 
cargo +nightly-2024-10-12 rapx -mleak

found two memroy leak(maybe) I am not sure these will cause memory leak issue so just leave the issue here

memory leak found by rapx

22:33:46|RAP|INFO|: Start analysis with RAP.
22:33:51|RAP|WARN|: Double free detected in function with_operation
warning: Memory Leak detected.
   --> src/types/error.rs:344:26
    |
339 | pub fn with_operation(mut self, operation: impl Into<&'static str>) -> Self {
340 |         if !self.operation.is_empty() {
341 |             self.context.push(("called", self.operation.to_string()));
342 |         }
343 |
344 |         self.operation = operation.into();
    |                          ---------------- Memory Leak Candidates.
345 |         self
346 |     }
    |
22:33:51|RAP|WARN|: Double free detected in function from_str
warning: Memory Leak detected.
   --> src/types/scheme.rs:414:36
    |
341 | fn from_str(s: &str) -> Result<Self, Self::Err> {
342 |         let s = s.to_lowercase();
343 |         match s.as_str() {
344 |             "aliyun_drive" => Ok(Scheme::AliyunDrive),
345 |             "atomicserver" => Ok(Scheme::Atomicserver),
346 |             "azblob" => Ok(Scheme::Azblob),
347 |             "alluxio" => Ok(Scheme::Alluxio),
348 |             // Notes:
349 |             //
350 |             // OpenDAL used to call `azdls` as `azdfs`, we keep it for backward compatibility.
351 |             // And abfs is widely used in hadoop ecosystem, keep it for easy to use.
352 |             "azdls" | "azdfs" | "abfs" => Ok(Scheme::Azdls),
353 |             "b2" => Ok(Scheme::B2),
354 |             "chainsafe" => Ok(Scheme::Chainsafe),
355 |             "cacache" => Ok(Scheme::Cacache),
356 |             "compfs" => Ok(Scheme::Compfs),
357 |             "cloudflare_kv" => Ok(Scheme::CloudflareKv),
358 |             "cos" => Ok(Scheme::Cos),
359 |             "d1" => Ok(Scheme::D1),
360 |             "dashmap" => Ok(Scheme::Dashmap),
361 |             "dropbox" => Ok(Scheme::Dropbox),
362 |             "etcd" => Ok(Scheme::Etcd),
363 |             "dbfs" => Ok(Scheme::Dbfs),
364 |             "fs" => Ok(Scheme::Fs),
365 |             "gcs" => Ok(Scheme::Gcs),
366 |             "gdrive" => Ok(Scheme::Gdrive),
367 |             "ghac" => Ok(Scheme::Ghac),
368 |             "gridfs" => Ok(Scheme::Gridfs),
369 |             "github" => Ok(Scheme::Github),
370 |             "hdfs" => Ok(Scheme::Hdfs),
371 |             "http" | "https" => Ok(Scheme::Http),
372 |             "huggingface" | "hf" => Ok(Scheme::Huggingface),
373 |             "ftp" | "ftps" => Ok(Scheme::Ftp),
374 |             "ipfs" | "ipns" => Ok(Scheme::Ipfs),
375 |             "ipmfs" => Ok(Scheme::Ipmfs),
376 |             "icloud" => Ok(Scheme::Icloud),
377 |             "koofr" => Ok(Scheme::Koofr),
378 |             "libsql" => Ok(Scheme::Libsql),
379 |             "memcached" => Ok(Scheme::Memcached),
380 |             "memory" => Ok(Scheme::Memory),
381 |             "mysql" => Ok(Scheme::Mysql),
382 |             "sqlite" => Ok(Scheme::Sqlite),
383 |             "mini_moka" => Ok(Scheme::MiniMoka),
384 |             "moka" => Ok(Scheme::Moka),
385 |             "monoiofs" => Ok(Scheme::Monoiofs),
386 |             "obs" => Ok(Scheme::Obs),
387 |             "onedrive" => Ok(Scheme::Onedrive),
388 |             "persy" => Ok(Scheme::Persy),
389 |             "postgresql" => Ok(Scheme::Postgresql),
390 |             "redb" => Ok(Scheme::Redb),
391 |             "redis" => Ok(Scheme::Redis),
392 |             "rocksdb" => Ok(Scheme::Rocksdb),
393 |             "s3" => Ok(Scheme::S3),
394 |             "seafile" => Ok(Scheme::Seafile),
395 |             "upyun" => Ok(Scheme::Upyun),
396 |             "yandex_disk" => Ok(Scheme::YandexDisk),
397 |             "pcloud" => Ok(Scheme::Pcloud),
398 |             "sftp" => Ok(Scheme::Sftp),
399 |             "sled" => Ok(Scheme::Sled),
400 |             "supabase" => Ok(Scheme::Supabase),
401 |             "swift" => Ok(Scheme::Swift),
402 |             "oss" => Ok(Scheme::Oss),
403 |             "vercel_artifacts" => Ok(Scheme::VercelArtifacts),
404 |             "vercel_blob" => Ok(Scheme::VercelBlob),
405 |             "webdav" => Ok(Scheme::Webdav),
406 |             "webhdfs" => Ok(Scheme::Webhdfs),
407 |             "tikv" => Ok(Scheme::Tikv),
408 |             "azfile" => Ok(Scheme::Azfile),
409 |             "mongodb" => Ok(Scheme::Mongodb),
410 |             "hdfs_native" => Ok(Scheme::HdfsNative),
411 |             "surrealdb" => Ok(Scheme::Surrealdb),
412 |             "lakefs" => Ok(Scheme::Lakefs),
413 |             "nebula_graph" => Ok(Scheme::NebulaGraph),
414 |             _ => Ok(Scheme::Custom(Box::leak(s.into_boxed_str()))),
    |                                    ----------------------------- Memory Leak Candidates.
415 |         }
416 |     }
    |
@Xuanwo
Copy link
Member

Xuanwo commented Feb 12, 2025

Thank you @yihong0618 for the report. I have checked those cases and am sure that they work as expected.

The operation itself is an enum, and each variant has its own static string. The conversion won't cause any memory leaks externally.

The Scheme::Custom is a bit tricky since it allows users to create a memory leak by repeatedly calling Scheme::from_str(uuid::Uuid::new_v4()). However, I personally think this is acceptable and ultimately up to the user's discretion. I will add some comments to this function to inform users about the potential issue—when they use this API to construct a Scheme for custom services, they will receive a static string internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants