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

Dubious compare and hash functions for libev loop wrapper objects #146

Closed
mark-christiaens opened this issue Apr 15, 2015 · 1 comment
Closed

Comments

@mark-christiaens
Copy link

In lwt_libev_stubs.c there is code to implement the hash and compare functions for the wrapper object for a libev loop structure (similar issues exists for the watcher structure):

static int compare_loops(value a, value b)
{
  return (int)(Data_custom_val(a) - Data_custom_val(b));
}

static long hash_loop(value loop)
{
  return (long)Data_custom_val(loop);
}

static struct custom_operations loop_ops = {
  "lwt.libev.loop",
  custom_finalize_default,
  compare_loops,
  hash_loop,
  custom_serialize_default,
  custom_deserialize_default
};

If I read the OCaml code correctly then Data_custom_val will give you a pointer into an ocaml object:

#define Data_custom_val( v ) ((void *) &Field(( v ), 1))

This pointer will change when compaction occurs making the compare and hash functions return different results. This could mess seriously with any data structures that rely on ordering or identity like maps, hash tables, sorted lists ...

I think the intent of the code was to use the C-structs pointer as a basis for calculating the hash and the comparison but that is not what the code is doing.

I tried to verify whether this code actually gets triggered: I attached a debugger to a running process and put some breakpoints in these functions but they were not hit in my small test. If this is indeed dead code, should it not simply be removed?

@aantron aantron closed this as completed in 816022d Jul 2, 2016
@aantron
Copy link
Collaborator

aantron commented Jul 2, 2016

Indeed, thanks for catching and reporting this. I don't think these functions are used, but I'd rather not remove them right now. I've yet to thoroughly consider Lwt_engine, and having these preserves some flexibility with what data structures can be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants