From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3 21/28] tools/libxl: Infrastructure for writing a v2 stream Date: Mon, 13 Jul 2015 16:33:53 +0100 Message-ID: <55A3DA61.6020100@citrix.com> References: <1436788907-1921-1-git-send-email-andrew.cooper3@citrix.com> <1436788907-1921-22-git-send-email-andrew.cooper3@citrix.com> <21923.55174.537923.565239@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21923.55174.537923.565239@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Ross Lagerwall , Wei Liu , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On 13/07/15 16:21, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH v3 21/28] tools/libxl: Infrastructure for writing a v2 stream"): > ... >> +void libxl__stream_write_start(libxl__egc *egc, >> + libxl__stream_write_state *stream) >> +{ >> + libxl__datacopier_state *dc = &stream->dc; >> + STATE_AO_GC(stream->ao); >> + struct libxl__sr_hdr hdr = { 0 }; > ... >> + libxl__datacopier_prefixdata(egc, dc, &hdr, sizeof(hdr)); > Sadly this pattern is not correct. I don't think this initialisation > ensures that the memory in hdr is all-bits-0. It guarantees that all object have their default values, which is 0 for PoD integers. (On a POSIX system, it is only floats/doubles/_Complex which have default representations with not all bits 0) >>From the standard, [6.7.8.21] If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration. i.e. everything will get the same value it would have done had it been declared static. > So you may leak > arbitrary data from the toolstack process stack into the migration or > save stream. I am fairly certain that it cannot leak process stack. > > I think you need to use FILLZERO. > >> + struct libxl__sr_rec_hdr rec = { REC_TYPE_LIBXC_CONTEXT }; > ... >> + setup_write(egc, stream, "libxc header", >> + &rec, NULL, libxc_header_done); > Same again. > > (I haven't searched thoroughly for these.) > > >> +/*----- Success/error/cleanup handling. -----*/ > As with read, I would prefer these to be unified into one > stream_complete function. They can't, because of differing function signatures from callbacks. ~Andrew