Skip to content

Commit

Permalink
fix: delete_func_by_name in ModuleImports/ModuleExports (#251)
Browse files Browse the repository at this point in the history
The functions `ModuleExports::delete_func_by_name` and
`ModuleImports::delete_func_by_name` were broken due to the
possibility of the *same* function ID being used in *multiple*
exports.

Since the code was getting the "first" function ID it found with a
matching name (or name & module in the case of imports), deleting
that export could mean deleting a *different* export ahead of time.

A worked example:
- export "A" w/ ID 24 exists, tied to fn 48
- export "B" w/ ID 99 exists, tied to fn 48

Things are fine when we use the export name "A" to get the relevant
function ID, but when we use that ID to *look up* an export, it's
unclear which one we will get -- searching by function ID would
surface *either* existing export. The existing functions don't
actually account for this possibility at all (they just happily return
the "first" one).

This commit fixes this by changing the logic of `delete_func_by_name`
to use *only* the export name, and do manual checking for whether the
export/import is a function.

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
  • Loading branch information
vados-cosmonic authored Oct 23, 2023
1 parent 45ca488 commit bf253d9
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 17 deletions.
24 changes: 14 additions & 10 deletions src/module/exports.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Exported items in a wasm module.
use anyhow::Context;
use anyhow::{bail, Context};

use crate::emit::{Emit, EmitContext};
use crate::parse::IndicesToIds;
Expand Down Expand Up @@ -138,15 +138,19 @@ impl ModuleExports {

/// Delete an exported function by name from this module.
pub fn delete_func_by_name(&mut self, name: impl AsRef<str>) -> Result<()> {
let fid = self.get_func_by_name(name.as_ref()).context(format!(
"failed to find exported func with name [{}]",
name.as_ref()
))?;
self.delete(
self.get_exported_func(fid)
.with_context(|| format!("failed to find exported func with ID [{fid:?}]"))?
.id(),
);
let export = self
.iter()
.find(|e| e.name == name.as_ref())
.with_context(|| {
format!("failed to find exported func with name [{}]", name.as_ref())
})?;

if let ExportItem::Function(_) = export.item {
self.delete(export.id());
} else {
bail!("export [{}] is not an exported function", name.as_ref());
}

Ok(())
}
}
Expand Down
21 changes: 14 additions & 7 deletions src/module/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,16 +137,23 @@ impl ModuleImports {
module: impl AsRef<str>,
name: impl AsRef<str>,
) -> Result<()> {
let fid = self
.get_func_by_name(module, name.as_ref())
let import = self
.iter()
.find(|e| e.module == module.as_ref() && e.name == name.as_ref())
.with_context(|| {
format!("failed to find imported func with name [{}]", name.as_ref())
})?;
self.delete(
self.get_imported_func(fid)
.with_context(|| format!("failed to find imported func with ID [{fid:?}]"))?
.id(),
);

if let ImportKind::Function(_) = import.kind {
self.delete(import.id());
} else {
bail!(
"import [{}] in module [{}] is not an imported function",
name.as_ref(),
module.as_ref()
);
}

Ok(())
}
}
Expand Down

0 comments on commit bf253d9

Please sign in to comment.