[ZBX-9776] Incorrect reading of long lines from configuration file Created: 2015 Aug 13  Updated: 2017 May 30  Resolved: 2015 Oct 16

Status: Closed
Project: ZABBIX BUGS AND ISSUES
Component/s: Agent (G), Proxy (P), Server (S)
Affects Version/s: 2.5.0
Fix Version/s: 3.0.0alpha3

Type: Incident report Priority: Trivial
Reporter: Andris Mednis Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: configfiles, validation
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

If a line in configuration file is longer than 2047 bytes (MAX_STRING_LEN -1), the function __parse_cfg_file() reads only the first 2047 bytes, the remaining part is read as the next line. Result depends on the contents of the remaining part - it could be counted as unknown parameter, produce a "not following "parameter=value" notation" error message or can even pass as a valid configuration parameter.



 Comments   
Comment by Alexander Vladishev [ 2015 Aug 20 ]

An appropriate error message should be generated.

Comment by Viktors Tjarve [ 2015 Aug 24 ]

Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-9776

Comment by Andris Zeila [ 2015 Sep 01 ]

(1) If the last line in configuration file does not end with \n character and line length length is sizeof(line) - 1 then the

ZBX_CFG_EOL_CHAR != line[line_length - 1] && !feof(file)

check will trigger 'line too long' error.

Probably the easiest solution would be adding last_line flag. Set this flag by checking if the last character in buffer is not \n and generate error if the reading continues after this flag is set (of course check for the flag before setting it).

Or another solution - use larger buffer (MAX_STRING_LEN + 1 bytes) and then generate error if the data length is MAX_STRING_LEN bytes. Although this way the last line could be larger by 1 byte than other lines (other lines will always have at least \n character at the end) it will not break anything.

viktors.tjarve RESOLVED in r55702: Solution where larger buffer (MAX_STRING_LEN + 1 bytes) is used and then generate error if the data length is MAX_STRING_LEN bytes.

wiper CLOSED

Comment by Andris Zeila [ 2015 Sep 23 ]

(2) The error messages are written in old style. In non-debug message we should use "" instead of [] to enclose values. For example instead of cannot open config file [file]: error reason message we should use cannot open config file "file": error reason message.

Would be nice to update all configuration file parsing error messages in cfg.c to the proper style.

Also I think it would be enough just to print a line number in the case the line was too long. Otherwise we are guaranteed with 2047 character spam in error message.

asaveljevs While at it, please consider investigating lines like the following in cfg.c:

zbx_error("%s: glob pattern should be the last component of the path\n", glob);

It seems that the newline at the end is not necessary.

viktors.tjarve RESOLVED in r55714.

  • Old style non-debug error message format changed to current style - [] changed to "".
  • In error line_too_long the line print removed. Line number print remains.
  • New line character removed from messages in cfg.c. When error message is handled by zbx_error new line character is added automatically.

wiper CLOSED. I changed the 'too long' error message slightly, please review r55731.

viktors.tjarve I agree with change in r55731

Comment by Andris Zeila [ 2015 Sep 24 ]

Successfully tested

Comment by Viktors Tjarve [ 2015 Sep 24 ]

Released in:

  • pre-3.0.0alpha3 r55734
Comment by Aleksandrs Saveljevs [ 2015 Sep 24 ]

(3) There is the following code:

+	/* only 2048 character (inc. '\n' or '\0') single lines supported in the config file */
+	if(MAX_STRING_LEN <= strlen(line))
+		goto line_too_long;

Stylistically, there should be a space between "if" and "(". But also the comment is a bit unclear - what does it mean for a line to include '\0' character? Our buffer includes '\0' character at the end, but the line does not. If we have a line with 2046 characters, then "\r\n", then such a line will be considered too long. If, however, we are on Unix and we have 2046 characters followed by "\n", it will be accepted. Was this intended? Also, the comment should probably refer to bytes, rather than characters.

viktors.tjarve The code changed to:

			/* check if line length exceeds limit (max. 2047 bytes) */
			s = line;
			zbx_rtrim(s, "\r\n");
			if (MAX_STRING_LEN <= strlen(s))
				goto line_too_long;
  • Code stile fixed.
  • Line length is checked after unprintable character trim. Now all lines including last line can be maximum 2047 bytes long. Also comment is made more clear.

asaveljevs The were two problems with the implemented approach:

  1. an unnecessary "s" variable was introduced;
  2. zbx_rtrim() was done two times and the second zbx_rtrim() tried to trim almost the same set of characters as the first one.

Commit r55776 tries to make a bit more efficient by removing the "s" and the first zbx_rtrim().

An alternative approach would use strcspn() function:

Index: src/libs/zbxconf/cfg.c
===================================================================
--- src/libs/zbxconf/cfg.c      (revision 55770)
+++ src/libs/zbxconf/cfg.c      (working copy)
@@ -373,7 +373,7 @@
 
        FILE            *file;
        int             i, lineno, param_valid;
-       char            line[MAX_STRING_LEN + 1], *parameter, *value, *s;
+       char            line[MAX_STRING_LEN + 1], *parameter, *value;
        zbx_uint64_t    var;
 #ifdef _WINDOWS
        wchar_t         *wcfg_file;
@@ -400,9 +400,7 @@
                for (lineno = 1; NULL != fgets(line, sizeof(line), file); lineno++)
                {
                        /* check if line length exceeds limit (max. 2047 bytes) */
-                       s = line;
-                       zbx_rtrim(s, "\r\n");
-                       if (MAX_STRING_LEN <= strlen(s))
+                       if (MAX_STRING_LEN <= strcspn(line, "\r\n"))
                                goto line_too_long;
 
                        zbx_ltrim(line, ZBX_CFG_LTRIM_CHARS);

However, it would be a bit less effiecient than r55776. RESOLVED.

asaveljevs As mentioned by wiper, if we read until "\r" on Windows, the code in r55776 considers this line to be good and processes the line with just "\n" in the next iteration. REOPENED.

asaveljevs RESOLVED in r55785 and r55789.

viktors.tjarve Successfully tested. CLOSED

Comment by Viktors Tjarve [ 2015 Oct 02 ]

Released in:

  • pre-3.0.0alpha3 r55919
Generated at Thu Mar 28 20:43:08 EET 2024 using Jira 9.12.4#9120004-sha1:625303b708afdb767e17cb2838290c41888e9ff0.