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 errors in Weak.get_copy and Weak.blit #1835

Merged
merged 4 commits into from Jun 14, 2018

Conversation

Projects
None yet
6 participants
@kayceesrk
Copy link
Contributor

kayceesrk commented Jun 12, 2018

This PR fixes off-by-one errors in Weak.get_copy and Weak.blit. All of the use of constants offsets in weak.c have been replaced by macros defined in weak.h.

Before this PR the following programs were accepted:

$ ocaml
# let w = Weak.create 0;;    
val w : '_weak1 Weak.t = <abstr>                           
# Weak.get_copy w (-1);;     
- : '_weak2 option = None    
# let w1, w2 = Weak.create 16, Weak.create 16;;            
val w1 : '_weak3 Weak.t = <abstr>                          
val w2 : '_weak4 Weak.t = <abstr>                          
# Weak.blit w1 (-1) w2 (-1) 8;;                            
- : unit = ()   

Since an offset of -1 refers to the data field of an ephemeron, you could get the program to segfault:

$ cat segfault.ml
let segfault =
  let e : (int ref, int) Ephemeron.Kn.t = Ephemeron.Kn.create 0 in
  Ephemeron.Kn.set_data e 0;
  match Ephemeron.Kn.get_key_copy e (-1) with
  | Some v -> print_int !v (* segfault here *)
  | None -> ()
$ ocaml segfault.ml
Segmentation fault (core dumped)

With this PR, the compiler correctly raises Invalid_argument exception for negative offsets.

$ ocaml
# let w1, w2 = Weak.create 16, Weak.create 16;;
val w1 : '_weak1 Weak.t = <abstr>
val w2 : '_weak2 Weak.t = <abstr>
# Weak.blit w1 (-1) w2 (-1) 8;;
Exception: Invalid_argument "Weak.blit".
# let w = Weak.create 0;;
val w : '_weak3 Weak.t = <abstr>
# Weak.get_copy w (-1);;
Exception: Invalid_argument "Weak.get_copy".
@nojb

nojb approved these changes Jun 12, 2018

Copy link
Contributor

nojb left a comment

Thanks for the nice cleanup, LGTM.

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented Jun 12, 2018

Also, maybe move the Changes entry to Bug fixes.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 12, 2018

Thanks for the review @nojb.

Also, maybe move the Changes entry to Bug fixes.

Done.

@bobot bobot referenced this pull request Jun 12, 2018

Merged

Ephemeron C API #676

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Jun 12, 2018

@bobot: if you're not too irritated about #676, would you be kind enough to review this one?

All: if it is still time, should this be rushed into 4.07 ?

@bobot

bobot approved these changes Jun 13, 2018

Copy link
Contributor

bobot left a comment

It indeed fix some off-by-one errors, so I approve it. But I would prefer to test the argument before doing any calculation, and I think the test in blit offset + length doesn't guard against sufficiently big value that makes the sum wrap (something I think is fixed in #676 CAMLassert(length <= Wosize_val(ars) - CAML_EPHE_FIRST_KEY); CAMLassert(offset_s <= Wosize_val(ars) - CAML_EPHE_FIRST_KEY - length);
).

For #676, I'm willing to cut it in pieces so that it could be more easily accepted. I just don't know where to cut and if it would be effective.

@damiendoligez
Copy link
Member

damiendoligez left a comment

Looks good except for one small omission.

Show resolved Hide resolved byterun/weak.c Outdated
@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 13, 2018

Thanks for the review @bobot. It might be best to split #676 to have a separate PR that handles wrap arounds since those are bug fixes.

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Jun 13, 2018

All: if it is still time, should this be rushed into 4.07 ?

How bad is it in practice?

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 13, 2018

@damiendoligez fixed the omission.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jun 13, 2018

How bad is it in practice?

Personally I would say that having pure-OCaml code that segfaults is "very bad", especially when the code is innocuous-looking and the behavior could happen by user mistake.

@damiendoligez damiendoligez merged commit 314cb28 into ocaml:trunk Jun 14, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

damiendoligez added a commit that referenced this pull request Jun 14, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Jun 14, 2018

Moved Changes entry and cherry-picked to 4.07 (commit a6c7dd0)

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 14, 2018

Thanks @damiendoligez.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.