search

ocaml-attic / ocaml-cstruct

Map OCaml arrays onto C-like structs via a syntax extension, and generate functions to convert to-and-from native OCaml values.
http://www.openmirage.org/
10 stars 2 forks source link

bounds checking: negative offsets and lengths #20

Closed djs55 closed 10 years ago

djs55 commented 10 years ago

The bounds checking currently looks like:

let check_bounds t len =
  Bigarray.Array1.dim t.buffer >= len
...
let sub t off len =
  let off = t.off + off in
  if not (check_bounds t (off+len)) then
    raise (Invalid_argument "Cstruct.sub");
  { t with off; len }
...
let shift t amount =
  let off = t.off + amount in
  let len = t.len - amount in
  if not (check_bounds t (off+len)) then
    raise (Invalid_argument "Cstruct.shift");
  { t with off; len }

I've screwed-up a number of times by getting a -ve offset and/or length. Nothing good happens in this state :)

I think that, if we're going to bounds-check at all, we should check everything and be safe. I've just noticed a higher-level library [johnelse/ocaml-crc] which is applying its own bounds checks at the layer above -- I'd like to eliminate those by making the lower-level safe. Does that sound sensible?

djs55 commented 10 years ago

I'll propose a few bounds-checking unit tests and some fixes.

avsm commented 10 years ago

I completely agree with you.

On 10 Dec 2013, at 15:58, Dave Scott notifications@github.com wrote:

I think that, if we're going to bounds-check at all, we should check everything and be safe. I've just noticed a higher-level library [johnelse/ocaml-crc] which is applying its own bounds checks at the layer above -- I'd like to eliminate those by making the lower-level safe. Does that sound sensible?

djs55 commented 10 years ago

OK, in the middle of adding some unit tests!

On Tue, Dec 10, 2013 at 6:28 PM, Anil Madhavapeddy <notifications@github.com

wrote:

I completely agree with you.

On 10 Dec 2013, at 15:58, Dave Scott notifications@github.com wrote:

I think that, if we're going to bounds-check at all, we should check everything and be safe. I've just noticed a higher-level library [johnelse/ocaml-crc] which is applying its own bounds checks at the layer above -- I'd like to eliminate those by making the lower-level safe. Does that sound sensible?

— Reply to this email directly or view it on GitHubhttps://github.com/mirage/ocaml-cstruct/issues/20#issuecomment-30254067 .

Dave Scott

avsm commented 10 years ago

test driven agile development; this feels so grown up.

On 10 Dec 2013, at 18:29, Dave Scott notifications@github.com wrote:

OK, in the middle of adding some unit tests!

On Tue, Dec 10, 2013 at 6:28 PM, Anil Madhavapeddy <notifications@github.com

wrote:

I completely agree with you.

On 10 Dec 2013, at 15:58, Dave Scott notifications@github.com wrote:

I think that, if we're going to bounds-check at all, we should check everything and be safe. I've just noticed a higher-level library [johnelse/ocaml-crc] which is applying its own bounds checks at the layer above -- I'd like to eliminate those by making the lower-level safe. Does that sound sensible?

— Reply to this email directly or view it on GitHubhttps://github.com/mirage/ocaml-cstruct/issues/20#issuecomment-30254067 .

Dave Scott — Reply to this email directly or view it on GitHub.

avsm commented 10 years ago

fixed in 1.x