type uint32 = int32 is risky

[talex5]

Cstruct declares uint32 = int32. However, int32 is signed and doing Int32.to_int may result in a negative number, which the calling code is unlikely to be expecting. Perhaps we should use a separate type for this, with safe accessor functions.

For example, ocaml-9p does:

let big_enough_for name buf needed =
  let length = Cstruct.len buf in
  if length < needed
  then error_msg "%s: buffer too small (%d < %d)" name length needed
  else return ()

let read rest =
  Fid.read rest
  >>= fun (fid, rest) ->
  Int64.read rest
  >>= fun (offset, rest) ->
  Int32.read rest
  >>= fun (len, rest) ->
  let len = Int32.to_int len in
  big_enough_for "Write.data" rest len
  >>= fun () ->
  let data = Cstruct.sub rest 0 len in
  let rest = Cstruct.shift rest len in
  return ({ fid; offset; data }, rest)

This can raise an exception (which the code is trying to avoid) for large values of len.

[avsm]

agreed, with a a major bump in the library version

[avsm]

We could do this via the integers package now, cc @yallop. I can do this after #138 is merged and immediately open up a version 4.0 with the bump for this type change.

[hannesm]

I'm not sure whether integers is always the right solution, please take a look at yallop/ocaml-integers#2 .. there could be some cstruct-integers layer which translates the stdlib types to integers types - it would be convenient (at least for me) to not introduce a hard dependency on integers, and be able to access the byte representation.

[yallop]

@hannesm, I'd like to understand your concerns better.

please take a look at yallop/ocaml-integers#2

Could you expand on how that (resolved) issue is relevant to using integers in cstruct? e.g. were you hoping for some other resolution?

some cstruct-integers layer which translates the stdlib types to integers types

The integers package already provides conversions between standard library types and types in that library (e.g.). Do you have something else in mind?

it would be convenient (at least for me) to [...] be able to access the byte representation.

I'm not sure what you mean here, either.

[hannesm]

On 11/05/2017 12:04, yallop wrote: Could you expand on how that (resolved) issue is relevant to using `integers` in `cstruct`? e.g. were you hoping for some other resolution?

Let's assume I have a struct which consists of a sequence number, such as in TLS (or OTR). The property which these should also satisfy is "may never ever wrap around" (otherwise, you use the same IV with the same key, and your symmetric crypto is broken). Of course, you could represent this as a (from `integers`) `Unsigned.Uint32.t`, and convert this to some other number representation which is more careful about wrapping. But wouldn't you then by construction introduce a chain of conversions: `bytes -> stdlib int -> integers Uint32.t -> other number` -- I'd prefer to have a more raw interface and avoid conversions where we can - esp. since cstruct seems to be used all over. Does this make sense? OTOH, adding a non-implicit way to handle wrap around in integers may solve the issue.

[hannesm]

Clearly, performance benchmarks of parsing e.g. uint32 (which atm is int32) vs Unsigned.Uint32.t (and the corresponding 64bit numbers) could help to show that my concerns are void.

[yallop]

Yes, I agree that it would be good to avoid a chain of conversions. We should certainly not go through stdlib int on the way to UInt32.t! It should instead be possible to go directly from the bigarray-referenced memory to an UInt32.t value.

I think the last piece of the puzzle, going from Unsigned.UInt32.t to the Usane type, would be fixed by this issue: hannesm/usane#3.

[XVilka]

So what was decided on this one?