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.

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

Fix possible infinite loop in message template parser + better handling incorrect templates #2576

Merged
merged 1 commit into from
Feb 19, 2018

Conversation

304NotModified
Copy link
Member

@codecov
Copy link

codecov bot commented Feb 19, 2018

Codecov Report

Merging #2576 into master will increase coverage by <1%.
The diff coverage is 67%.

@@           Coverage Diff           @@
##           master   #2576    +/-   ##
=======================================
+ Coverage      81%     81%   +<1%     
=======================================
  Files         325     325            
  Lines       23908   23918    +10     
  Branches     3020    3022     +2     
=======================================
+ Hits        19385   19408    +23     
+ Misses       3707    3673    -34     
- Partials      816     837    +21

@304NotModified 304NotModified force-pushed the 304NotModified-patch-1 branch 2 times, most recently from 02c62f6 to d37f15d Compare February 19, 2018 15:14
fix unneeded index out of bound exception
@304NotModified
Copy link
Member Author

@snakefoot thanks for reviewing, I will react in this PR to make the discussions a bit more separated.

No need to use short (16 bit) since new processors prefer 32 bit and 64 bit variables.

This is a side effect hole.Index, which is already a 'short' (see

public readonly short Index;
)

Also no need to perform "safe" lookup, as invalid templates should trigger exceptions (just not unhandled exceptions)

What is the expected output of MessageTemplateParameters.Count with " {3} {4} {9} {8} {5} {6} {7}" ? It was 2, which was really unexpected for me. Also if we could make (partly) a good result, that really better then no result. e.g. in " {3} {4} {9} {8} {5} {6} {7}" we could maybe render 6 out of 7, which is better then 2 (and 0).

@304NotModified 304NotModified changed the title Fix isPositional Fix possible infinite loop in message template parser + better handling incorrect templates Feb 19, 2018
@snakefoot
Copy link
Contributor

snakefoot commented Feb 19, 2018 via email

@304NotModified
Copy link
Member Author

I prefer strict mode, but I can see Serilog has undocumented behavior when using invalid templates. Mixing strict with loose.

I prefer strict mode when:

  • it's a compile error (not run-time)
  • and/or it could be easily fixed (read: without recompile)
  • if the non-strict mode has undefined behavior.

@304NotModified 304NotModified merged commit 52dfff9 into master Feb 19, 2018
@304NotModified 304NotModified deleted the 304NotModified-patch-1 branch May 3, 2018 15:48
@snakefoot snakefoot modified the milestones: 4.5 rc6, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants