Segfault in Weak.get_copy with Infix_tag

[stedolan]

The implementation of Weak.get_copy is missing the special case necessary to handle Infix_tag, so it can corrupt memory:

        OCaml version 4.10.0

# let w = Weak.create 1 in
  let rec f () = g () and g () = f () in
  Weak.set w 0 (Some g);
  match Weak.get_copy w 0 with Some h -> h () | _ -> ();;
Segmentation fault

I don't think this is particularly urgent to fix, because it's been there a long time without anyone noticing:

        Objective Caml version 3.10.2

# let w = Weak.create 1 in
  let rec f () = g () and g () = f () in
  Weak.set w 0 (Some g);
  match Weak.get_copy w 0 with Some h -> h () | _ -> ();;        
Segmentation fault

[bobot]

In the same vein than this issue #7810 reported by someone named @stedolan. Sorry to not have fixed those issues since 2 years ago.

[stedolan]

I'd forgotten about that one!

This is a different Infix_tag bug, though - Weak.get_copy's reimplementation of Obj.dup is missing a case.

[bobot]

Do you think it is possible to directly call the code of Obj.dup?

[stedolan]

Looking at it, I think there are enough differences that this would be awkward.

I think our best option might be to deprecate get_copy and make it be an alias for get - I don't think the optimisation it enables is really helpful any more.

[bobot]

I don't see what changed from when the optimisation was added. It is useful except in OCaml stdlib in weakhashset and weakhashtbl, in order to not make alive a value just to see it fail the equality test:

• if the hash function is very bad
• if it is a problem to keep the bindings that are colliding with an often accessed binding

Still I really don't like (hate?) get_copy, Obj.dup in disguise. So removing it in an MR and checking the behavior with e.g Frama-C and other opam packages can be very beneficial.

[stedolan]

When the optimisation was added, an equality comparison was done whenever two keys ended up in the same bucket, which happens regularly. These days, and equality comparison is only done if two keys have exactly the same hash (not just bucket), which is very rare.

[bobot]

Indeed, I misremember the history. The weak hashtables were added in 0032c48, the use of the hash has been added many years later in db20929 (and described in Hashconsing in an Incrementally Garbage-Collected System by @damiendoligez and @pascal-cuoq).

So are you going to propose the removal of this function, at least from its use in weak hashtbl and deprecation?