[Linuxptp-devel] [PATCH 3/3] phc2sys: provide more strict arg value check

[Ken ICHIKAWA]

Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
phc2sys.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 1dbe2bc..5b884da 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,89 @@ int main(int argc, char *argv[])
src = clock_open(optarg);
break;
case 'P':
- configured_pi_kp = atof(optarg);
+ if (get_ranged_double(optarg, &configured_pi_kp,
+ 0.0, DBL_MAX)) {
+ fprintf(stderr,
+ "-P: %s is a bad value\n", optarg);
+ return -1;
+ }
break;
case 'I':
- configured_pi_ki = atof(optarg);
+ if (get_ranged_double(optarg, &configured_pi_ki,
+ 0.0, DBL_MAX)) {
+ fprintf(stderr,
+ "-I: %s is a bad value\n", optarg);
+ return -1;
+ }
break;
case 'S':
- configured_pi_offset = atof(optarg);
+ if (get_ranged_double(optarg, &configured_pi_offset,
+ 0.0, DBL_MAX)) {
+ fprintf(stderr,
+ "-S: %s is a bad value\n", optarg);
+ return -1;
+ }
break;
case 'R':
- phc_interval = 1.0 / atof(optarg);
+ if (get_ranged_double(optarg, &phc_rate, 0.0, DBL_MAX)) {
+ fprintf(stderr,
+ "-R: %s is a bad value\n", optarg);
+ return -1;
+ }
+ phc_interval = 1.0 / phc_rate;
+ /* phc_interval will be assigned to a time_t value */
+ 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_ranged_int(optarg, &phc_readings, 1, INT_MAX)) {
+ fprintf(stderr,
+ "-N: %s is a bad value\n", optarg);
+ return -1;
+ }
break;
case 'O':
- dst_clock.sync_offset = atoi(optarg);
+ if (get_ranged_int(optarg, &dst_clock.sync_offset,
+ INT_MIN, INT_MAX)) {
+ fprintf(stderr,
+ "-O: %s is a bad value\n", optarg);
+ return -1;
+ }
dst_clock.sync_offset_direction = -1;
forced_sync_offset = 1;
break;
case 'u':
- dst_clock.stats_max_count = atoi(optarg);
+ if (get_ranged_uint(optarg, &dst_clock.stats_max_count,
+ 0, UINT_MAX)) {
+ fprintf(stderr,
+ "-u: %s is a bad value\n", optarg);
+ return -1;
+ }
break;
case 'w':
wait_sync = 1;
break;
case 'n':
- domain_number = atoi(optarg);
+ if (get_ranged_int(optarg, &domain_number, 0, 255)) {
+ fprintf(stderr,
+ "-n: %s is a bad value\n", optarg);
+ return -1;
+ }
break;
case 'x':
dst_clock.kernel_leap = 0;
break;
case 'l':
- print_level = atoi(optarg);
+ if (get_ranged_int(optarg, &print_level,
+ PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
+ fprintf(stderr,
+ "-l: %s is a bad value\n", optarg);
+ return -1;
+ }
break;
case 'm':
verbose = 1;

[Ken ICHIKAWA]

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.

Signed-off-by: Ken ICHIKAWA <***@jp.fujitsu.com>
---
util.c | 41 +++++++++++++++++++++++++++++++++++++++++
util.h | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/util.c b/util.c
index 55ec49f..501c027 100644
--- a/util.c
+++ b/util.c
@@ -17,7 +17,9 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
+#include <errno.h>

#include "sk.h"
#include "util.h"
@@ -190,3 +192,42 @@ int leap_second_status(uint64_t ts, int leap_set, int *leap, int *utc_offset)

return leap_status;
}
+
+int 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, 10);
+ if (errno != 0 || *endptr != '\0' ||
+ parsed_val < min || parsed_val > max)
+ return -1;
+ *result = parsed_val;
+ return 0;
+}
+
+int 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, 10);
+ if (errno != 0 || *endptr != '\0' ||
+ parsed_val < min || parsed_val > max)
+ return -1;
+ *result = parsed_val;
+ return 0;
+}
+
+int 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 (errno != 0 || *endptr != '\0' ||
+ parsed_val < min || parsed_val > max)
+ return -1;
+ *result = parsed_val;
+ return 0;
+}
diff --git a/util.h b/util.h
index d2734d1..006069a 100644
--- a/util.h
+++ b/util.h
@@ -114,4 +114,46 @@ 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);
+
+/**
+ * Get an integer value from string with error checking and range
+ * specification.
+ *
+ * @param str_val String which contains an integer value.
+ * @param result Parsed integer value is stored on success.
+ * @param min If parsed value is less than min, the value is
+ * treated as an invalid value.
+ * @param max If parsed value is bigger than max, the value is
+ * treated as an invalid value.
+ * @return 0 on success, -1 if str_val is not valid.
+ */
+int 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 unsigned integer value is stored on success.
+ * @param min If parsed value is less than min, the value is
+ * treated as an invalid value.
+ * @param max If parsed value is bigger than max, the value is
+ * treated as an invalid value.
+ * @return 0 on success, -1 if str_val is not valid.
+ */
+int 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 double value is stored on success.
+ * @param min If parsed value is less than min, the value is
+ * treated as an invalid value.
+ * @param max If parsed value is bigger than max, the value is
+ * treated as an invalid value.
+ * @return 0 on success, -1 if str_val is not valid.
+ */
+int get_ranged_double(const char *str_val, double *result, double min, double max);
#endif

[Keller, Jacob E]

Comments inline,

-----Original Message-----
Sent: Thursday, May 23, 2013 12:02 AM
Subject: [Linuxptp-devel] [PATCH 1/3] Add functions for input error
checking
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.
---
util.c | 41 +++++++++++++++++++++++++++++++++++++++++
util.h | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 0 deletions(-)
diff --git a/util.c b/util.c
index 55ec49f..501c027 100644
--- a/util.c
+++ b/util.c
@@ -17,7 +17,9 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
+#include <errno.h>
#include "sk.h"
#include "util.h"
@@ -190,3 +192,42 @@ int leap_second_status(uint64_t ts, int leap_set,
int *leap, int *utc_offset)
return leap_status;
}
+
+int 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, 10);

Return separate error values if it can't be parsed as an int vs if it is out of range. I would use some sort of define for this..

+ if (errno != 0 || *endptr != '\0' ||
+ parsed_val < min || parsed_val > max)
+ return -1;
+ *result = parsed_val;
+ return 0;
+}
+
+int 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, 10);
+ if (errno != 0 || *endptr != '\0' ||
+ parsed_val < min || parsed_val > max)
+ return -1;

Same as above.

+ *result = parsed_val;
+ return 0;
+}
+
+int 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 (errno != 0 || *endptr != '\0' ||
+ parsed_val < min || parsed_val > max)

Again, it would be good to return separate error cases so that the print statement can be tailored.

+ return -1;
+ *result = parsed_val;
+ return 0;
+}
diff --git a/util.h b/util.h
index d2734d1..006069a 100644
--- a/util.h
+++ b/util.h
@@ -114,4 +114,46 @@ 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);
+
+/**
+ * Get an integer value from string with error checking and range
+ * specification.
+ *
+ * treated as an invalid value.
+ * treated as an invalid value.
+ */
+int 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.
+ *
+ * treated as an invalid value.
+ * treated as an invalid value.
+ */
+int 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.
+ *
+ * treated as an invalid value.
+ * treated as an invalid value.
+ */
+int get_ranged_double(const char *str_val, double *result, double min, double max);
#endif
--
1.7.1

- Jake

[Ken ICHIKAWA]

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

diff --git a/ptp4l.c b/ptp4l.c
index ecaf9ed..6c27510 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -237,7 +237,12 @@ int main(int argc, char *argv[])
*cfg_ignore |= CFG_IGNORE_SLAVEONLY;
break;
case 'l':
- cfg_settings.print_level = atoi(optarg);
+ if (get_ranged_int(optarg, &cfg_settings.print_level,
+ PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
+ fprintf(stderr,
+ "-l: %s is a bad value\n", optarg);
+ return -1;
+ }
*cfg_ignore |= CFG_IGNORE_PRINT_LEVEL;
break;
case 'm':

[Keller, Jacob E]

Comment inline,

-----Original Message-----
Sent: Thursday, May 23, 2013 12:04 AM
Subject: [Linuxptp-devel] [PATCH 2/3] ptp4l: provide more strict arg
value check
---
ptp4l.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/ptp4l.c b/ptp4l.c
index ecaf9ed..6c27510 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -237,7 +237,12 @@ int main(int argc, char *argv[])
*cfg_ignore |= CFG_IGNORE_SLAVEONLY;
break;
- cfg_settings.print_level = atoi(optarg);
+ if (get_ranged_int(optarg,
&cfg_settings.print_level,
+ PRINT_LEVEL_MIN,
PRINT_LEVEL_MAX)) {
+ fprintf(stderr,
+ "-l: %s is a bad value\n", optarg);
+ return -1;

Here, you should have get_ranged_int return -1 if it isn't an int, and -2 if it is out of range, and have the error message show "malformed value" for non-ints and "out of range" for values out of range. This also should apply to the phc2sys patch as well.

+
*cfg_ignore |= CFG_IGNORE_PRINT_LEVEL;
break;
--
1.7.1

- Jake

[Keller, Jacob E]

Looks good to me, but I don't see 1/3 of your patch series?

- Jake

-----Original Message-----
Sent: Thursday, May 23, 2013 12:00 AM
Subject: [Linuxptp-devel] [PATCH 0/3] More strict input value validation

[Ken ICHIKAWA]

Post by Keller, Jacob E
Looks good to me, but I don't see 1/3 of your patch series?
- Jake

Do you mean [PATCH 1/3] has not reached you?

[Linuxptp-devel] [PATCH 1/3] Add functions for input error checking
http://sourceforge.net/mailarchive/message.php?msg_id=30884562

Thanks,
Ken ICHIKAWA

Post by Keller, Jacob E

-----Original Message-----
Sent: Thursday, May 23, 2013 12:00 AM
Subject: [Linuxptp-devel] [PATCH 0/3] More strict input value validation

[Keller, Jacob E]

Comment at bottom,

-----Original Message-----
Sent: Thursday, May 23, 2013 12:00 AM
Subject: [Linuxptp-devel] [PATCH 0/3] More strict input value validation

[Ken ICHIKAWA]

Post by Keller, Jacob E
Comment at bottom,

-----Original Message-----
Sent: Thursday, May 23, 2013 12:00 AM
Subject: [Linuxptp-devel] [PATCH 0/3] More strict input value validation

[Keller, Jacob E]

-----Original Message-----
Sent: Sunday, May 26, 2013 6:42 PM
To: Keller, Jacob E
Subject: Re: [Linuxptp-devel] [PATCH 0/3] More strict input value
validation

Post by Keller, Jacob E
Comment at bottom,

-----Original Message-----
Sent: Thursday, May 23, 2013 12:00 AM
Subject: [Linuxptp-devel] [PATCH 0/3] More strict input value

validation