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

typeopt.ml unsafely assumes all modules called Bigarray are the same #7552

Closed
vicuna opened this Issue Jun 4, 2017 · 6 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented Jun 4, 2017

Original bug ID: 7552
Reporter: @lpw25
Status: resolved (set by @xavierleroy on 2017-09-30T16:06:49Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.04.1
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: middle end (typedtree to clambda)
Monitored by: @gasche

Bug description

The following program will segfault:

module Alias = struct module Bigarray = Bigarray end

module Bigarray : sig

type int_elt

val x : (float, int_elt, Bigarray.c_layout) Bigarray.Array1.t

end = struct

type int_elt = Bigarray.float32_elt

let x = Bigarray.Array1.create Bigarray.Float32 Bigarray.C_layout 2

end

let x = Bigarray.x

open Alias

let f = x.{0}

let () = Printf.printf "%f\n%!" f

The issue is that typeopt.ml assumes that the [int_elt] type from the [Bigarray] submodule is the same as the normal [Bigarray.int_elt] and so incorrectly optimises the array access.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 4, 2017

Comment author: @gasche

This comes from bytecomp/typeopt.ml:

let bigarray_decode_type env ty tbl dfl =
match scrape env ty with
| Tconstr(Pdot(Pident mod_id, type_name, _), [], _)
when Ident.name mod_id = "Bigarray" ->
begin try List.assoc type_name tbl with Not_found -> dfl end
| _ ->
dfl

In trunk (as opposed to 4.05) "CamlinternalBigarray" is used instead, but the test is still purely syntactic so the same trick would work.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 4, 2017

Comment author: @gasche

I don't know how to fix this without adding the kind types to typing/Predef, which (if I understand correctly) are the only ones that the compiler can reason about using absolute (non-shadowable) names.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 8, 2017

Comment author: @alainfrisch

This is perhaps naive, but couldn't we check [Ident.global mod_id]? It would still not protect against shadowing the global bigarray.cmi (which requires linking without the stdlib), but that would reduce the problem quite a bit.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 8, 2017

Comment author: @lpw25

I think Gabriel's suggestion is the correct solution: the compiler should really only be reasoning about abstract types it defined. I agree that Alain's suggestion would make it less of a problem, and we should probably do that in the short term if no one has time to do the right thing at the moment.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 8, 2017

Comment author: @alainfrisch

The compiler also does name-based lookups on CamlinternalOO and CamlinternalMod during code generation. Linking against a "non-standard" stdlib is already unsafe for this reason, I believe.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Sep 30, 2017

Comment author: @xavierleroy

In 4.06 and up the example works fine because the crucial type definitions are now in CamlinternalBigarray. That name is enough protection against accidental shadowing, in my opinion. Malicious users can still shadown the various Camlinternal* modules with their own concotions, but there are much easier ways to cause an OCaml program to segfault.

As to "the right thing" being to predefine more things within the compiler,I don't think it's sustainable. It might work here because only types are involved but would not work for the other Camlinternal* modules that also define functions and values. I'd be more interested in lightweight ways to "bless" a module as being the standard library module the compiler expects.

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.