Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix off-by-one error in Weak.create. #1782

Merged
merged 2 commits into from May 13, 2018

Conversation

Projects
None yet
4 participants
@kayceesrk
Copy link
Contributor

commented May 13, 2018

Weak.create accepts -1 for the array length. It should raise Invalid_argument instead.

$ ocaml
        OCaml version 4.08.0+dev0-2018-04-09

# let v = Weak.create (-1);;
val v : '_weak1 Weak.t = <abstr>
# Weak.length v;;
- : int = -1

This appears to be an off-by-one error in the bounds check in caml_weak_create. This PR fixes the bounds. The corresponding bug report is MPR#7785.

kayceesrk added some commits May 13, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented May 13, 2018

I wondered while reading the code whether it was intentional that passing 0 for the len parameter is accepted. (If you interpret size <= 0 in the previous code as a typo for something like Long_val(len) <= 0, then this would be rejected.) But it seems reasonable (as a valid corner case) to create "ephemerons with no keys", which if I understand correctly corresponds to dummy wrappers around an optional value.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2018

The original code seems to have inherited the bounds checks from the original weak array implementation. Zero length weak arrays have always been allowed similar to zero length arrays. I am not sure whether there is any use for zero length weak arrays (or for that matter zero length arrays?). In the same sense, "ephemerons with no keys" does not seem particularly useful. The PR continues to allow zero length weak arrays and ephemerons without keys.

@let-def

This comment has been minimized.

Copy link
Contributor

commented May 13, 2018

Zero length weak arrays can occur naturally when implementing dynamically sized weak arrays, rejecting that case would add unnecessary complexities for some implementation.

But the -1 really looks like a mistake :) (and the fix looks good to me).

@gasche

gasche approved these changes May 13, 2018

@gasche gasche merged commit 4b33a7e into ocaml:trunk May 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bobot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

@kayceesrk instead of 2 you could use CAML_EPHE_FIRST_KEY. It will help you for when you will add domain id for the multi-core (same for the previous addition). Even better we could move the test before the addition so that not doing arithmetic modulo.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

@bobot Good idea.

Even better we could move the test before the addition so that not doing arithmetic modulo.

In that case, the code would be:

if (Long_val(len) < 0 || Long_val(len) > Max_wosize - CAML_EPHE_FIRST_KEY) 
  caml_invalid_argument ("Weak.create");
size = Long_val (len) + CAML_EPHE_FIRST_KEY;

This reads better and also guards against any changes to the ephemeron structure where new fields may be added to the structure before the keys (such as in the case of multicore). Looks like the documentation for Weak.create also needs to be updated: https://github.com/ocaml/ocaml/blob/trunk/stdlib/weak.mli#L41. It should be {!Sys.max_array_length}[-2].

@gasche How should I got about making this change. Should I add a new commit to the same PR?

@bobot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2018

#676 defines CAML_EPHE_MAX_WOSIZE. I'm going to add you to my ocaml repository, so that you could make this modification there if you want.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

#676 does not have this off-by-one bug. Both code and documentation are fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.