From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Daley Subject: Re: [PATCH 08/13 v5] libxl: don't leak ptr in libxl_list_vm error case [and 1 more messages] Date: Sat, 14 Dec 2013 12:26:02 +1300 Message-ID: References: <21148.41308.706376.518540@mariner.uk.xensource.com> <1386034144-9572-1-git-send-email-mattd@bugfuzz.com> <1386066096.16012.59.camel@kazak.uk.xensource.com> <21163.15190.993775.174911@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: 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: Andrew Cooper , Stefano Stabellini , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On Sat, Dec 14, 2013 at 12:22 PM, Matthew Daley wrote: > On Sat, Dec 14, 2013 at 5:52 AM, Ian Jackson wrote: >> Ian Campbell writes ("Re: [PATCH 08/13 v5] libxl: don't leak ptr in libxl_list_vm error case"): >>> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote: >>> > + /* >>> > + * Always make sure to allocate at least one element; if we don't and we >>> > + * request zero, libxl__calloc (might) think its internal call to calloc >>> > + * has failed (if it returns null), if so it would kill our process. >> [rewrapped -iwj] >>> >>> Is size==0 something we could/should handle in our libxl__*alloc >>> wrappers? >> >> I think so. I think they should promise that if you pass size==0 you >> get a non-null pointer. Calling realloc with size==0 should crash. >> >> Matthew Daley writes ("Re: [PATCH 08/13 v5] libxl: don't leak ptr in libxl_list_vm error case"): >>> Ping? >> >> See Ian C's comment above, which AFAICT hasn't been answered. > > Sorry, I implicitly agreed with Andrew's comment (at least, the second part...) > > For the record, for custom wrapper functions like these I usually > prefer to stick as close to the existing functions' semantics as > possible, not so much out of love for standards but out of desiring > the absence of surprising behaviour. Although, I suppose changing things wrt. the standards would just introduce new slightly-surprising behaviours vs. having existing really-surprising ones... - Matthew > > As for the second part of the comment, as Andrew already mentioned, > the nb_domains output argument is sufficient to distinguish failure > vs. 0 domains from libxl_list_vm already. > > Out of curiosity, looking through libxl_list_vm's existing callers: > - libxl__domain_make is only (ab)using libxl_list_vm to get the number > of vms via nb_domains... it just frees the vm list right afterward! > - xl_cmdimpl.c:print_uptime doesn't even check libxl_list_vm's result > :D I will patch this shortly. > > (Also, I see you have applied my patch regardless of this discussion; thanks.) > > - Matthew > >> >> Thanks, >> Ian.