descriptor: size int64

[runcom]

Signed-off-by: Antonio Murdaca [email protected]

[wking]

On Thu, Jun 16, 2016 at 02:26:05PM -0700, Antonio Murdaca wrote:

• descriptor: size int64

I'm meh about the change ;), but for consistency you should update 1
to $ref 2.

And maybe uint64? Why keep the sign?

[runcom]

And maybe uint64? Why keep the sign?

because docker/distribution uses int64 - and I didn't want to move that far there. Obviously uint64 would be better as long as sizes aren't negative :)

but for consistency you should update [1]
to $ref [2].

done

[wking]

On Thu, Jun 16, 2016 at 02:40:29PM -0700, Antonio Murdaca wrote:

And maybe uint64? Why keep the sign?

because docker/distribution uses int64 - and I didn't want to move
that far there. Obviously uint64 would be better as long as sizes
aren't negative :)

I think that if we're going to get picky about how many bits we're
using, we should at least get the signedness right ;).

but for consistency you should update 1
to $ref [2].

done

int64 isn't a native type, you have to $ref it like 1.

[runcom]

I personally like being picky about types because it wasn't obvious to me if the image-spec defines an int but in reality it's at least an int64 and since it's a spec I expect this pickyness (and since I defined types in Go as well.). I also think we have an int64 defined for some reason.....

BTW I'll leave this decision to the maintainers and I'll either close or update this PR

[philips]

Are int64's represented by json numbers? A reference to the docker distribution value would be helpful for tracking.

[runcom]

https://github.com/docker/distribution/blob/master/blobs.go#L66

[wking]

On Thu, Jun 16, 2016 at 02:52:36PM -0700, Antonio Murdaca wrote:

I personally like being picky about types because it wasn't obvious
to me if the image-spec defines an int…

I'd been interpreting this like JSON which does not specify a maximum
size. “At least 64 bits of integer” would be a reasonable requirement
(like C's short, int, long, …), but int64 sets a maximum 1. I
don't see much use in that, because if you have an object that is
bigger (unlikely, but still), you probably want the right to take that
risk and set:

"size": 9223372036854776001

Signedness, on the other hand, is something that we should be able to
form a stable agreement on. What would negative sizes mean?

[runcom]

fwiw we already have something defined as int64 also https://github.com/runcom/image-spec/blob/fce87bead2bbbad896c6208b8518c29d78687255/schema/defs-config.json#L11

[stevvooe]

I think that if we're going to get picky about how many bits we're
using, we should at least get the signedness right ;).

signedness shouldn't be used for data validation. And a negative size is also a very clear way of saying the size is unknown, albeit inadvisable.

I'm not really sure if we need to be picky, other than that the parser should error out if the value overflows the field on whatever platform is consuming the data.

[wking]

On Wed, Jul 06, 2016 at 12:06:35PM -0700, Stephen Day wrote:

signedness shouldn't be used for data validation. And a negative
size is also a very clear way of saying the size is unknown, albeit
inadvisable.

The spec REQUIRES size 1, so I think:

• We can (and should) consider it during data validation.
• We should not allow negative values. If we want “I don't know the
  size” to be a legal statement, we should instead make ‘size’
  optional (and use a *uint in Go).

I'm not really sure if we need to be picky, other than that the
parser should error out if the value overflows the field on whatever
platform is consuming the data.

Agreed, and that^ sounds like a JSON-parser issue to me, and not
something we need to specify in the spec. RFC 7159 has a paragraph on
this here [2](“This specification allows implementations to set
limits on the range and precision of numbers accepted…”), although it
doesn't explicitly require the JSON implementation to error out on
overflows.

[stevvooe]

we should instead make ‘size’
optional (and use a *uint in Go).

I'm sorry, but using unsigned integers for data validation is bad practice. It makes underflow detection challenging and complicates offset declaration when working with binary storage formats. int64 is used throughout Go's APIs and they made a good decision. We can validate that this integer is positive, but requiring an unsigned integer in the implementation is just silly.

In general, this is a good addition. Throughout "the stack", 64-bit integers are sufficient for declaring offsets and size for large binary objects. The only area where I see this being a problem is with javascript, where it doesn't have a 64-bit integer, so making a requirement to support at least 64 bits, is problematic.

@runcom Was this PR a matter of hygiene or did you encounter a specific problem?

[wking]

On Wed, Jul 06, 2016 at 02:14:45PM -0700, Stephen Day wrote:

we should instead make ‘size’ optional (and use a *uint in Go).

I'm sorry, but using unsigned integers for data validation is bad
practice. It makes underflow detection challenging and complicates
offset declaration when working with binary storage formats.

Isn't underflow just a floating point thing 1? When I try to
json.Unmarshal a negative integer into a uint64 or *uint64 type, I
get:

json: cannot unmarshal number -1 into Go value of type uint64

The same is true of uint and *uint, although the error message ends
with ‘uint’.

I don't see how a signed size helps with offset declaration. Do you
use negative offsets? Can you point me to an example where using an
unsigned size would have complicated things?

int64 is used throughout Go's APIs and they made a good
decision. We can validate that this integer is positive, but
requiring an unsigned integer in the implementation is just silly.

I'm not sure why Go would choose to use a signed integer for a
parameter that can never be negative. That seems like it's just
asking for manual type checking that I'd rather leave to the compiler.

[stevvooe]

When I said "underflow", I mean, negative numbers. Let's say we have to ask the question "Can it fit?". space - size >= 0 answers that question when using signed integers.

The use case of negative binary offsets is to declare blocks relative to some known point. Let's say we add binary blob storage metadata to the end of a file. We can say that the block is -blocksize bytes from the end of the file and the end of the blob is at -n*blocksize, where n is the number of metadata blocks.

I'm not sure why Go would choose to use a signed integer for a
parameter that can never be negative. That seems like it's just
asking for manual type checking that I'd rather leave to the compiler.

The point of an unsigned integer is not to provide a number that can't go negative. The point is to define a field with specific overflow behavior. When working with sizes and offsets, we ignore the behavior of that field and operate as if they are integers, which is true when operating within the bounds of the field. When doing arithmetic operations on size, we want to believe it is an integer, not a field with overflow.

This is all beside the point: the question being asked here is, should we validate the size as positive (Yes, probably) and should we have a minimum range requirement on the field value for size?

[wking]

On Wed, Jul 06, 2016 at 02:53:27PM -0700, Stephen Day wrote:

… space - size >= 0 answers that question when using signed
integers.

And ‘space >= size’ answers it for all integers, signed or not.

The point of an unsigned integer is not to provide a number that
can't go negative. The point is to define a field with specific
overflow behavior.

And also to give you an extra bit's worth of positive-number space.
An int64 only gives you 63 bits of positive-number space.

This is all beside the point: the question being asked here is,
should we validate the size as positive (Yes, probably)…

I agree here.

… and should we have a minimum range requirement on the field value
for size?

And I'd rather punt to JSON here 1.

[philips]

I am fine with int64 and saying it MUST be positive.

[wking]

On Wed, Jul 27, 2016 at 05:05:13PM -0700, Brandon Philips wrote:

I am fine with int64 and saying it MUST be positive.

And putting a cap on the value in the JSON Schema, or would you leave
that off 1?

[philips]

@wking If we can express greater than 0 and less than 9223372036854775807 (9223 Petabytes) then I think that would be great.

Hopefully this won't be my 640k is enough for anyone moment :-P But I think 9,223 Petabytes is enough for anyone. :)

[philips]

LGTM

We can followup with more code if necessary. But, this is a good fix as-is.

[wking]

On Wed, Aug 03, 2016 at 10:50:10AM -0700, Brandon Philips wrote:

Hopefully this won't be my 640k is enough for anyone moment :-P But
I think 9,223 Petabytes is enough for anyone. :)

I don't see a reason to specify a max cap, but I agree that I'm
unlikely to bump into this one myself ;).

[stevvooe]

9PB layer -> maybe break up your image?

LGTM

[vbatts]

LGTM

[cgwalters]

And a negative size is also a very clear way of saying the size is unknown, albeit inadvisable.

If we wanted to support unknown size wouldn't we just make the size field optional? Why express that via negative numbers?

Anyways, 9 years later, I would like to know: is -1 valid in the size field or not? The spec doesn't say. We're getting this from some registries apparently (xref bootc-dev/bootc#1567 )

I introduced this problem apparently with youki-dev/oci-spec-rs#203 (my bad for not digging through the history of this spec).

But I don't want to just change it back to int64 without having some guidance for why I'm doing that. (And if I were to do so...because in Rust we have Option<T> I would definitely make it Option<u64> and not int64)

[cyphar]

In umoci we support -1 as a value for unknown sizes (I think I added this because of this discussion, but it was ~10 years ago...). I now think this is generally a misfeature -- the main reason why we have a size field along with a hash to avoid DoS attacks where an attacker drip-feeds you data forever and locks up a client because they don't know how long the stream should be. With a size field you can kill the connection once you've read enough bytes.

[sudo-bmitch]

In my own code, a -1 would always fail the size check, and I wouldn't ever expect to see that in a descriptor. In what scenario would the digest be known, but not the size?

[cgwalters]

The point is to define a field with specific overflow behavior.

@stevvooe You are conflating the types with specific programming language semantics. For example in Rust, there's explicit saturating methods on integers.

(Actually this gets into a different issue with the spec that afaics "int64" is not defined anywhere - i.e. the mapping of that type to JSON though its definition is pretty obvious; anyways that sub-thread was mostly covered above)

And ‘space >= size’ answers it for all integers, signed or not.

But yes exactly (or often I think people would write the inverse i.e. if (size > free_space) error()) but anyways this is really the problem domain for storage systems, having the type be forced to be signed isn't helpful at all in that as negative numbers as input are nonsensical and would need to be rejected early on anyways.

---

In my own code, a -1 would always fail the size check, and I wouldn't ever expect to see that in a descriptor. In what scenario would the digest be known, but not the size?

Anyways...it turns out that this issue was not directly related to this spec. It happens in the https://github.com/containers/container-libs codebase when the remote registry doesn't return a Content-Length for GetBlob HTTP request at least. The codebase used -1 for this case in a returned size data, but...I would place a monetary bet that the person writing it was very influenced in choosing int64 for this because the OCI spec did.

Obviously we do know the size up front because we're following the manifest, and it turns out that all callers of this internal API basically just ignore it - except in my Rust bindings I try to pervasively use u64 because negative sizes are obviously malformed and nothing should emit them, and the deserializer was erroring in this case.

So again, not directly related to this spec issue, but I'm sure influenced by it.

[sudo-bmitch]

Anyways...it turns out that this issue was not directly related to this spec. It happens in the https://github.com/containers/container-libs codebase when the remote registry doesn't return a Content-Length for GetBlob HTTP request at least. The codebase used -1 for this case in a returned size data

I'd treat the descriptor and the registry Content-Length header separately. Code will often have the descriptor when pulling the blob, and would therefore know the size in advance. If Content-Length is a valid value, then it could be validated against the descriptor. But otherwise I'd ignore the registry and depend on the descriptor.

There are scenarios in interacting with the registry where the size isn't known in advance (chunked or streaming blob push), but in those cases the digest isn't known yet and you don't have the descriptor yet, they become available at the end of the blob push.

[tianon]

saying it MUST be positive

So maybe we need a PR that adds this clarification in the markdown?
(I don't think we can reasonably change the Go types again, as nice as it might be to do so).

[sudo-bmitch]

saying it MUST be positive

So maybe we need a PR that adds this clarification in the markdown?

I've raised #1285 for this.