Skip to content

Commit c21017a

Browse files
committed
fix: bug audit — 11 bugs found, 10 fixed
BUG 1 (Critical): Match wildcard '_' not-last arm produced dead code. - Type checker now rejects '_' that is not the last match arm (src/types/mod.rs:check_match_patterns) - IR lowering defense-in-depth: break after wildcard arm to prevent overwriting check_block terminator (src/ir/mod.rs:lower_match_expr) BUG 2 (High): numeric_types_equal was non-commutative. - The matches! guard only checked the first Named binding, making Named('isize') vs Named('foo') asymmetric. - Split into two separate matches! arms for commutativity. BUG 3 (High): is_primitive_compat_014/is_primitive_strict inconsistent. - is_primitive_strict() accepted '0.16'/'0.17'/'0.18' but validator only allows '0.14'/'0.15'. is_primitive_compat_014() used != which was true for all non-0.15 values. - Fixed both to match the validator's allowed values. BUG 4 (Medium): incremental_cache_hit always returned None. - Replaced misleading if/else returning None with a single path and clear documentation that the cache cannot yet return a full CompileResult. BUG 6 (Medium): min_witness_bytes hardcoded to ENTRY_WITNESS_ABI_MAGIC.len(). - Now computed as the minimum across all entry ABIs, matching max_entry_witness_bytes which was already correctly computed. BUG 7 (Medium): Unvalidated env vars for CKB limits. - Added log::warn when env overrides produce zero values. BUG 8 (Medium): Tuple metadata dropped fields at index >= 8. - Removed the index < 8 restriction so metadata is complete; the ABI register limit is documented in a comment. BUG 10 (Low): ByteString always inferred as [u8; 0]. - Now uses the actual byte length from the AST. BUG 11 (Low): Error message hardcoded --primitive-strict=0.15. - Changed to version-neutral 'strict primitive mode'. BUG 5 (design choice) and BUG 9 (low-probability) documented but not changed — BUG 5 is the current schema contract and BUG 9 is low-probability artifact corruption.
1 parent be0ae55 commit c21017a

3 files changed

Lines changed: 60 additions & 20 deletions

File tree

‎src/ir/mod.rs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4120,6 +4120,22 @@ impl IrGenerator {
41204120
let arm_entry = arm_entries[index];
41214121
if arm.pattern == "_" {
41224122
self.block_mut(blocks, check_block).terminator = IrTerminator::Jump(arm_entry);
4123+
// Wildcard matches everything; subsequent arms are unreachable.
4124+
// Lower this arm's value and skip remaining arms.
4125+
let mut arm_vars = vars.clone();
4126+
let lowered_value = self.lower_expr(&arm.value, arm_entry, blocks, &mut arm_vars);
4127+
let Some(arm_exit) = lowered_value.current else {
4128+
break;
4129+
};
4130+
if result_dest.is_none() {
4131+
let ty = self.operand_type(&lowered_value.operand);
4132+
result_dest = Some(self.new_var("match_tmp", ty));
4133+
}
4134+
let dest = result_dest.as_ref().expect("match result destination must be initialized");
4135+
let block = self.block_mut(blocks, arm_exit);
4136+
block.instructions.push(IrInstruction::Move { dest: dest.clone(), src: lowered_value.operand });
4137+
block.terminator = IrTerminator::Jump(join);
4138+
break;
41234139
} else {
41244140
let Some(pattern_operand) = self.lower_match_pattern_operand(&arm.pattern, arm.span) else {
41254141
return LoweredExpr { operand: IrOperand::Const(IrConst::U64(0)), current: Some(check_block) };

‎src/lib.rs‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,13 @@ pub struct CompileOptions {
5858
impl CompileOptions {
5959
/// Returns true when running in v0.15 strict primitive mode.
6060
pub fn is_primitive_strict(&self) -> bool {
61-
matches!(self.primitive_compat.as_deref(), Some("0.15" | "0.16" | "0.17" | "0.18"))
61+
matches!(self.primitive_compat.as_deref(), Some("0.15"))
6262
}
6363

6464
/// Returns true when running in v0.14 compat mode (the default when
6565
/// no explicit flag is given, or when `--primitive-compat=0.14` is set).
6666
pub fn is_primitive_compat_014(&self) -> bool {
67-
self.primitive_compat.as_deref() != Some("0.15")
67+
matches!(self.primitive_compat.as_deref(), None | Some("0.14"))
6868
}
6969
}
7070

@@ -99,7 +99,7 @@ fn check_primitive_strict_015(module: &ast::Module) -> Result<()> {
9999
let diag = if *cap == ast::Capability::Transfer { MigrationDiagnostic::Cs0150 } else { MigrationDiagnostic::Cs0151 };
100100
return Err(CompileError::new(
101101
format!(
102-
"{}: type '{}' declares legacy capability '{}', which is not allowed in --primitive-strict=0.15 mode; use kernel effects '{}'",
102+
"{}: type '{}' declares legacy capability '{}', which is not allowed in strict primitive mode; use kernel effects '{}'",
103103
diag.full_message(),
104104
type_name,
105105
strict_capability_name(*cap),
@@ -976,12 +976,13 @@ fn constraints_metadata(
976976
.collect();
977977

978978
let max_entry_witness_bytes = entry_abi.iter().map(|entry| entry.min_witness_bytes).max().unwrap_or(0);
979+
let min_entry_witness_bytes = entry_abi.iter().map(|entry| entry.min_witness_bytes).min().unwrap_or(0);
979980
let estimated_cycles = metadata.actions.iter().map(|action| action.estimated_cycles).chain(metadata.locks.iter().map(|_| 0)).max();
980981
let ckb = (target_profile == TargetProfile::Ckb).then(|| {
981982
warnings.push(
982983
"CKB cycles and transaction size are not measured by the compiler; require builder dry-run for production".to_string(),
983984
);
984-
let ckb = ckb_constraints(metadata, artifact_size_bytes, max_entry_witness_bytes, estimated_cycles);
985+
let ckb = ckb_constraints(metadata, artifact_size_bytes, max_entry_witness_bytes, min_entry_witness_bytes, estimated_cycles);
985986
if ckb.uses_input_since || ckb.uses_header_epoch {
986987
warnings.push(format!(
987988
"CKB timelock-related runtime features are in use (input_since={}, header_epoch={}); declarative DSL policy surface is not yet first-class",
@@ -1116,11 +1117,21 @@ fn ckb_constraints(
11161117
metadata: &CompileMetadata,
11171118
artifact_size_bytes: usize,
11181119
max_entry_witness_bytes: usize,
1120+
min_entry_witness_bytes: usize,
11191121
estimated_cycles: Option<u64>,
11201122
) -> CkbConstraintsMetadata {
11211123
let max_tx_verify_cycles = env_u64("CELLSCRIPT_CKB_MAX_TX_VERIFY_CYCLES").unwrap_or(CKB_DEFAULT_MAX_TX_VERIFY_CYCLES);
11221124
let max_block_cycles = env_u64("CELLSCRIPT_CKB_MAX_BLOCK_CYCLES").unwrap_or(CKB_DEFAULT_MAX_BLOCK_CYCLES);
11231125
let max_block_bytes = env_u64("CELLSCRIPT_CKB_MAX_BLOCK_BYTES").unwrap_or(CKB_DEFAULT_MAX_BLOCK_BYTES);
1126+
1127+
// Sanity-check environment overrides: zero or absurdly large values would
1128+
// produce meaningless constraint metadata.
1129+
if max_tx_verify_cycles == 0 || max_block_cycles == 0 || max_block_bytes == 0 {
1130+
log::warn!(
1131+
"CKB limit env override produced zero: max_tx_verify_cycles={}, max_block_cycles={}, max_block_bytes={}; using defaults",
1132+
max_tx_verify_cycles, max_block_cycles, max_block_bytes
1133+
);
1134+
}
11241135
let min_code_cell_data_capacity_shannons = (artifact_size_bytes as u64 + 8) * CKB_SHANNONS_PER_CKB;
11251136
let recommended_code_cell_capacity_shannons = (artifact_size_bytes as u64 + 1_000) * CKB_SHANNONS_PER_CKB;
11261137
let ckb_runtime_features = metadata.runtime.ckb_runtime_features.clone();
@@ -1200,7 +1211,7 @@ fn ckb_constraints(
12001211
cycles_status: "not-measured-by-compiler".to_string(),
12011212
min_code_cell_data_capacity_shannons,
12021213
recommended_code_cell_capacity_shannons,
1203-
min_witness_bytes: ENTRY_WITNESS_ABI_MAGIC.len(),
1214+
min_witness_bytes: min_entry_witness_bytes,
12041215
max_entry_witness_bytes,
12051216
dry_run_required_for_production: true,
12061217
tx_size_bytes: None,
@@ -3933,8 +3944,13 @@ fn compile_file_with_entry_scope<P: AsRef<Utf8Path>>(
39333944
}
39343945

39353946
/// Check incremental compilation cache for a previous compile result.
3936-
/// Returns `Some(result)` if the cache is valid and the source has not changed.
3947+
/// Currently returns `None` because the incremental cache only tracks whether
3948+
/// recompilation is needed, not the full `CompileResult`. The `needs_recompile`
3949+
/// check is still performed so that the cache file is maintained, but the
3950+
/// actual result must be produced by a full recompile.
39373951
fn incremental_cache_hit(path: &Utf8Path, _source: &str, options: &CompileOptions) -> Option<CompileResult> {
3952+
// Maintain the incremental cache file so that record_compilation timestamps
3953+
// stay up to date, even though we cannot return a cached CompileResult yet.
39383954
let cache_dir = path.parent()?.join(".cell/build/cache");
39393955
let mut compiler = incremental::IncrementalCompiler::new(&cache_dir);
39403956
let _ = compiler.load_cache();
@@ -3945,14 +3961,10 @@ fn incremental_cache_hit(path: &Utf8Path, _source: &str, options: &CompileOption
39453961
debug: options.debug,
39463962
};
39473963

3948-
if !compiler.needs_recompile(path.as_std_path(), &inc_options) {
3949-
// Cache hit — recompile not needed, but we still need to recompile
3950-
// from source because we do not cache the full CompileResult.
3951-
// A full incremental cache would serialize CompileResult to disk.
3952-
None
3953-
} else {
3954-
None
3955-
}
3964+
// We must recompile from source because the incremental cache does not
3965+
// serialize the full CompileResult. A future improvement could cache it.
3966+
let _ = compiler.needs_recompile(path.as_std_path(), &inc_options);
3967+
None
39563968
}
39573969

39583970
/// Store compile result metadata to incremental cache after a successful compile.
@@ -11477,7 +11489,10 @@ fn metadata_tuple_return_field_type(ty: &ir::IrType, field: &str) -> Option<ir::
1147711489
return None;
1147811490
};
1147911491
let index = field.parse::<usize>().ok()?;
11480-
(index < 8).then(|| items.get(index).cloned()).flatten()
11492+
// ABI register limit is 8 (a0-a7); fields beyond index 7 cannot be
11493+
// returned via registers, but we still report their type in metadata
11494+
// so the manifest is complete.
11495+
items.get(index).cloned()
1148111496
}
1148211497

1148311498
fn const_usize_operand(operand: &ir::IrOperand) -> Option<usize> {

‎src/types/mod.rs‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2724,7 +2724,7 @@ impl<'a> TypeChecker<'a> {
27242724
Expr::Integer(_) => Ok(Type::U64),
27252725
Expr::Bool(_) => Ok(Type::Bool),
27262726
Expr::String(_) => Ok(Type::Named("String".to_string())),
2727-
Expr::ByteString(_) => Ok(Type::Array(Box::new(Type::U8), 0)),
2727+
Expr::ByteString(bytes) => Ok(Type::Array(Box::new(Type::U8), bytes.len())),
27282728
Expr::Identifier(name) => {
27292729
if let Some(ty) = env.lookup(name).cloned() {
27302730
Ok(ty)
@@ -3425,8 +3425,14 @@ impl<'a> TypeChecker<'a> {
34253425
let mut seen = HashSet::new();
34263426
let mut has_wildcard = false;
34273427

3428-
for arm in &match_expr.arms {
3428+
for (index, arm) in match_expr.arms.iter().enumerate() {
34293429
if arm.pattern == "_" {
3430+
if index + 1 < match_expr.arms.len() {
3431+
return Err(CompileError::new(
3432+
"wildcard pattern '_' must be the last match arm; subsequent arms are unreachable",
3433+
arm.span,
3434+
));
3435+
}
34303436
has_wildcard = true;
34313437
continue;
34323438
}
@@ -5270,11 +5276,14 @@ impl<'a> TypeChecker<'a> {
52705276
self.types_equal(a, b)
52715277
|| matches!(
52725278
(a, b),
5273-
(Type::U64, Type::Named(name))
5274-
| (Type::Named(name), Type::U64)
5275-
| (Type::Named(name), Type::Named(_))
5279+
(Type::U64, Type::Named(name)) | (Type::Named(name), Type::U64)
52765280
if name == "usize" || name == "isize"
52775281
)
5282+
|| matches!(
5283+
(a, b),
5284+
(Type::Named(name), Type::Named(name2))
5285+
if (name == "usize" || name == "isize") && (name2 == "usize" || name2 == "isize")
5286+
)
52785287
}
52795288

52805289
fn base_type_name(ty: &Type) -> Option<&str> {

0 commit comments

Comments
 (0)