* Fixed issues amazon-ion#224 & amazon-ion#226: Ion int to/from int64_t

* Fixed issues amazon-ion#224 & amazon-ion#226: Ion int to/from int64_t

* Addressed a corner case in ion_int_from_long that caused an
infinite loop when MIN_INT64 was passed in as the long value.
* Modified the `ion_int_to_int64` function to work on 63- and
  64-bit magnitudes.
* Adds roundtripping unit tests that convert an int64_t to an
  IINT and then back to an int64_t again.
* Added a unit test to verify that `ion_int_to_int64` detects
  overflow as expected. It tests both the case where the value
  can fit in 64 unsigned bits (but not 64 signed bits) and the
  case where the number is too big to fit in 64 bits.

Co-authored-by: Zack Slayton <[email protected]>

ionc/ion_int.c

@@ -39,6 +39,7 @@
 #define ION_INT_GLOBAL /* static */
 
 #include <decNumber/decNumber.h>
+#include <math.h>
 #include "ion_internal.h"
 
 iERR ion_int_alloc(void *owner, ION_INT **piint)
@@ -615,8 +616,12 @@ iERR ion_int_from_long(ION_INT *iint, int64_t value)
 {
     iENTER;
     SIZE  ii_length, digit_idx;
-    int64_t temp;
-    BOOL    is_neg;
+    // Stores the unsigned magnitude of the provided int64_t value. This variable must be
+    // unsigned to accommodate the absolute value of MIN_INT64, which requires 64 bits to store.
+    uint64_t magnitude;
+    // Used for shifting operations that consume the variable.
+    uint64_t temp_magnitude;
+    BOOL is_negative;
 
     IONCHECK(_ion_int_validate_arg(iint));
 
@@ -625,25 +630,32 @@ iERR ion_int_from_long(ION_INT *iint, int64_t value)
         SUCCEED();
     }
 
-    if ((is_neg = (value < 0)) == TRUE) {
-        value = -value;
+    is_negative = value < 0;
+    magnitude = (uint64_t) value;
+    if (is_negative) {
+        // This negates an unsigned value, which is well-defined behavior. Doing so handles the
+        // MIN_INT64 case: an int64_t whose absolute value is too large to be stored in an int64_t.
+        magnitude = -magnitude;
     }
 
-    ii_length = 0; 
-    temp = value;
-    while (temp) {
-        temp >>= II_SHIFT;
+    ii_length = 0;
+    temp_magnitude = magnitude;
+    while (temp_magnitude) {
+        temp_magnitude >>= II_SHIFT;
         ii_length++;
     }
 
+    // Reallocate iint's storage if it's not big enough to hold
+    // (ii_length * II_BITS_PER_II_DIGIT) bits.
     IONCHECK(_ion_int_extend_digits(iint, ii_length, TRUE));
 
-    for (digit_idx = iint->_len-1; value; digit_idx--) {
-        iint->_digits[digit_idx] = (II_DIGIT)(value & II_MASK);
-        value >>= II_SHIFT;
+    temp_magnitude = magnitude;
+    for (digit_idx = iint->_len-1; temp_magnitude; digit_idx--) {
+        iint->_digits[digit_idx] = (II_DIGIT)(temp_magnitude & II_MASK);
+        temp_magnitude >>= II_SHIFT;
     }
 
-    iint->_signum = is_neg ? -1 : 1;
+    iint->_signum = is_negative ? -1 : 1;
 
     iRETURN;
 }
@@ -1599,28 +1611,63 @@ SIZE _ion_int_abs_bytes_signed_length_helper(const ION_INT *iint)
     return _ion_int_abs_bytes_length_helper_helper(iint, /*is_signed=*/TRUE);
 }
 
-
 iERR _ion_int_to_int64_helper(ION_INT *iint, int64_t *p_int64)
 {
     iENTER;
     II_DIGIT *digits, *end, digit;
-    int64_t   value = 0;
+    uint64_t magnitude = 0;
 
     digits = iint->_digits;
     end    = digits + iint->_len;
-    while (digits < end) {
+
+    // iint's magnitude is stored in an array of 31-bit II_DIGIT values. Magnitudes that
+    // require 63 or 64 bits to represent will then take 3 II_DIGITS, only using 1 to 2
+    // bits from the most significant II_DIGIT.
+    const uint32_t max_digits_per_int64_t = 3;
+    const uint32_t whole_digits_per_int64_t = 2;
+    const uint32_t max_partial_digit_value = 3; // == 0b11, two populated bits
+
+    // If iint has more 31-bit digits than could possibly fit in an int64_t, return an error.
+    if (iint->_len > max_digits_per_int64_t) {
+        FAILWITH(IERR_NUMERIC_OVERFLOW);
+    }
+
+    // Check whether iint has a partial leading digit.
+    if (iint->_len > whole_digits_per_int64_t) {
         digit = *digits++;
-        value <<= II_SHIFT;
-        value += digit;
-        if (value < 0) {
-             FAILWITH(IERR_NUMERIC_OVERFLOW);
+        // If the leading digit uses too many bits to fit in the int64_t, return an error.
+        if (digit > max_partial_digit_value) {
+            FAILWITH(IERR_NUMERIC_OVERFLOW);
         }
+        // This digit is small enough to fit in the space available.
+        magnitude += digit;
     }
 
+    // The remaining digits will fit into 64 bits and can be processed as whole values.
+    while (digits < end) {
+        digit = *digits++;
+        magnitude <<= II_SHIFT;
+        magnitude += digit;
+    }
+
+    // While we know that the magnitude was able fit into 64 unsigned bits, it may still
+    // be too large to fit into an int64_t. We'll need to do some more bounds checking.
     if (iint->_signum == -1) {
-        value = -value;
+        if (magnitude > ((uint64_t) MIN_INT64)) {
+            FAILWITH(IERR_NUMERIC_OVERFLOW);
+        } else {
+            // Negate the unsigned magnitude before casting back to a signed value.
+            // Negating an unsigned value is well-defined behavior. Doing so handles the
+            // MIN_INT64 case: an int64_t whose absolute value is too large to be stored
+            // in an int64_t.
+            *p_int64 = (int64_t) -magnitude;
+        }
+    } else {
+        if (magnitude > ((uint64_t) MAX_INT64)) {
+            FAILWITH(IERR_NUMERIC_OVERFLOW);
+        }
+        *p_int64 = (int64_t) magnitude;
     }
-    *p_int64 = value;
     SUCCEED();
 
     iRETURN;

test/CMakeLists.txt

@@ -24,6 +24,7 @@ add_executable(all_tests
     test_ion_timestamp.cpp
     test_ion_values.cpp
     test_ion_extractor.cpp
+    test_ion_integer.cpp
     test_ion_cli.cpp
     test_ion_stream.cpp
     test_ion_reader_seek.cpp

test/test_ion_integer.cpp

@@ -0,0 +1,121 @@
+/*
+ * Copyright 2009-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at:
+ *
+ *     http://aws.amazon.com/apache2.0/
+ *
+ * or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific
+ * language governing permissions and limitations under the License.
+ */
+
+#include "ion_assert.h"
+#include "ion_helpers.h"
+#include "ion_test_util.h"
+
+iERR test_ion_int_roundtrip_int64_t(int64_t value_in, int64_t * value_out) {
+    iENTER;
+    // Create an uninitialized Ion integer.
+    ION_INT * iint;
+    // Initialize the Ion integer, setting its owner to NULL.
+    IONCHECK(ion_int_alloc(NULL, &iint));
+    // Populate the Ion integer with the provided int64_t value.
+    IONCHECK(ion_int_from_long(iint, value_in));
+    // Read the Ion integer's value back out into the output int64_t.
+    IONCHECK(ion_int_to_int64(iint, value_out));
+    // Free the memory used to store the Ion integer's digits.
+    ion_int_free(iint);
+    iRETURN;
+}
+
+TEST(IonInteger, IIntToInt64RoundTrip) {
+    // This test verifies that the `ion_int_from_long` and `ion_int_to_int64` functions work as
+    // intended. For each of the following values in the range from MIN_INT64 to MAX_INT64 inclusive,
+    // the test will convert the int64_t to an IINT and then back again, confirming that the output
+    // int64_t is equal to the input int64_t.
+    const uint32_t number_of_values = 19;
+    int64_t values[number_of_values] = {
+            MIN_INT64, MAX_INT64,
+            MIN_INT64 + 16, MAX_INT64 - 16,
+            -9670031482938124, 9670031482938124,
+            -10031482954246, 10031482954246,
+            -58116329947, 58116329947,
+            -66182226, 66182226,
+            -75221, 75221,
+            -825, 825
+            -1, 1,
+            0
+    };
+
+    int64_t value_in;
+    int64_t value_out;
+    for(int m = 0; m < number_of_values; m++) {
+        value_in = values[m];
+        ION_ASSERT_OK(test_ion_int_roundtrip_int64_t(value_in, &value_out));
+        ASSERT_EQ(value_in, value_out);
+    }
+}
+
+iERR test_ion_int_to_int64_t_overflow_detection(const char * p_chars) {
+    iENTER;
+    const uint32_t max_string_length = 32;
+    uint32_t string_length = strnlen(p_chars, max_string_length);
+    // Create an uninitialized Ion integer.
+    ION_INT iint;
+    // Create an int64_t that we will later populate with the value of iint.
+    int64_t value_out;
+    // Initialize the Ion integer, setting its owner to NULL.
+    IONCHECK(ion_int_init(&iint, NULL));
+    // Populate the Ion integer with the value of the provided base-10 string
+    IONCHECK(ion_int_from_chars(&iint, p_chars, string_length));
+    // Attempt to read the Ion integer's value back out into the int64_t.
+    // If the number is outside the range of values that can be represented by
+    // an int64_t, this should return IERR_NUMERIC_OVERFLOW.
+    IONCHECK(ion_int_to_int64(&iint, &value_out));
+    iRETURN;
+}
+
+TEST(IonInteger, IIntToInt64Overflow) {
+    // This test verifies that the `ion_int_to_int64` method will return IERR_NUMERIC_OVERFLOW
+    // if the provided Ion integer's value will not fit in an int64_t. Because any Ion integer
+    // constructed using `ion_int_from_long` will inherently fit in an int64_t, we instead
+    // construct each Ion integer with `ion_int_from_chars`, passing in Ion text encodings
+    // of the integers to create.
+    const uint32_t number_of_ok_values = 9;
+    const char *small_integers[number_of_ok_values] = {
+        "-10004991088",
+        "-9862",
+        "-138",
+        "-1",
+        "0",
+        "1",
+        "138",
+        "9862",
+        "10004991088"
+    };
+
+    // Each of the above values will fit in an int64_t, so the test function should succeed.
+    for (int m = 0; m < number_of_ok_values; m++) {
+        const char *small_integer = small_integers[m];
+        ION_ASSERT_OK(test_ion_int_to_int64_t_overflow_detection(small_integer));
+    }
+
+    const uint32_t number_of_oversized_values = 4;
+    const char *oversized_integers[number_of_oversized_values] = {
+        "9223372036854775808",  // MAX_INT64 + 1
+        "-9223372036854775809", // MIN_INT64 - 1
+        "10004991088252643637337337422",
+        "-10004991088252643637337337422",
+    };
+
+    // Each of the above values has a magnitude that is too large to fit in an int64_t,
+    // so the test function should fail, returning IERR_NUMERIC_OVERFLOW.
+    for (int m = 0; m < number_of_oversized_values; m++) {
+        const char *oversized_integer = oversized_integers[m];
+        iERR error_value = test_ion_int_to_int64_t_overflow_detection(oversized_integer);
+        ASSERT_EQ(error_value, IERR_NUMERIC_OVERFLOW);
+    }
+}