Large positive integers in OTIO JSON file misinterpreted as negative

[peachey]

Bug Report

Incorrect Functionality

A positive integer value greater than 2**31 - 1 in an OTIO JSON file will be read incorrectly by opentimelineio.adapters.read_from_file and may be misrepresented as a negative value due to 32-bit integer overflow.

To Reproduce

Run this python script:

import opentimelineio as otio
timeline = otio.adapters.read_from_file("test_input.otio")
otio.adapters.write_to_file(timeline, "test_output.otio")

using the input JSON OTIO file test_input.otio which is as follows:

{
    "OTIO_SCHEMA": "Timeline.1",
    "metadata": {},
    "name": "testdata",
    "global_start_time": null,
    "tracks": {
        "OTIO_SCHEMA": "Stack.1",
        "metadata": {},
        "name": "tracks",
        "source_range": null,
        "effects": [],
        "markers": [],
        "children": [
            {
                "OTIO_SCHEMA": "Track.1",
                "metadata": {},
                "name": "",
                "source_range": null,
                "effects": [],
                "markers": [],
                "children": [
                    {
                        "OTIO_SCHEMA": "Clip.1",
                        "metadata": {
                            "foobar": {
                                 "maxint32": 2147483647,
                                 "toobig": 2147483648,
                                 "verybig": 3450100000
                            }
                        },
                        "name": "black",
                        "source_range": {
                            "OTIO_SCHEMA": "TimeRange.1",
                            "duration": {
                                "OTIO_SCHEMA": "RationalTime.1",
                                "rate": 24.0,
                                "value": 24.0
                            },
                            "start_time": {
                                "OTIO_SCHEMA": "RationalTime.1",
                                "rate": 24.0,
                                "value": 1.0
                            }
                        },
                        "effects": [],
                        "markers": [],
                        "media_reference": {
                            "OTIO_SCHEMA": "ExternalReference.1",
                            "metadata": {},
                            "name": "",
                            "available_range": {
                                "OTIO_SCHEMA": "TimeRange.1",
                                "duration": {
                                    "OTIO_SCHEMA": "RationalTime.1",
                                    "rate": 24.0,
                                    "value": 240.0
                                },
                                "start_time": {
                                    "OTIO_SCHEMA": "RationalTime.1",
                                    "rate": 24.0,
                                    "value": 1.0
                                }
                            },
                            "target_url": "black.mov"
                        }
                    }
                ],
                "kind": "Video"
            }
        ]
    }
}

Then diff test_input.otio test_output.otio and you will see something like this:

<                                  "maxint32": 2147483647,
<                                  "toobig": 2147483648,
<                                  "verybig": 3450100000
---
>                                 "maxint32": 2147483647,
>                                 "toobig": -2147483648,
>                                 "verybig": -844867296

Note how the integer value of toobig turns from positive to negative, and the value of verybig is changed to a negative value that is completely different due to misrepresentation as a 32-bit integer.

Tested on:

• Linux ubuntu 20.04 with Python 3.8.5 (also reproduced on RHEL 7 with Python 2.7)
• OTIO version: commit 546b333
• gcc version 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04)

Expected Behavior

The expected behavior is that the contents of the test_output.otio file will be equivalent to the test_input.otio, that is, identical except for insignificant differences such as whitespace changes and ordering of keys in objects.

Additional Context

I think this is a result of line 41 of src/opentimelineio/deserialization.cpp:

    bool Uint(unsigned u) {  return store(any(int(u))); }

I have tested the following alternative code which seems to fix the bug:

    bool Uint(unsigned u) {
        if (u > INT32_MAX) {
            return store(any(int64_t(u)));
        }
        return store(any(int(u)));
    }

[meshula]

That's a good catch . Since (unsigned, int, long, etc) vary between machines and architectures, I think it would be reasonable to store uint64_t for UInt, always. It's fishy to me that we are storing a Uint in an int in the first place. Does anyone recall if there was a principled reason to to use int in the first place?

[jminor]

I don't think the memory size of this will be limiting, so 64-bits seems fine, but surely OTIO needs to support signed integers, not just unsigned.

[peachey]

I believe that if a signed int is seen by the rapidJSON reader it would call the Int (or Int64 if magnitude large enough) routine rather than the Uint routine, so signed integers would be fully supported in any case. I did already test the unconditional use of uint64_t for the Uint routine and that worked fine, but I thought it might be worth saving memory by doing the test on the value. Perhaps @davidbaraff has a preference for how to solve the issue?

[meshula]

Are we using json-schema, and specifying that some attributes have a "minimum" property of zero? If no, I do wonder how we wind up in Uint in the first place, since json itself doesn't know about unsigned integers.

If I'm not mistaken, int types wind up coerced into the storage_union which is big enough for two pointers. So going with 64 bits across the board is going to fine on 32 and 64 bit architectures, with respect to the "any" class.

    union storage_union
    {
        using stack_storage_t = typename std::aligned_storage<2 * sizeof(void*), std::alignment_of<void*>::value>::type;

        void*               dynamic;
        stack_storage_t     stack;      // 2 words for e.g. shared_ptr
    };