Fix memory violation when download callback not provided

This commit is contained in:
Caelan Sayler
2024-12-20 22:36:53 +00:00
committed by Caelan
parent 037344537d
commit ee8ff2bbc4
6 changed files with 74 additions and 115 deletions

View File

@@ -362,7 +362,7 @@ private:
IUpdateSource(vpkc_update_source_t* pSource) : m_pSource(pSource) {}
vpkc_update_source_t* m_pSource = 0;
public:
~IUpdateSource() {
virtual ~IUpdateSource() {
vpkc_free_source(m_pSource);
}
IUpdateSource() {
@@ -447,7 +447,8 @@ public:
* @param options Optional extra configuration for update manager.
* @param locator Override the default locator configuration (usually used for testing / mocks).
*/
UpdateManager(std::unique_ptr<IUpdateSource> pUpdateSource, const UpdateOptions* options = nullptr, const VelopackLocatorConfig* locator = nullptr) {
template <typename T, typename = std::enable_if_t<std::is_base_of_v<IUpdateSource, T>>>
UpdateManager(std::unique_ptr<T> pUpdateSource, const UpdateOptions* options = nullptr, const VelopackLocatorConfig* locator = nullptr) {
vpkc_update_options_t vpkc_options;
vpkc_update_options_t* pOptions = nullptr;
if (options != nullptr) {
@@ -462,7 +463,7 @@ public:
pLocator = &vpkc_locator;
}
m_pUpdateSource.swap(pUpdateSource);
m_pUpdateSource = std::unique_ptr<IUpdateSource>(static_cast<IUpdateSource*>(pUpdateSource.release()));
vpkc_update_source_t* pSource = m_pUpdateSource->m_pSource;
if (!vpkc_new_update_manager_with_source(pSource, pOptions, pLocator, &m_pManager)) {
throw_last_error();

View File

@@ -14,7 +14,7 @@ use velopack::{bundle::Manifest, sources::UpdateSource, Error, VelopackAsset, Ve
lazy_static! {
static ref PROGRESS_CALLBACKS: RwLock<HashMap<size_t, Sender<i16>>> = RwLock::new(HashMap::new());
static ref PROGRESS_ID: AtomicUsize = AtomicUsize::new(0);
static ref PROGRESS_ID: AtomicUsize = AtomicUsize::new(1);
}
pub fn report_csource_progress(callback_id: size_t, progress: i16) {
@@ -39,38 +39,53 @@ impl UpdateSource for CCallbackUpdateSource {
fn get_release_feed(&self, channel: &str, _: &Manifest) -> Result<VelopackAssetFeed, Error> {
let releases_name = format!("releases.{}.json", channel);
let releases_name_cstr = CString::new(releases_name).unwrap();
let json_cstr_ptr = (self.cb_get_release_feed)(self.p_user_data, releases_name_cstr.as_ptr());
let json = c_to_string_opt(json_cstr_ptr)
.ok_or(Error::Generic("User vpkc_release_feed_delegate_t returned a null pointer instead of an asset feed".to_string()))?;
(self.cb_free_release_feed)(self.p_user_data, json_cstr_ptr); // Free the C string returned by the callback
let feed: VelopackAssetFeed = serde_json::from_str(&json)?;
Ok(feed)
if let Some(cb_get_release_feed) = self.cb_get_release_feed {
let json_cstr_ptr = (cb_get_release_feed)(self.p_user_data, releases_name_cstr.as_ptr());
let json = c_to_string_opt(json_cstr_ptr)
.ok_or(Error::Generic("User vpkc_release_feed_delegate_t returned a null pointer instead of an asset feed".to_string()))?;
if let Some(cb_free_release_feed) = self.cb_free_release_feed {
(cb_free_release_feed)(self.p_user_data, json_cstr_ptr); // Free the C string returned by the callback
} else {
log::error!("User vpkc_release_feed_delegate_t is null, this may be a memory leak");
}
let feed: VelopackAssetFeed = serde_json::from_str(&json)?;
Ok(feed)
} else {
Err(Error::Generic("User vpkc_release_feed_delegate_t is null".to_string()))
}
}
fn download_release_entry(&self, asset: &VelopackAsset, local_file: &str, progress_sender: Option<Sender<i16>>) -> Result<(), Error> {
let local_file_cstr = CString::new(local_file).unwrap();
let asset_ptr: *mut vpkc_asset_t = std::ptr::null_mut();
unsafe { allocate_velopackasset(asset.clone(), asset_ptr) };
if let Some(cb_download_release_entry) = self.cb_download_release_entry {
let local_file_cstr = CString::new(local_file).unwrap();
let mut asset_c: vpkc_asset_t = unsafe { std::mem::zeroed() };
let asset_ptr: *mut vpkc_asset_t = &mut asset_c as *mut vpkc_asset_t;
unsafe { allocate_velopackasset(asset.clone(), asset_ptr) };
let progress_callback_id = PROGRESS_ID.fetch_add(1, Ordering::SeqCst);
if let Some(progress_sender) = &progress_sender {
let _ = progress_sender.send(0);
PROGRESS_CALLBACKS.write().unwrap().insert(progress_callback_id, progress_sender.clone());
let progress_callback_id = PROGRESS_ID.fetch_add(1, Ordering::SeqCst);
if let Some(progress_sender) = &progress_sender {
let _ = progress_sender.send(0);
PROGRESS_CALLBACKS.write().unwrap().insert(progress_callback_id, progress_sender.clone());
}
let success = (cb_download_release_entry)(self.p_user_data, asset_ptr, local_file_cstr.as_ptr(), progress_callback_id);
unsafe { free_velopackasset(asset_ptr) };
if let Some(sender) = PROGRESS_CALLBACKS.write().unwrap().remove(&progress_callback_id) {
let _ = sender.send(100);
}
if !success {
return Err(Error::Generic("User vpkc_download_asset_delegate_t returned false to indicate download failed".to_owned()));
}
Ok(())
} else {
Err(Error::Generic("User vpkc_download_asset_delegate_t is null".to_string()))
}
let success = (self.cb_download_release_entry)(self.p_user_data, asset_ptr, local_file_cstr.as_ptr(), progress_callback_id);
unsafe { free_velopackasset(asset_ptr) };
if let Some(sender) = PROGRESS_CALLBACKS.write().unwrap().remove(&progress_callback_id) {
let _ = sender.send(100);
}
if !success {
return Err(Error::Generic("User vpkc_download_asset_delegate_t returned false to indicate download failed".to_owned()));
}
Ok(())
}
fn clone_boxed(&self) -> Box<dyn UpdateSource> {

View File

@@ -48,18 +48,15 @@ pub extern "C" fn vpkc_new_source_custom_callback(
cb_download_entry: vpkc_download_asset_delegate_t,
p_user_data: *mut c_void,
) -> *mut vpkc_update_source_t {
let cb_release_feed = cb_release_feed.to_option();
let cb_download_entry = cb_download_entry.to_option();
let cb_free_release_feed = cb_free_release_feed.to_option();
if cb_release_feed.is_none() || cb_download_entry.is_none() || cb_free_release_feed.is_none() {
return ptr::null_mut();
}
let source = CCallbackUpdateSource {
p_user_data,
cb_get_release_feed: cb_release_feed.unwrap(),
cb_download_release_entry: cb_download_entry.unwrap(),
cb_free_release_feed: cb_free_release_feed.unwrap(),
cb_get_release_feed: cb_release_feed,
cb_download_release_entry: cb_download_entry,
cb_free_release_feed: cb_free_release_feed,
};
UpdateSourceRawPtr::new(Box::new(source))
@@ -216,7 +213,6 @@ pub extern "C" fn vpkc_download_updates(
None => bail!("pManager must not be null"),
};
let cb_progress = cb_progress.to_option();
let update = c_to_updateinfo_opt(p_update).ok_or(anyhow!("pUpdate must not be null"))?;
if let Some(cb_progress) = cb_progress {
@@ -407,7 +403,6 @@ pub extern "C" fn vpkc_app_set_locator(p_locator: *mut vpkc_locator_config_t) {
/// Only supported on windows; On other operating systems, this will never be called.
#[no_mangle]
pub extern "C" fn vpkc_app_set_hook_after_install(cb_after_install: vpkc_hook_callback_t) {
let cb_after_install = cb_after_install.to_option();
update_app_options(|opt| {
opt.install_hook = cb_after_install;
});
@@ -419,7 +414,6 @@ pub extern "C" fn vpkc_app_set_hook_after_install(cb_after_install: vpkc_hook_ca
/// Only supported on windows; On other operating systems, this will never be called.
#[no_mangle]
pub extern "C" fn vpkc_app_set_hook_before_uninstall(cb_before_uninstall: vpkc_hook_callback_t) {
let cb_before_uninstall = cb_before_uninstall.to_option();
update_app_options(|opt| {
opt.uninstall_hook = cb_before_uninstall;
});
@@ -431,7 +425,6 @@ pub extern "C" fn vpkc_app_set_hook_before_uninstall(cb_before_uninstall: vpkc_h
/// Only supported on windows; On other operating systems, this will never be called.
#[no_mangle]
pub extern "C" fn vpkc_app_set_hook_before_update(cb_before_update: vpkc_hook_callback_t) {
let cb_before_update = cb_before_update.to_option();
update_app_options(|opt| {
opt.obsolete_hook = cb_before_update;
});
@@ -443,7 +436,6 @@ pub extern "C" fn vpkc_app_set_hook_before_update(cb_before_update: vpkc_hook_ca
/// Only supported on windows; On other operating systems, this will never be called.
#[no_mangle]
pub extern "C" fn vpkc_app_set_hook_after_update(cb_after_update: vpkc_hook_callback_t) {
let cb_after_update = cb_after_update.to_option();
update_app_options(|opt| {
opt.update_hook = cb_after_update;
});
@@ -452,7 +444,6 @@ pub extern "C" fn vpkc_app_set_hook_after_update(cb_after_update: vpkc_hook_call
/// This hook is triggered when the application is started for the first time after installation.
#[no_mangle]
pub extern "C" fn vpkc_app_set_hook_first_run(cb_first_run: vpkc_hook_callback_t) {
let cb_first_run = cb_first_run.to_option();
update_app_options(|opt| {
opt.firstrun_hook = cb_first_run;
});
@@ -461,7 +452,6 @@ pub extern "C" fn vpkc_app_set_hook_first_run(cb_first_run: vpkc_hook_callback_t
/// This hook is triggered when the application is restarted by Velopack after installing updates.
#[no_mangle]
pub extern "C" fn vpkc_app_set_hook_restarted(cb_restarted: vpkc_hook_callback_t) {
let cb_restarted = cb_restarted.to_option();
update_app_options(|opt| {
opt.restarted_hook = cb_restarted;
});
@@ -477,10 +467,5 @@ pub extern "C" fn vpkc_get_last_error(psz_error: *mut c_char, c_error: size_t) -
/// Set a custom log callback. This will be called for all log messages generated by the Velopack library.
#[no_mangle]
pub extern "C" fn vpkc_set_logger(cb_log: vpkc_log_callback_t, p_user_data: *mut c_void) {
let cb_log = cb_log.to_option();
if let Some(cb_log) = cb_log {
set_log_callback(cb_log, p_user_data);
} else {
clear_log_callback();
}
set_log_callback(cb_log, p_user_data);
}

View File

@@ -1,46 +1,6 @@
use crate::types::*;
use velopack::{sources::UpdateSource, UpdateManager};
pub trait CallbackExt: Sized {
fn to_option(self) -> Option<Self>;
}
impl CallbackExt for vpkc_progress_callback_t {
fn to_option(self) -> Option<Self> {
unsafe { std::mem::transmute::<Self, Option<Self>>(self) }
}
}
impl CallbackExt for vpkc_log_callback_t {
fn to_option(self) -> Option<Self> {
unsafe { std::mem::transmute::<Self, Option<Self>>(self) }
}
}
impl CallbackExt for vpkc_hook_callback_t {
fn to_option(self) -> Option<Self> {
unsafe { std::mem::transmute::<Self, Option<Self>>(self) }
}
}
impl CallbackExt for vpkc_release_feed_delegate_t {
fn to_option(self) -> Option<Self> {
unsafe { std::mem::transmute::<Self, Option<Self>>(self) }
}
}
impl CallbackExt for vpkc_download_asset_delegate_t {
fn to_option(self) -> Option<Self> {
unsafe { std::mem::transmute::<Self, Option<Self>>(self) }
}
}
impl CallbackExt for vpkc_free_release_feed_t {
fn to_option(self) -> Option<Self> {
unsafe { std::mem::transmute::<Self, Option<Self>>(self) }
}
}
pub trait RawPtrExt<'a, T>: Sized {
fn to_opaque_ref(self) -> Option<&'a T>;
}

View File

@@ -8,12 +8,12 @@ use crate::types::*;
#[derive(Debug, Default, Clone)]
pub struct AppOptions {
pub install_hook: Option<vpkc_hook_callback_t>,
pub update_hook: Option<vpkc_hook_callback_t>,
pub obsolete_hook: Option<vpkc_hook_callback_t>,
pub uninstall_hook: Option<vpkc_hook_callback_t>,
pub firstrun_hook: Option<vpkc_hook_callback_t>,
pub restarted_hook: Option<vpkc_hook_callback_t>,
pub install_hook: vpkc_hook_callback_t,
pub update_hook: vpkc_hook_callback_t,
pub obsolete_hook: vpkc_hook_callback_t,
pub uninstall_hook: vpkc_hook_callback_t,
pub firstrun_hook: vpkc_hook_callback_t,
pub restarted_hook: vpkc_hook_callback_t,
pub auto_apply: Option<bool>,
pub args: Option<Vec<String>>,
pub locator: Option<VelopackLocatorConfig>,
@@ -21,7 +21,7 @@ pub struct AppOptions {
lazy_static::lazy_static! {
static ref LAST_ERROR: RwLock<String> = RwLock::new(String::new());
static ref LOG_CALLBACK: Mutex<Option<(vpkc_log_callback_t, usize)>> = Mutex::new(None);
static ref LOG_CALLBACK: Mutex<(vpkc_log_callback_t, usize)> = Mutex::new((None, 0));
pub static ref VELOPACK_APP: RwLock<AppOptions> = RwLock::new(Default::default());
}
@@ -71,17 +71,13 @@ pub fn set_log_callback(callback: vpkc_log_callback_t, user_data: *mut c_void) {
log::set_max_level(log::LevelFilter::Trace);
let mut log_callback = LOG_CALLBACK.lock().unwrap();
*log_callback = Some((callback, user_data as usize));
}
pub fn clear_log_callback() {
let mut log_callback = LOG_CALLBACK.lock().unwrap();
*log_callback = None;
*log_callback = (callback, user_data as usize);
}
pub fn log_message(level: &str, message: &str) {
let log_callback = LOG_CALLBACK.lock().unwrap();
if let Some((callback, user_data)) = *log_callback {
let (callback, user_data) = *log_callback;
if let Some(callback) = callback {
let c_level = CString::new(level).unwrap();
let c_message = CString::new(message).unwrap();
callback(user_data as *mut c_void, c_level.as_ptr(), c_message.as_ptr());

View File

@@ -19,30 +19,32 @@ pub type vpkc_update_manager_t = c_void;
pub type vpkc_update_source_t = c_void;
/// Progress callback function.
pub type vpkc_progress_callback_t = extern "C" fn(p_user_data: *mut c_void, progress: size_t);
pub type vpkc_progress_callback_t = Option<extern "C" fn(p_user_data: *mut c_void, progress: size_t)>;
/// Log callback function.
pub type vpkc_log_callback_t = extern "C" fn(p_user_data: *mut c_void, psz_level: *const c_char, psz_message: *const c_char);
pub type vpkc_log_callback_t = Option<extern "C" fn(p_user_data: *mut c_void, psz_level: *const c_char, psz_message: *const c_char)>;
/// VelopackApp startup hook callback function.
pub type vpkc_hook_callback_t = extern "C" fn(p_user_data: *mut c_void, psz_app_version: *const c_char);
pub type vpkc_hook_callback_t = Option<extern "C" fn(p_user_data: *mut c_void, psz_app_version: *const c_char)>;
/// User delegate for to fetch a release feed. This function should return the raw JSON string of the release.json feed.
pub type vpkc_release_feed_delegate_t = extern "C" fn(p_user_data: *mut c_void, psz_releases_name: *const c_char) -> *mut c_char;
pub type vpkc_release_feed_delegate_t = Option<extern "C" fn(p_user_data: *mut c_void, psz_releases_name: *const c_char) -> *mut c_char>;
/// User delegate for freeing a release feed. This function should free the feed string returned by `vpkc_release_feed_delegate_t`.
pub type vpkc_free_release_feed_t = extern "C" fn(p_user_data: *mut c_void, psz_feed: *mut c_char);
pub type vpkc_free_release_feed_t = Option<extern "C" fn(p_user_data: *mut c_void, psz_feed: *mut c_char)>;
/// User delegate for downloading an asset file. This function is expected to download the provided asset
/// to the provided local file path. Througout, you can use the progress callback to write progress reports.
/// The function should return true if the download was successful, false otherwise.
/// Progress
pub type vpkc_download_asset_delegate_t = extern "C" fn(
p_user_data: *mut c_void,
p_asset: *const vpkc_asset_t,
psz_local_path: *const c_char,
progress_callback_id: size_t,
) -> bool;
pub type vpkc_download_asset_delegate_t = Option<
extern "C" fn(
p_user_data: *mut c_void,
p_asset: *const vpkc_asset_t,
psz_local_path: *const c_char,
progress_callback_id: size_t,
) -> bool,
>;
pub fn c_to_string_opt(psz: *const c_char) -> Option<String> {
if psz.is_null() {