Fail to build if tasks may overflow #1890
Conversation
mkeeter
force-pushed
the
stack-overflow-check
branch
2 times, most recently
from
1387f12
to
b6ad2f7
Compare
mkeeter
force-pushed
the
stack-overflow-check
branch
from
3bb0c0d
to
4f306fc
Compare
| let detail = cs.insn_detail(instr).map_err(|e| { | ||
| anyhow!("could not get instruction details: {e}") | ||
| })?; | ||
| if detail.groups().iter().any(|g| { | ||
| g == &InsnGroupId(InsnGroupType::CS_GRP_CALL as u8) | ||
| }) { |
There was a problem hiding this comment.
i am obligated by myself to ask: is this why capstone in particular? :)
There was a problem hiding this comment.
Not specifically – I was using yaxpeax earlier and was fine looking for BL opcodes, but we're using enough weird instructions that I was hitting DecodeError::Incomplete in a bunch of tasks.
There was a problem hiding this comment.
huh! i'll have to take a look later.
There was a problem hiding this comment.
It should be easy enough to test by checking out one of the earlier commits, e.g. d7fc2eb uses yaxpeax
mkeeter
force-pushed
the
stack-overflow-check
branch
from
4f306fc
to
9bb0552
Compare
| @@ -1656,6 +1933,7 @@ fn build( | |||
| "-C link-arg=-z -C link-arg=common-page-size=0x20 \ | |||
| -C link-arg=-z -C link-arg=max-page-size=0x20 \ | |||
| -C llvm-args=--enable-machine-outliner=never \ | |||
| -Z emit-stack-sizes \ | |||
There was a problem hiding this comment.
do you know if there is any work tracking this not being nightly only? We still have a goal (dream?) of being able to build hubris with a stable toolchain.
There was a problem hiding this comment.
The path to stabilization appears rocky, unfortunately:
rust-lang/rust#54192
| @@ -1064,6 +1075,272 @@ fn build_task(cfg: &PackageConfig, name: &str) -> Result<()> { | |||
| .context(format!("failed to build {}", name)) | |||
| } | |||
|
|
|||
| fn task_can_overflow( | |||
There was a problem hiding this comment.
Are there any concerns about false positives / false negatives? I guess for a false positive we'd increase the stack size unnecessarily and for a false negative, well, we're back to our old methods.
There was a problem hiding this comment.
Added more comments in 9471544 about the possibility of false positives (possible but unlikely) and negatives (possible if using dynamic dispatch).
mkeeter
force-pushed
the
stack-overflow-check
branch
from
9bb0552
to
f960090
Compare
(details to come, but opening as a draft to see if any of the CI-checked builds fail)