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.

Sign up for GitHub

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

Jump to bottom

[FIRRTL] Crash in LowerClasses related to StringMap buckets #7327

Closed
mikeurbach opened this issue Jul 16, 2024 · 1 comment · Fixed by #7328
Closed

[FIRRTL] Crash in LowerClasses related to StringMap buckets #7327

mikeurbach opened this issue Jul 16, 2024 · 1 comment · Fixed by #7328

Comments

@mikeurbach
Copy link
Contributor

I just observed this in an internal design:


[2024-07-16T19:06:09.356Z] Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
[2024-07-16T19:06:09.356Z] 0  firtool         0x00000000006ddac7 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 39
[2024-07-16T19:06:09.356Z] 1  firtool         0x00000000006db87e llvm::sys::RunSignalHandlers() + 238
[2024-07-16T19:06:09.356Z] 2  firtool         0x00000000006de3ef
[2024-07-16T19:06:09.356Z] 3  libpthread.so.0 0x00007f794e6f0cf0
[2024-07-16T19:06:09.356Z] 4  firtool         0x0000000000699053 llvm::StringMapImpl::LookupBucketFor(llvm::StringRef, unsigned int) + 307
[2024-07-16T19:06:09.356Z] 5  firtool         0x000000000075bddc
[2024-07-16T19:06:09.356Z] 6  firtool         0x000000000075ed5e
[2024-07-16T19:06:09.356Z] 7  firtool         0x0000000000e04076 circt::firrtl::getOrAddInnerSym(mlir::MLIRContext*, circt::hw::InnerSymAttr, unsigned long, llvm::function_ref<circt::hw::InnerSymbolNamespace& ()>) + 198
[2024-07-16T19:06:09.356Z] 8  firtool         0x0000000000e044a3 circt::firrtl::getOrAddInnerSym(circt::hw::InnerSymTarget const&, llvm::function_ref<circt::hw::InnerSymbolNamespace& ()>) + 131
[2024-07-16T19:06:09.356Z] 9  firtool         0x0000000000e04654 circt::firrtl::getOrAddInnerSym(circt::hw::InnerSymTarget const&, llvm::function_ref<circt::hw::InnerSymbolNamespace& (circt::firrtl::FModuleLike)>) + 292
[2024-07-16T19:06:09.356Z] 10 firtool         0x0000000000e04707 circt::firrtl::getInnerRefTo(circt::hw::InnerSymTarget const&, llvm::function_ref<circt::hw::InnerSymbolNamespace& (circt::firrtl::FModuleLike)>) + 151
[2024-07-16T19:06:09.356Z] 11 firtool         0x000000000089ae09
[2024-07-16T19:06:09.356Z] 12 firtool         0x0000000000bbf6bb circt::firrtl::AnnotationSet::removeAnnotations(llvm::function_ref<bool (circt::firrtl::Annotation)>) + 91
[2024-07-16T19:06:09.356Z] 13 firtool         0x0000000000895aca
[2024-07-16T19:06:09.356Z] 14 firtool         0x00000000011c2b98 mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) + 632
[2024-07-16T19:06:09.356Z] 15 firtool         0x00000000011c34b7 mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) + 327
[2024-07-16T19:06:09.356Z] 16 firtool         0x00000000011c4d0c mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool) + 2668
[2024-07-16T19:06:09.356Z] 17 firtool         0x00000000011c2d88 mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) + 1128
[2024-07-16T19:06:09.356Z] 18 firtool         0x00000000011c5b5a mlir::PassManager::run(mlir::Operation*) + 1338
[2024-07-16T19:06:09.356Z] 19 firtool         0x000000000066df40
[2024-07-16T19:06:09.356Z] 20 firtool         0x000000000066cb8e
[2024-07-16T19:06:09.356Z] 21 firtool         0x0000000000668cfb
[2024-07-16T19:06:09.356Z] 22 firtool         0x00000000006671f8 main + 392
[2024-07-16T19:06:09.356Z] 23 libc.so.6       0x00007f794d4f1d85 __libc_start_main + 229
[2024-07-16T19:06:09.356Z] 24 firtool         0x0000000000663f6e _start + 46

Unfortunately it's a release build, and there's not much info. I have tried reproducing and haven't been able to catch it, so it seems non-deterministic?

Perhaps this is related to ce012c0?

That specific code was removed when this was made sequential: f4920aa. But maybe the loop to pre-allocate namespaces is required still?
@mikeurbach
Copy link
Contributor Author

Looks like we just need to re-land ce012c0. I wasn't able to reproduce the crash, but I was able to catch a UAF with ASAN. With the above patch re-added, the UAF is gone.

mikeurbach added a commit that referenced this issue Jul 16, 2024
@mikeurbach
Reland "[FIRRTL][LowerClass] Pre-allocate namespaces before caputring…
766bc37 
… refs (#7102)".

This was originally added to avoid a use-after-free. However, it got
removed as part of f4920aa. It seems this is still required, even when
we process PathTrackers sequentially. I confirmed there was a
use-after-free without this change, which is fixed after adding this
back.

Closes #7327.
@mikeurbach mikeurbach mentioned this issue Jul 16, 2024
Merged
mikeurbach added a commit that referenced this issue Jul 16, 2024
@mikeurbach
Reland "[FIRRTL][LowerClass] Pre-allocate namespaces before capturing…
de80eb4 
… refs (#7102)". (#7328)

This was originally added to avoid a use-after-free. However, it got
removed as part of f4920aa. It seems this is still required, even when
we process PathTrackers sequentially. I confirmed there was a
use-after-free without this change, which is fixed after adding this
back.

Closes #7327.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reland "[FIRRTL][LowerClass] Pre-allocate namespaces before capturing refs (#7102)". llvm/circt
1 participant
@mikeurbach