[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.
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:
|
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;
asaveljevs The were two problems with the implemented approach:
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:
|