Implement RuntimePattern

[SpriteOvO]

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:

1. Split pattern template parser from crate spdlog-macros to a separate crate spdlog-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.

2. Implement Clone for all Pattern types by using dyn-clone crate, as formatters are required to be clonable, Box<dyn Pattern> are also need to be clonable.

3. Add RuntimePattern and RuntimePatternBuilder (Removed, added a runtime_pattern! macro instead).

4. Documentation improvements.

[Lancern]

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.

[Lancern]

Generally LGTM except for some small problems listed. And we need further discussion on the security problem I just commented in the PR discussion thread before this PR can actually land.

[Lancern]

Why does Pattern requires Clone after the change?

[SpriteOvO]

Formatter requires Clone to be implemented since 78e359c, so that we can implement set_formatter for combined sink

spdlog-rs/spdlog/src/sink/async_sink/async_pool_sink.rs

Lines 86 to 92 in 0a2489c

	/// For [`AsyncPoolSink`], the function performs the same call to all
	/// internal sinks.
	fn set_formatter(&self, formatter: Box<dyn Formatter>) {
	for sink in &self.backend.sinks {
	sink.set_formatter(formatter.clone_box())
	}
	}

Before this PR, all patterns are actual types, present in the generic P of PatternFormatter<P>, also all patterns implemented Clone, so we didn't explicitly bound Clone for trait Pattern.

But with runtime pattern, we now have Box<dyn Pattern> which doesn't implement Clone, if we impl Clone for Box<dyn Pattern>, we have to bound Clone on trait Pattern, and DynClone make it possible with object-safe.

[SpriteOvO]

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.

[Lancern]

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.

[SpriteOvO]

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 current pattern! 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);

[SpriteOvO]

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 :)

[Lancern]

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);

I think this is better. It is consistent with existing pattern! stuff.

[Lancern]

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.

[Lancern]

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.

[Lancern]

Now the feature should be completed. Seems that I cannot request a review from the PR author through GitHub, so have a review @SpriteOvO .

[SpriteOvO]

@Lancern Thanks! I'm gonna review it in the next few days. And happy new year! 🎉

[SpriteOvO]

I have left some comments, and feel free to discuss about them. If there is no objection, I will apply these changes and finish the rest of the work of the PR.

[SpriteOvO]

Mmm, I think we should pass an array of custom patterns into the registry, and storing it in a separated field of PatternRegistry. Otherwise there is not much difference compare to the previous struct RuntimePatternBuilder approach, it's still building a set of custom patterns at runtime.

[SpriteOvO]

Mmm, I think we should pass an array of custom patterns into the registry, and storing it in a separated field of PatternRegistry. Otherwise there is not much difference compare to the previous struct RuntimePatternBuilder approach, it's still building a set of custom patterns at runtime.

The idea is actually more complicated than I expected, so let's just keep it that way.

[SpriteOvO]

I removed the dependency self_cell in commit cb21cb0. 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.

[Lancern]

@SpriteOvO I'll review it as soon as I get some free time before this weekend!

[Lancern]

Mostly good, just a single NIT in the inline comments and it's ready to go!

[Lancern]

I believe this function can be inlined into synthesis.

[SpriteOvO]

build_patterns will recursively call itself with an style_range_seen = true argument, while synthesis calls it initially with style_range_seen = false.

Keeping this way we can hide the initial value of this argument instead of the caller being responsible for specifying it.

[SpriteOvO]

Cleared some doc and rebased to branch main-dev, let's merge it! @Lancern Many thanks for your co-work and review!