From a84df4673ae5d067d57c3a4e2cb08c554d04c233 Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Thu, 15 Jun 2017 05:02:38 -0700 Subject: [PATCH] [flock] truncate locked files explicitly to prevent race conditions Summary: This avoids race conditions when two processes or more try to lock a file for writing. It could be that the process losing the race writes less than the winner, then we get rubbish at the end of the file. Calling `ftruncate(2)` inside the critical section makes sure the contents of the file are erased first. The harmful race was observed in xcodebuild sometimes, as it can call infer on the same file several times in parallel (!). Reviewed By: jberdine Differential Revision: D5209177 fbshipit-source-id: 744169c --- infer/src/backend/PerfStats.ml | 16 +++++++--------- infer/src/base/Logging.ml | 2 +- infer/src/base/Serialization.ml | 24 ++++-------------------- infer/src/base/Utils.ml | 18 ++++++++++++++++++ infer/src/base/Utils.mli | 4 ++++ 5 files changed, 34 insertions(+), 30 deletions(-) diff --git a/infer/src/backend/PerfStats.ml b/infer/src/backend/PerfStats.ml index 912a1bc64..d1ade44b9 100644 --- a/infer/src/backend/PerfStats.ml +++ b/infer/src/backend/PerfStats.ml @@ -11,6 +11,8 @@ open! IStd +module L = Logging + type perf_stats = { rtime : float; utime : float; @@ -147,18 +149,14 @@ let register_report_at_exit file = let json_stats = to_json (stats ()) in try Unix.mkdir_p (Filename.dirname file); - let stats_oc = open_out file in - let stats_fd = Unix.descr_of_out_channel stats_oc in - ignore (Unix.flock stats_fd Unix.Flock_command.lock_exclusive); - Yojson.Basic.pretty_to_channel stats_oc json_stats; - flush stats_oc; - ignore (Unix.flock stats_fd Unix.Flock_command.unlock); - Out_channel.close stats_oc + Utils.write_file_with_locking file ~f:(fun stats_oc -> + Yojson.Basic.pretty_to_channel stats_oc json_stats; + ); with exc -> - Format.eprintf "Info: failed to write stats to %s@\n%s@\n%s@\n%s@." + L.internal_error "Info: failed to write stats to %s@\n%s@\n%s@\n%s@." file (Exn.to_string exc) (Yojson.Basic.pretty_to_string json_stats) (Printexc.get_backtrace ()) with exc -> - Format.eprintf "Info: failed to compute stats for %s@\n%s@\n%s@." + L.internal_error "Info: failed to compute stats for %s@\n%s@\n%s@." file (Exn.to_string exc) (Printexc.get_backtrace ()) ) ("stats reporting in " ^ file) diff --git a/infer/src/base/Logging.ml b/infer/src/base/Logging.ml index ec536459e..7b2955b15 100644 --- a/infer/src/base/Logging.ml +++ b/infer/src/base/Logging.ml @@ -243,7 +243,7 @@ let external_error fmt = log ~to_console:(not Config.quiet) external_error_file_fmts fmt let internal_error fmt = - log ~to_console:(not Config.developer_mode) internal_error_file_fmts fmt + log ~to_console:Config.developer_mode internal_error_file_fmts fmt (** Type of location in ml source: __POS__ *) type ml_loc = string * int * int * int diff --git a/infer/src/base/Serialization.ml b/infer/src/base/Serialization.ml index dd24b60a2..220059a23 100644 --- a/infer/src/base/Serialization.ml +++ b/infer/src/base/Serialization.ml @@ -97,28 +97,12 @@ let create_serializer (key : Key.t) : 'a serializer = (fun () -> retry_exception ~timeout:1.0 ~catch_exn ~f:read ()) (fun () -> In_channel.close inc) in - let write_file_with_locking ?(delete=false) ~do_write fname = - let file_descr = Unix.openfile ~mode:[Unix.O_WRONLY; Unix.O_CREAT] fname in - let outc = Unix.out_channel_of_descr file_descr in - if Unix.flock file_descr Unix.Flock_command.lock_exclusive - then - begin - do_write outc; - flush outc; - ignore (Unix.flock file_descr Unix.Flock_command.unlock); - end; - Out_channel.close outc; - if delete - then - try Unix.unlink fname with - | Unix.Unix_error _ -> () in - let write_to_tmp_file fname data = let fname_tmp = Filename.temp_file ~in_dir:(Filename.dirname fname) (Filename.basename fname) ".tmp" in - write_file_with_locking + Utils.write_file_with_locking fname_tmp - ~do_write:(fun outc -> Marshal.to_channel outc (key, version, data) []); + ~f:(fun outc -> Marshal.to_channel outc (key, version, data) []); fname_tmp in (* The .lock file is used to synchronize the writers. @@ -128,10 +112,10 @@ let create_serializer (key : Key.t) : 'a serializer = let fname_str = DB.filename_to_string fname in let fname_str_lock = fname_str ^ ".lock" in - write_file_with_locking + Utils.write_file_with_locking fname_str_lock ~delete:true - ~do_write:(fun _outc -> + ~f:(fun _outc -> let (data_to_write : 'a) = match cmd with | Replace data -> data diff --git a/infer/src/base/Utils.ml b/infer/src/base/Utils.ml index b818bd3dc..c47876967 100644 --- a/infer/src/base/Utils.ml +++ b/infer/src/base/Utils.ml @@ -297,3 +297,21 @@ let compare_versions v1 v2 = let lv1 = int_list_of_version v1 in let lv2 = int_list_of_version v2 in [%compare : int list] lv1 lv2 + +let write_file_with_locking ?(delete=false) ~f:do_write fname = + Unix.with_file ~mode:Unix.[O_WRONLY; O_CREAT] fname ~f:(fun file_descr -> + if Unix.flock file_descr Unix.Flock_command.lock_exclusive then ( + (* make sure we're not writing over some existing, possibly longer content: some other + process may have snagged the file from under us between open(2) and flock(2) so passing + O_TRUNC to open(2) above would not be a good substitute for calling ftruncate(2) + below. *) + Unix.ftruncate file_descr ~len:Int64.zero; + let outc = Unix.out_channel_of_descr file_descr in + do_write outc; + flush outc; + ignore (Unix.flock file_descr Unix.Flock_command.unlock) + ); + ); + if delete then + try Unix.unlink fname with + | Unix.Unix_error _ -> () diff --git a/infer/src/base/Utils.mli b/infer/src/base/Utils.mli index 0a06e0995..c1bdc2fc9 100644 --- a/infer/src/base/Utils.mli +++ b/infer/src/base/Utils.mli @@ -85,3 +85,7 @@ val suppress_stderr2 : ('a -> 'b -> 'c) -> 'a -> 'b -> 'c -1 if v1 is older than v2 and 0 if they are the same version. The versions are strings of the shape "n.m.t", the order is lexicographic. *) val compare_versions : string -> string -> int + +(** Lock file passed as argument and write into it using [f]. If [delete] then the file is unlinked + once this is done. *) +val write_file_with_locking : ?delete:bool -> f:(out_channel -> unit) -> string -> unit