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

-no-alias-deps allows to build self-referential compilation units #6886

Closed
vicuna opened this issue Jun 1, 2015 · 10 comments
Closed

-no-alias-deps allows to build self-referential compilation units #6886

vicuna opened this issue Jun 1, 2015 · 10 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Jun 1, 2015

Original bug ID: 6886
Reporter: @sliquister
Assigned to: @garrigue
Status: closed (set by @xavierleroy on 2017-02-16T14:15:11Z)
Resolution: fixed
Priority: normal
Severity: crash
Version: 4.02.1
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.02.2+dev / +rc1
Category: typing
Monitored by: @gasche

Bug description

Normally, the compiler forbids access to module A when compiling a.ml.
But if a.ml tries to access module A through an alias defined in an other compilation unit (with -no-alias-deps, otherwise we can't build the files), then there is apparently no such check. Then one can write the equivalent of:

module rec X : sig val x : string end = struct
include X
let () = print_endline x
end

at the compilation unit level and crash the program, and get other compile time or run time errors.

Steps to reproduce

#!/bin/bash

$ cat build.sh
cd /tmp
rm -f .cm
cat > lib.ml <<EOF
module A = A
EOF
ocamlopt -w -49 -no-alias-deps -c lib.ml
cat > a.ml <<EOF
let x = "1"
EOF
ocamlopt -no-alias-deps -open Lib -c a.ml
cat > a.ml <<EOF
include Lib.A
let () = Printf.printf "%s\n%!" x (* with this line, the code segfaults )
(
let y = 1 ) ( with this line instead, we get inconsistent assumption )
(
with neither line, the code types even though it shouldn't. Or we could have
other errors, like "this is not a cmi for this version of ocaml" depending
on the state of the filesystem *)
EOF
ocamlopt -no-alias-deps -open Lib -c a.ml
ocamlopt -i -no-alias-deps -open Lib -c a.ml
ocamlopt a.cmx -o a
./a

$ ./build.sh
val x : string
./build.sh: line 21: 20098 Segmentation fault ./a

File attachments

@vicuna
Copy link
Author

vicuna commented Jun 2, 2015

Comment author: @garrigue

Note sure what to do about this.
To me, it looks like there should be a link-time error.
I.e. A should not be allowed to refer to itself as an external value.
Why is this allowed?

@vicuna
Copy link
Author

vicuna commented Jun 2, 2015

Comment author: @sliquister

I see things like:

  begin try
    EnvTbl.find_name s env.components
  with Not_found ->
    if s = !current_unit then raise Not_found;

in env.ml, I would imagine the current unit is loaded anyway through an other code
path that doesn't have this check.

@vicuna
Copy link
Author

vicuna commented Jun 2, 2015

Comment author: @mshinwell

Leo is going to look at this

@vicuna
Copy link
Author

vicuna commented Jun 2, 2015

Comment author: @garrigue

The included patch seems to fix the problem, as you had diagnosed it.
Indeed, when using module aliases one may end up loading a unit in find_module rather than in lookup_module, and there was no check there.
This happens here because the self-reference to A goes through Lib, which is already compiled.
Yet, this patch requires more testing to be sure that this does not break anything.

@vicuna
Copy link
Author

vicuna commented Jun 2, 2015

Comment author: @damiendoligez

I'm going to test it against OPAM.

@vicuna
Copy link
Author

vicuna commented Jun 3, 2015

Comment author: @damiendoligez

The patch doesn't break anything in OPAM.

@vicuna
Copy link
Author

vicuna commented Jun 3, 2015

Comment author: @damiendoligez

@garrigue, can you commit the fix in branch 4.02?

@vicuna
Copy link
Author

vicuna commented Jun 3, 2015

Comment author: @garrigue

Fixed in trunk and 4.02 at revisions 16152 and 16153

@vicuna
Copy link
Author

vicuna commented Jun 28, 2015

Comment author: @glondu

This change breaks monotone-viz (which is packaged in Debian). See the bugreport:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=790062

in particular, message #10 which was also posted to the caml-list.

@vicuna
Copy link
Author

vicuna commented Jun 28, 2015

Comment author: @sliquister

The error message is not great but I think your code is broken, because compilation units are not supposed to be recursive. The dependencies between interfaces may be acylic, and the dependencies between implementations too, but the dependencies between (interface + implementation) are cyclic.

I can't test now, but it looks like a case that didn't work properly even before: try packing app.cmx query.cmx, and you should see that if you specify query.cmx first you get a garbage cmi because its Pack.Query.make takes an argument incompatible with Pack.App.t, and if you put app.cmx first, you'll get a pack time or link time error about an unresolved reference to module Query in App.

@vicuna vicuna closed this as completed Feb 16, 2017
@vicuna vicuna added the typing label Mar 14, 2019
@vicuna vicuna added this to the 4.02.2 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants