Discussion:
[Linuxptp-devel] [PATCHv2 0/4] More strict input value validation
Ken ICHIKAWA
2013-06-03 07:57:14 UTC
Permalink
Now, ptp4l and phc2sys accept option values that are obviously invalid.
For example of logging level:
-l abcd
-l 6a
-l -1
-l 8

This patchset provides more strict option value validation for ptp4l and
phc2sys.

change from v1:
- Extend to config file parsing
- get_ranged_int,uint,double return separate error values so that caller can print the reason of the error

Ken ICHIKAWA (4):
config: Add support for more strict config value validation
config: Apply config value validation to logging_level option
util: Add common procedure to get argument values for ptp4l and
phc2sys
ptp4l and phc2sys: Get argument values with strict error checking

config.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
config.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
makefile | 4 +-
phc2sys.c | 46 +++++++++++++++++++++++++++++++++---------
ptp4l.c | 4 ++-
util.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
util.h | 38 +++++++++++++++++++++++++++++++++++
7 files changed, 241 insertions(+), 22 deletions(-)
Ken ICHIKAWA
2013-06-03 08:03:01 UTC
Permalink
This patch adds functions to get int, uint, double value from string
with error checking and range specification.
These functions don't allow overflow and outside of the range values.

In addition, it adds parser_result cases "MALFORMED" and "OUT_OF_RANGE" to make
reason of parse error more clear.

Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
config.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
config.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 104 insertions(+), 6 deletions(-)

diff --git a/config.c b/config.c
index 734bd36..cc0e1bc 100644
--- a/config.c
+++ b/config.c
@@ -17,8 +17,10 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
#include <ctype.h>
+#include <errno.h>
#include "config.h"
#include "ether.h"
#include "print.h"
@@ -30,12 +32,6 @@ enum config_section {
UNKNOWN_SECTION,
};

-enum parser_result {
- PARSED_OK,
- NOT_PARSED,
- BAD_VALUE,
-};
-
static enum parser_result parse_section_line(char *s, enum config_section *section)
{
if (!strcasecmp(s, "[global]")) {
@@ -512,6 +508,14 @@ int config_read(char *name, struct config *cfg)
fprintf(stderr, "%s is a bad value for option %s at line %d\n",
value, option, line_num);
goto parse_error;
+ case MALFORMED:
+ fprintf(stderr, "%s is a malformed value for option %s at line %d\n",
+ value, option, line_num);
+ goto parse_error;
+ case OUT_OF_RANGE:
+ fprintf(stderr, "%s is an out of range value for option %s at line %d\n",
+ value, option, line_num);
+ goto parse_error;
}

break;
@@ -562,3 +566,45 @@ int config_create_interface(char *name, struct config *cfg)

return i;
}
+
+enum parser_result get_ranged_int(const char *str_val, int *result, int min, int max)
+{
+ long parsed_val;
+ char *endptr = NULL;
+ errno = 0;
+ parsed_val = strtol(str_val, &endptr, 0);
+ if (*endptr != '\0' || endptr == str_val)
+ return MALFORMED;
+ if (errno == ERANGE || parsed_val < min || parsed_val > max)
+ return OUT_OF_RANGE;
+ *result = parsed_val;
+ return PARSED_OK;
+}
+
+enum parser_result get_ranged_uint(const char *str_val, unsigned int *result, unsigned int min, unsigned int max)
+{
+ unsigned long parsed_val;
+ char *endptr = NULL;
+ errno = 0;
+ parsed_val = strtoul(str_val, &endptr, 0);
+ if (*endptr != '\0' || endptr == str_val)
+ return MALFORMED;
+ if (errno == ERANGE || parsed_val < min || parsed_val > max)
+ return OUT_OF_RANGE;
+ *result = parsed_val;
+ return PARSED_OK;
+}
+
+enum parser_result get_ranged_double(const char *str_val, double *result, double min, double max)
+{
+ double parsed_val;
+ char *endptr = NULL;
+ errno = 0;
+ parsed_val = strtod(str_val, &endptr);
+ if (*endptr != '\0' || endptr == str_val)
+ return MALFORMED;
+ if (errno == ERANGE || parsed_val < min || parsed_val > max)
+ return OUT_OF_RANGE;
+ *result = parsed_val;
+ return PARSED_OK;
+}
diff --git a/config.h b/config.h
index 901b50a..02eaeb1 100644
--- a/config.h
+++ b/config.h
@@ -46,6 +46,14 @@ struct interface {
#define CFG_IGNORE_USE_SYSLOG (1 << 5)
#define CFG_IGNORE_VERBOSE (1 << 6)

+enum parser_result {
+ PARSED_OK,
+ NOT_PARSED,
+ BAD_VALUE,
+ MALFORMED,
+ OUT_OF_RANGE,
+};
+
struct config {
/* configuration override */
int cfg_ignore;
@@ -82,4 +90,48 @@ struct config {
int config_read(char *name, struct config *cfg);
int config_create_interface(char *name, struct config *cfg);

+/**
+ * Get an integer value from string with error checking and range
+ * specification.
+ *
+ * @param str_val String which contains an integer value.
+ * @param result Parsed value is stored in here.
+ * @param min Lower limit. Return OUT_OF_RANGE if parsed value
+ * is less than min.
+ * @param max Upper Limit. Return OUT_OF_RANGE if parsed value
+ * is bigger than max.
+ * @return PARSED_OK on success, MALFORMED if str_val is malformed,
+ * OUT_OF_RANGE if str_val is out of range.
+ */
+enum parser_result get_ranged_int(const char *str_val, int *result, int min, int max);
+
+/**
+ * Get an unsigned integer value from string with error checking and range
+ * specification.
+ *
+ * @param str_val String which contains an unsigned integer value.
+ * @param result Parsed value is stored in here.
+ * @param min Lower limit. Return OUT_OF_RANGE if parsed value
+ * is less than min.
+ * @param max Upper Limit. Return OUT_OF_RANGE if parsed value
+ * is bigger than max.
+ * @return PARSED_OK on success, MALFORMED if str_val is malformed,
+ * OUT_OF_RANGE if str_val is out of range.
+ */
+enum parser_result get_ranged_uint(const char *str_val, unsigned int *result, unsigned int min, unsigned int max);
+
+/**
+ * Get a double value from string with error checking and range
+ * specification.
+ *
+ * @param str_val String which contains a double value.
+ * @param result Parsed value is stored in here.
+ * @param min Lower limit. Return OUT_OF_RANGE if parsed value
+ * is less than min.
+ * @param max Upper Limit. Return OUT_OF_RANGE if parsed value
+ * is bigger than max.
+ * @return PARSED_OK on success, MALFORMED if str_val is malformed,
+ * OUT_OF_RANGE if str_val is out of range.
+ */
+enum parser_result get_ranged_double(const char *str_val, double *result, double min, double max);
#endif
--
1.7.1
Ken ICHIKAWA
2013-06-03 08:03:25 UTC
Permalink
# I'll apply to other options too if this patchset accepted.

Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
config.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index cc0e1bc..66cdce5 100644
--- a/config.c
+++ b/config.c
@@ -311,9 +311,10 @@ static enum parser_result parse_global_setting(const char *option,
*cfg->udp6_scope = u8;

} else if (!strcmp(option, "logging_level")) {
- if (1 != sscanf(value, "%d", &val) ||
- val < PRINT_LEVEL_MIN || val > PRINT_LEVEL_MAX)
- return BAD_VALUE;
+ r = get_ranged_int(value, &val,
+ PRINT_LEVEL_MIN, PRINT_LEVEL_MAX);
+ if (r != PARSED_OK)
+ return r;
if (!(cfg_ignore & CFG_IGNORE_PRINT_LEVEL)) {
cfg->print_level = val;
}
--
1.7.1
Ken ICHIKAWA
2013-06-03 08:03:49 UTC
Permalink
Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
makefile | 4 ++--
util.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
util.h | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 94 insertions(+), 2 deletions(-)

diff --git a/makefile b/makefile
index 058bdb2..6cdb852 100644
--- a/makefile
+++ b/makefile
@@ -49,10 +49,10 @@ all: $(PRG)

ptp4l: $(OBJ)

-pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
+pmc: config.o msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
udp6.o uds.o util.o version.o

-phc2sys: clockadj.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o servo.o \
+phc2sys: clockadj.o config.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o servo.o \
raw.o sk.o stats.o sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o \
version.o

diff --git a/util.c b/util.c
index 55ec49f..c8aa9c1 100644
--- a/util.c
+++ b/util.c
@@ -190,3 +190,57 @@ int leap_second_status(uint64_t ts, int leap_set, int *leap, int *utc_offset)

return leap_status;
}
+
+int get_arg_val_i(int op, const char *optarg, int *val, int min, int max)
+{
+ enum parser_result r;
+ r = get_ranged_int(optarg, val, min, max);
+ if (r == MALFORMED) {
+ fprintf(stderr,
+ "-%c: %s is a malformed value\n", op, optarg);
+ return -1;
+ }
+ if (r == OUT_OF_RANGE) {
+ fprintf(stderr,
+ "-%c: %s is out of range. Must be in the range %d to %d\n",
+ op, optarg, min, max);
+ return -1;
+ }
+ return 0;
+}
+
+int get_arg_val_ui(int op, const char *optarg, unsigned int *val, unsigned int min, unsigned int max)
+{
+ enum parser_result r;
+ r = get_ranged_uint(optarg, val, min, max);
+ if (r == MALFORMED) {
+ fprintf(stderr,
+ "-%c: %s is a malformed value\n", op, optarg);
+ return -1;
+ }
+ if (r == OUT_OF_RANGE) {
+ fprintf(stderr,
+ "-%c: %s is out of range. Must be in the range %u to %u\n",
+ op, optarg, min, max);
+ return -1;
+ }
+ return 0;
+}
+
+int get_arg_val_d(int op, const char *optarg, double *val, double min, double max)
+{
+ enum parser_result r;
+ r = get_ranged_double(optarg, val, min, max);
+ if (r == MALFORMED) {
+ fprintf(stderr,
+ "-%c: %s is a malformed value\n", op, optarg);
+ return -1;
+ }
+ if (r == OUT_OF_RANGE) {
+ fprintf(stderr,
+ "-%c: %s is out of range. Must be in the range %e to %e\n",
+ op, optarg, min, max);
+ return -1;
+ }
+ return 0;
+}
diff --git a/util.h b/util.h
index d2734d1..d58ca6a 100644
--- a/util.h
+++ b/util.h
@@ -20,6 +20,7 @@
#ifndef HAVE_UTIL_H
#define HAVE_UTIL_H

+#include "config.h"
#include "ddt.h"

/**
@@ -114,4 +115,41 @@ int is_utc_ambiguous(uint64_t ts);
* inserted, -1 if leap second will be deleted.
*/
int leap_second_status(uint64_t ts, int leap_set, int *leap, int *utc_offset);
+
+/**
+ * Common procedure to get an int value from argument for ptp4l and phc2sys.
+ *
+ * @param op Character code of an option.
+ * @param optarg Option argument string.
+ * @param val Parsed value is stored in here.
+ * @param min Lower limit. Return -1 if parsed value is less than min.
+ * @param max Upper limit. Return -1 if parsed value is bigger than max.
+ * @return 0 on success, -1 if some error occurs.
+ */
+int get_arg_val_i(int op, const char *optarg, int *val, int min, int max);
+
+/**
+ * Common procedure to get an unsigned int value from argument for ptp4l
+ * and phc2sys.
+ *
+ * @param op Character code of an option.
+ * @param optarg Option argument string.
+ * @param val Parsed value is stored in here.
+ * @param min Lower limit. Return -1 if parsed value is less than min.
+ * @param max Upper limit. Return -1 if parsed value is bigger than max.
+ * @return 0 on success, -1 if some error occurs.
+ */
+int get_arg_val_ui(int op, const char *optarg, unsigned int *val, unsigned int min, unsigned int max);
+
+/**
+ * Common procedure to get a double value from argument for ptp4l and phc2sys.
+ *
+ * @param op Character code of an option.
+ * @param optarg Option argument string.
+ * @param val Parsed value is stored in here.
+ * @param min Lower limit. Return -1 if parsed value is less than min.
+ * @param max Upper limit. Return -1 if parsed value is bigger than max.
+ * @return 0 on success, -1 if some error occurs.
+ */
+int get_arg_val_d(int op, const char *optarg, double *val, double min, double max);
#endif
--
1.7.1
Richard Cochran
2013-06-03 13:55:27 UTC
Permalink
Post by Ken ICHIKAWA
diff --git a/makefile b/makefile
index 058bdb2..6cdb852 100644
--- a/makefile
+++ b/makefile
@@ -49,10 +49,10 @@ all: $(PRG)
ptp4l: $(OBJ)
-pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
+pmc: config.o msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
udp6.o uds.o util.o version.o
-phc2sys: clockadj.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o servo.o \
+phc2sys: clockadj.o config.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o servo.o \
raw.o sk.o stats.o sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o \
version.o
Can we have all of the new helper functions in util.c?

It makes more sense to me that way, and then you don't have to change
the makefile.

Thanks,
Richard
Keller, Jacob E
2013-06-03 19:55:32 UTC
Permalink
-----Original Message-----
Sent: Monday, June 03, 2013 6:55 AM
To: Ken ICHIKAWA
Subject: Re: [Linuxptp-devel] [PATCHv2 3/4] util: Add common
procedure to get argument values for ptp4l and phc2sys
Post by Ken ICHIKAWA
diff --git a/makefile b/makefile
index 058bdb2..6cdb852 100644
--- a/makefile
+++ b/makefile
@@ -49,10 +49,10 @@ all: $(PRG)
ptp4l: $(OBJ)
-pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o
udp.o \
Post by Ken ICHIKAWA
+pmc: config.o msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o
transport.o udp.o \
Post by Ken ICHIKAWA
udp6.o uds.o util.o version.o
-phc2sys: clockadj.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o
servo.o \
Post by Ken ICHIKAWA
+phc2sys: clockadj.o config.o msg.o phc.o phc2sys.o pi.o
pmc_common.o print.o servo.o \
Post by Ken ICHIKAWA
raw.o sk.o stats.o sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o \
version.o
Can we have all of the new helper functions in util.c?
It makes more sense to me that way, and then you don't have to change
the makefile.
Thanks,
Richard
I agree on this also.

- Jake
Ken ICHIKAWA
2013-06-03 23:56:36 UTC
Permalink
Post by Richard Cochran
Post by Ken ICHIKAWA
diff --git a/makefile b/makefile
index 058bdb2..6cdb852 100644
--- a/makefile
+++ b/makefile
@@ -49,10 +49,10 @@ all: $(PRG)
ptp4l: $(OBJ)
-pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
+pmc: config.o msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
udp6.o uds.o util.o version.o
-phc2sys: clockadj.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o servo.o \
+phc2sys: clockadj.o config.o msg.o phc.o phc2sys.o pi.o pmc_common.o print.o servo.o \
raw.o sk.o stats.o sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o \
version.o
Can we have all of the new helper functions in util.c?
It makes more sense to me that way, and then you don't have to change
the makefile.
OK, I'll send v3 with the fix
Ken ICHIKAWA
Post by Richard Cochran
Thanks,
Richard
Ken ICHIKAWA
2013-06-03 08:04:13 UTC
Permalink
Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
phc2sys.c | 46 ++++++++++++++++++++++++++++++++++++----------
ptp4l.c | 4 +++-
2 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 1dbe2bc..3fa75bb 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -29,6 +29,8 @@
#include <sys/types.h>
#include <unistd.h>
#include <inttypes.h>
+#include <limits.h>
+#include <float.h>

#include <linux/pps.h>
#include <linux/ptp_clock.h>
@@ -576,7 +578,7 @@ int main(int argc, char *argv[])
int c, domain_number = 0, phc_readings = 5, pps_fd = -1;
int max_ppb, r, wait_sync = 0, forced_sync_offset = 0;
int print_level = LOG_INFO, use_syslog = 1, verbose = 0;
- double ppb, phc_interval = 1.0;
+ double ppb, phc_interval = 1.0, phc_rate;
struct timespec phc_interval_tp;
struct clock dst_clock = {
.clkid = CLOCK_REALTIME,
@@ -611,39 +613,63 @@ int main(int argc, char *argv[])
src = clock_open(optarg);
break;
case 'P':
- configured_pi_kp = atof(optarg);
+ if (get_arg_val_d(c, optarg, &configured_pi_kp,
+ 0.0, DBL_MAX))
+ return -1;
break;
case 'I':
- configured_pi_ki = atof(optarg);
+ if (get_arg_val_d(c, optarg, &configured_pi_ki,
+ 0.0, DBL_MAX))
+ return -1;
break;
case 'S':
- configured_pi_offset = atof(optarg);
+ if (get_arg_val_d(c, optarg, &configured_pi_offset,
+ 0.0, DBL_MAX))
+ return -1;
break;
case 'R':
- phc_interval = 1.0 / atof(optarg);
+ if (get_arg_val_d(c, optarg, &phc_rate, 0.0, DBL_MAX))
+ return -1;
+ phc_interval = 1.0 / phc_rate;
+ /* phc_interval will be assigned to a time_t variable. */
+ /* check if that occurs overflow. */
+ if ((sizeof(time_t) == 8 && phc_interval > INT64_MAX) ||
+ (sizeof(time_t) == 4 && phc_interval > INT32_MAX)) {
+ fprintf(stderr,
+ "-R: %s is too small\n", optarg);
+ return -1;
+ }
break;
case 'N':
- phc_readings = atoi(optarg);
+ if (get_arg_val_i(c, optarg, &phc_readings, 1, INT_MAX))
+ return -1;
break;
case 'O':
- dst_clock.sync_offset = atoi(optarg);
+ if (get_arg_val_i(c, optarg, &dst_clock.sync_offset,
+ INT_MIN, INT_MAX))
+ return -1;
dst_clock.sync_offset_direction = -1;
forced_sync_offset = 1;
break;
case 'u':
- dst_clock.stats_max_count = atoi(optarg);
+ if (get_arg_val_ui(c, optarg, &dst_clock.stats_max_count,
+ 0, UINT_MAX))
+ return -1;
break;
case 'w':
wait_sync = 1;
break;
case 'n':
- domain_number = atoi(optarg);
+ if (get_arg_val_i(c, optarg, &domain_number, 0, 255))
+ return -1;
break;
case 'x':
dst_clock.kernel_leap = 0;
break;
case 'l':
- print_level = atoi(optarg);
+ if (get_arg_val_i(c, optarg, &print_level,
+ PRINT_LEVEL_MIN, PRINT_LEVEL_MAX))
+ return -1;
break;
case 'm':
verbose = 1;
diff --git a/ptp4l.c b/ptp4l.c
index ecaf9ed..8ad58bf 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -237,7 +237,9 @@ int main(int argc, char *argv[])
*cfg_ignore |= CFG_IGNORE_SLAVEONLY;
break;
case 'l':
- cfg_settings.print_level = atoi(optarg);
+ if (get_arg_val_i(c, optarg, &cfg_settings.print_level,
+ PRINT_LEVEL_MIN, PRINT_LEVEL_MAX))
+ return -1;
*cfg_ignore |= CFG_IGNORE_PRINT_LEVEL;
break;
case 'm':
--
1.7.1
Libor Pechacek
2013-06-03 11:44:04 UTC
Permalink
Post by Ken ICHIKAWA
config: Add support for more strict config value validation
config: Apply config value validation to logging_level option
util: Add common procedure to get argument values for ptp4l and
phc2sys
ptp4l and phc2sys: Get argument values with strict error checking
Look good to me.

Reviewed-by: Libor Pechacek <***@suse.cz>
Loading...