spdlog-rs
spdlog-rs copied to clipboard
Implement `RuntimePattern`
This PR introduces RuntimePattern to implement the ability of building a patterns at runtime, this also allows us to implement serialization for loggers (#25) containing PatternFormatter in the future.
Basically, this PR did:
-
Split pattern template parser from crate
spdlog-macrosto a separate cratespdlog-internal. so that the runtime builder and compile-time builder can shared the same parser code.Creating a separate crate seems to be the only way to share code between lib crate and proc-macro crate, or maybe we could publish a generic crate for the pattern feature, which would allow more other crates to use it. @Lancern Are you interested in this? Since you are the author of the pattern feature, to publish such a crate I think you are the best person to do it if you are interested.
-
Implement
Clonefor allPatterntypes by usingdyn-clonecrate, as formatters are required to be clonable,Box<dyn Pattern>are also need to be clonable. -
Add
RuntimePatternandRuntimePatternBuilder. -
Documentation improvements.
Nice work. But I believe we need further discussion on this to avoid any potential security vulnerabilities. There are just too many severe security vulnerabilities in logging libraries.
While it is essential to be able to parse pattern template strings at runtime to support features like configuration files, is it necessary to allow the user to build the "pattern registry" at runtime? In the current design, the registry is basically a Vec<(String, Fn() -> Box<dyn Pattern>)> that is built at runtime by the user. In a worst case scenario, the adversary may be able to control both the pattern name and the pattern vtable, which effectively means arbitrary code execution.
Thus, I believe it is more appropriate to only allow building the pattern registry at compile time.
I agree with your security concerns, but I'm still hesitant to drop the custom pattern support for RuntimePattern.
So I decided to change the built-in patterns in the registry to a compile-time array (the type would be [(&'static str, Box<dyn Pattern>); N]), and also try to remove pattern-related unsafe code to rely on Rust's safety model to avoid potential vulnerabilities, such as self-ref for zero-copy (https://github.com/SpriteOvO/spdlog-rs/pull/45/files#diff-e601b4baf8558f5a6458c7a768b3b71303144c3c7e8645e5ff01b0de63b8bb1dR34-R40). The parser is completely based on nom, which describes itself as a "safe parser" in its project description. I did a quick check and it only uses a couple of unsafe operations fairly conservatively.
From the information so far, I'm still not sure if I should be for or against dropping custom patterns. However, deserializing from config files is not currently planned to support custom patterns, as the build will be done internally by spdlog-rs and there is no way for the user to pass custom patterns.
Well, custom pattern support itself is not the root cause of the security problem. The whole point is, we can provide custom pattern support, provided that the set of custom patterns provided by the user must be determined at compile time. For security's sake, we just cannot let the user build the set of custom patterns at runtime. The idea of custom patterns itself has nothing wrong.
As for implementation, maybe we can introduce a new macro named pattern_set! and use this macro to build pattern set during compile time. The syntax can be copied directly from the current pattern! macro. Here is a PoC:
let patterns = pattern_set!{
{$mypat} => MyPattern::default,
{$myotherpat} => MyOtherPattern::default,
};
The implementation may take some time. If you are in a hurry to land this PR, maybe we can just land first and then implement the custom pattern support in future PRs.
Well, custom pattern support itself is not the root cause of the security problem. The whole point is, we can provide custom pattern support, provided that the set of custom patterns provided by the user must be determined at compile time. For security's sake, we just cannot let the user build the set of custom patterns at runtime. The idea of custom patterns itself has nothing wrong.
Nice point, I see.
As for implementation, maybe we can introduce a new macro named
pattern_set!and use this macro to build pattern set during compile time. The syntax can be copied directly from the currentpattern!macro. Here is a PoC:let patterns = pattern_set!{ {$mypat} => MyPattern::default, {$myotherpat} => MyOtherPattern::default, };
Or maybe it would be better to just implement a new runtime_pattern! macro? The only difference with the pattern! macro for user is that the first parameter only accepts a Expr.
let pattern: RuntimePattern = runtime_pattern!(template_str, {$custom} => Custom::default);
The implementation may take some time. If you are in a hurry to land this PR, maybe we can just land first and then implement the custom pattern support in future PRs.
I plan to release v0.4.0 with the 2 new features RuntimePattern and Config. So RuntimePattern just needs to be done before Config PR is ready to be merged. There's no clear timeline at the moment, but there should be plenty of time, so we are not in a hurry to merge this PR :)
Or maybe it would be better to just implement a new
runtime_pattern!macro? The only difference with thepattern!macro for user is that the first parameter only accepts a Expr.let pattern: RuntimePattern = runtime_pattern!(template_str, {$custom} => Custom::default);
I think this is better. It is consistent with existing pattern! stuff.
I quickly go through the code in this PR and I found it a good starting point for implementing the runtime_pattern! macro. So I'm going to push new commits directly to this PR and land the feature here.
I believe what left for me to do is just to remove RuntimePatternBuilder from public interface and add the runtime_pattern! macro.
I have added the runtime_pattern macro. I found that I have to implement it as a procedural macro in the spdlog_macros crate because the normal macro by example cannot match the $ token we use in the custom pattern formatter syntax.
The documentation of the runtime_pattern macro is not finished, though.
Now the feature should be completed. Seems that I cannot request a review from the PR author through GitHub, so have a review @SpriteOvO .
@Lancern Thanks! I'm gonna review it in the next few days. And happy new year! 🎉
I removed the dependency self_cell in commit cb21cb00e6a5127ef8e6115a8f4e2684e5dc60ac. Instead, I manually created a self-reference through Box::{leak,from_raw}>, which introduces a unsafe operation. Would you mind reviewing it? @Lancern
The reason for using self-reference is that the Template parser consists of complex combinators and the input type are dependent. I tried replacing &str in it with Cow<'a, str> but failed.
Once that's fine, I think this PR might be ready to merge, I'll do some final checks. I apologize for the long delay.
@SpriteOvO I'll review it as soon as I get some free time before this weekend!
Cleared some doc and rebased to branch main-dev, let's merge it! @Lancern Many thanks for your co-work and review!