-
Notifications
You must be signed in to change notification settings - Fork 229
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
ini_parser: Remove inline comment parsing on ini handler for values #4175
base: master
Are you sure you want to change the base?
Conversation
@@ -199,11 +199,6 @@ int ini_parse_stream(ini_reader reader, void* stream, ini_handler handler, | |||
*end = '\0'; | |||
name = rstrip(start); | |||
value = end + 1; | |||
#if INI_ALLOW_INLINE_COMMENTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/prusa3d/Prusa-Firmware-Buddy/blob/master/lib/inih/README.md#compile-time-options
Inline comments: By default, inih allows inline comments with the
;
character. To disable, add-DINI_ALLOW_INLINE_COMMENTS=0
. You can also specify which character(s) start an inline comment usingINI_INLINE_COMMENT_PREFIXES
.Start-of-line comments: By default, inih allows both
;
and#
to start a comment at the beginning of a line. You can override this by changingINI_START_COMMENT_PREFIXES
.
Did you consider simply tweaking either or both of these options, vs eliding this single dependent block of code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not want to remove inline comments. Instead we just want to remove inline comments where inline comments shouldn't happen : On parsing values. That's why only this define is removed, but INI_ALLOW_INLINE_COMMENTS stays defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, sounds like a bug in inih then? Or, is this an effect of the quoted string modification? I think it's not, given the comments from the issue this addresses, but unsure.
Perhaps this is fixed in more recent inih? https://github.com/benhoyt/inih/commits/master/ini.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, it isn't fixed in the latest master, it's an issue by design of the parser. It would require to be rewritten and it would add more complexity. However this is only a temporary fix to solve current user issues until the inih is completely removed and replaced by a different parser library.
On rare occations when parsing the prusa_printer_settings.ini, a value after = is being interpreted as inline comment, although it shouldn't, leading to an invalid null termination of a value value string.
For example:
field = #value
The parser does only treat an inline comment as comment if a whitespace is followed by a hashtag char. In this case, field and value aren't treated as one, but as two lines. it is setting a nulltermination at "=" for parsing the field and then parses the value as " #value", assuming that an inline comment has occured.
By removing this inline comment check when parsing the value fields, we can rule out this problem.
Related to issue #4162, can relate to #2325 as well.