Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006460OCamlOCaml runtime systempublic2014-06-17 23:392014-07-18 11:18
Reporterlpw25 
Assigned Todoligez 
PrioritynormalSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.02.0+dev 
Target Version4.02.0+devFixed in Version4.02.0+dev 
Summary0006460: caml_make_array assumes small arrays
Descriptioncaml_make_array asserts that the size of the array is less than Max_young_wosize so that it can use caml_alloc_small. However, the compiler does not enforce this invariant.

    $ cat test.ml
    let f x =
      [| x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x;
         x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; x; |]
     
    let _ = f 3.5

    $ ocamlopt -runtime-variant d test.ml

    $ ./a.out
    ### OCaml runtime: debug mode ###
    Initial minor heap size: 2048k bytes
    Initial major heap size: 3840k bytes
    Initial space overhead: 80%
    Initial max overhead: 500%
    Initial heap increment: 15%
    Initial allocation policy: 0
    file array.d.c; line 217 ### Assertion failed: size < Max_young_wosize
Tagsjunior_job, patch
Attached Files? file icon test.ml [^] (3,420 bytes) 2014-06-17 23:39 [Show Content]
? file icon test_minor_overflow.ml [^] (15,464 bytes) 2014-06-20 10:09 [Show Content]
diff file icon caml_make_array.diff [^] (729 bytes) 2014-06-20 10:10 [Show Content]

- Relationships

-  Notes
(0011763)
nevor (reporter)
2014-06-20 10:09

This bug is interesting because any literal given (including the one in test.ml) will be valid as long as it is smaller than the size of the minor heap which is a lot bigger than the enforced invariant.

So we found a minimal test that trashes the memory because of the violated invariant and we have attached a simple patch that corrects the problem by calling caml_alloc_shr when the size goes over Max_young_wosize. The new example and the patch work both on bytecode and native.

Maybe this was not done before for a good reason, in that case we might consider something along the lines of refusing too big literals, any constant smaller than Minor_heap_min would be good enough.
(0011880)
doligez (administrator)
2014-07-17 16:36

Patch will be applied before 4.02.0.
(0011882)
xleroy (administrator)
2014-07-18 11:18

Fixed as proposed in 4.02 (commit 15010) and on trunk (commit 15011).

I heard some questions about the use of caml_check_urgent_gc after caml_alloc_shr. This is correct, this is cheap, and this can help memory performance. caml_alloc_shr never triggers a GC, because often it is being called during a minor GC. However, caml_alloc_shr can set flags saying that it is "urgent" to perform a GC in the future. caml_check_urgent_gc will check and honor this flag, without waiting for the next polling of this flag by the bytecode interpreter or generated native code.

- Issue History
Date Modified Username Field Change
2014-06-17 23:39 lpw25 New Issue
2014-06-17 23:39 lpw25 File Added: test.ml
2014-06-19 17:55 gasche Tag Attached: junior_job
2014-06-20 10:09 nevor Note Added: 0011763
2014-06-20 10:09 nevor File Added: test_minor_overflow.ml
2014-06-20 10:10 nevor File Added: caml_make_array.diff
2014-07-16 10:29 doligez Tag Attached: patch
2014-07-16 10:30 doligez Status new => acknowledged
2014-07-16 10:30 doligez Target Version => 4.02.0+dev
2014-07-17 16:34 doligez Assigned To => doligez
2014-07-17 16:34 doligez Status acknowledged => assigned
2014-07-17 16:36 doligez Note Added: 0011880
2014-07-17 16:36 doligez Status assigned => confirmed
2014-07-18 11:18 xleroy Note Added: 0011882
2014-07-18 11:18 xleroy Status confirmed => resolved
2014-07-18 11:18 xleroy Resolution open => fixed
2014-07-18 11:18 xleroy Fixed in Version => 4.02.0+dev


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker