Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005942OCamlruntime system and C interfacepublic2013-03-11 23:292017-10-09 20:15
Assigned To 
PlatformOSOS Version
Product Version4.00.1 
Target VersionFixed in Version 
Summary0005942: Weak hash of serialized closures
DescriptionUsually, when you serialize a closure with one program, it is impossible to unserialize it with a different program, and you expect to obtain a runtime exception like: Failure("input_value: unknown code module FAE2E4BE7A3AE0091CF3043126B2CC65")

But, when two programs differs only by their data segments (see the attached example), it is possible to marshal a closure with the first program and to unmarshal it with the second program. Obviously, if you try to call the invalid unserialized closue, execution results in the famous "segmentation fault".

This bug is reproducible with ocamlc and ocamlopt.

The problem is that the hash sum stored in the marshalled closure is only computed with the code segment and not with the data segment.

I wrote a patch to fix it.
Steps To ReproduceWith attached and, run:

ocamlopt -o x
ocamlopt -o y
Do not import "data", ok
Segmentation fault (core dumped)

Additional InformationFixed by the attached patch: ocaml-4.00.1-data-marsh.diff.

Remark: dynlink is ok because the entire dynlinked files are hashed.
So, I just modify byterun and asmrun.
Attached Filestgz file icon ocaml-4.00.1-data-marsh.tgz [^] (2,649 bytes) 2013-03-11 23:29
diff file icon ocaml-4.00.1-data-marsh.diff [^] (9,866 bytes) 2013-03-11 23:57 [Show Content]

- Relationships

-  Notes
gasche (administrator)
2013-03-12 00:03
edited on: 2013-03-12 00:11

I took the liberty to upload the patch independently from the archive, for eventual reviewers that would be more comfortable having a look directly inside Mantis.

I could reproduce the bug with 3.12.1, 32 bits, but not 4.00.1 64 bits: as the data fields should be of the same size (or at least aligned), on a 64 bits machine you need to shorten the list in to two integers rather than four to observe the segfault.

frisch (developer)
2013-07-22 12:45

Important, but too risky to merge just before a release, I'd say.
frisch (developer)
2015-11-30 11:44

I've started to review the code.

- In byterun/startup.c, function caml_main: if I'm not wrong, the patch has the effect of loading the DATA section twice (once to compute the digest, and once to deserialize the value). Instead, one could read once into caml_data and use caml_input_value_from_block to deserialize directly from this block.

- caml_startup_code (i.e. "ocamlc -custom") should be adapted as well. It receives the data block as an argument, so this should be quite easy.

Benoit: would you be as kind as to submit a Github PR rebased on trunk (including changes for the remarks above), trying to add a test to the suite (preferably one that would fail before including on 64-bit)?
bvaugon (developer)
2015-12-04 01:12

Thanks for your remarks.

I just submit a PR on github (see [^]). This PR is for the trunk and contains tests for 32 and 64 bits.

 - The double reading of the DATA segment is now fixed.

 - However, the caml_startup_code function from byterun/startup.c seems to already set the caml_data and caml_data_size variables in the original patch from mantis. So I didn't change that code in the GitHub PR. Maybe I missed something from your second remark... ?
frisch (developer)
2015-12-04 07:36

> However, the caml_startup_code function from byterun/startup.c seems to already set the caml_data and caml_data_size variables in the original patch from mantis.

Indeed, sorry!
shinwell (developer)
2016-12-08 11:58

Downgrading to high priority.

@bvaugon Have you made any further progress on this problem? GPR#330 and GPR#332 have both stalled.
xleroy (administrator)
2017-02-16 11:19

For reference: latest proposal at [^]
xleroy (administrator)
2017-10-09 20:15

This report and the associated Github pull requests have been dormant for a long time. What should we do? I still believe that my proposal is the most reasonable: use an MD5 checksum of the file denoted by Sys.executable_name, computed at run-time the first time we need it. MD5 is fast, don't worry.

- Issue History
Date Modified Username Field Change
2013-03-11 23:29 bvaugon New Issue
2013-03-11 23:29 bvaugon File Added: ocaml-4.00.1-data-marsh.tgz
2013-03-11 23:57 gasche File Added: ocaml-4.00.1-data-marsh.diff
2013-03-12 00:03 gasche Note Added: 0008959
2013-03-12 00:03 gasche Status new => confirmed
2013-03-12 00:11 gasche Note Edited: 0008959 View Revisions
2013-07-12 09:41 doligez Target Version => 4.01.0+dev
2013-07-22 12:45 frisch Note Added: 0009822
2013-07-22 12:45 frisch Target Version 4.01.0+dev => 4.01.1+dev
2013-12-16 14:29 doligez Tag Attached: patch
2014-05-25 20:20 doligez Target Version 4.01.1+dev => 4.02.0+dev
2014-07-17 16:52 doligez Severity minor => major
2014-07-17 16:52 doligez Target Version 4.02.0+dev => 4.02.1+dev
2014-07-17 16:54 doligez Priority normal => high
2014-09-04 00:25 doligez Target Version 4.02.1+dev => undecided
2014-09-24 19:38 doligez Target Version undecided => 4.02.2+dev / +rc1
2015-02-24 23:38 doligez Target Version 4.02.2+dev / +rc1 => 4.02.3+dev
2015-07-15 15:23 doligez Target Version 4.02.3+dev => 4.03.0+dev / +beta1
2015-07-15 15:24 doligez Priority high => urgent
2015-11-30 11:44 frisch Note Added: 0014905
2015-12-04 01:12 bvaugon Note Added: 0015021
2015-12-04 07:36 frisch Note Added: 0015026
2015-12-09 13:59 frisch Target Version 4.03.0+dev / +beta1 => later
2016-12-08 11:58 shinwell Priority urgent => high
2016-12-08 11:58 shinwell Note Added: 0016851
2016-12-08 11:59 shinwell Assigned To => bvaugon
2016-12-08 11:59 shinwell Status confirmed => assigned
2017-02-16 11:19 xleroy Note Added: 0017282
2017-02-16 11:20 xleroy Target Version later => 4.06.0 +dev/beta1/beta2/rc1
2017-02-23 16:43 doligez Category OCaml runtime system => runtime system
2017-03-03 17:45 doligez Category runtime system => runtime system and C interface
2017-10-05 17:45 doligez Assigned To bvaugon =>
2017-10-05 17:45 doligez Priority high => normal
2017-10-05 17:45 doligez Status assigned => confirmed
2017-10-05 17:45 doligez Target Version 4.06.0 +dev/beta1/beta2/rc1 =>
2017-10-09 20:15 xleroy Note Added: 0018522

Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker