From: Matthew Daley <mattd@bugfuzz.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Ian Campbell <Ian.Campbell@citrix.com>,
Xen-devel <xen-devel@lists.xen.org>
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 [thread overview]
Message-ID: <CAD3CaneqXN-ys9FmgkLdOUAakkNLrdL5BtaoQ8VBd4g4rPCs=w@mail.gmail.com> (raw)
In-Reply-To: <CAD3Candp=dxsNyNL85f+TG2jx2K8yerUrXj6vdEsL_oGqpQY0A@mail.gmail.com>
On Sat, Dec 14, 2013 at 12:22 PM, Matthew Daley <mattd@bugfuzz.com> wrote:
> On Sat, Dec 14, 2013 at 5:52 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> 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.
next prev parent reply other threads:[~2013-12-13 23:26 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-01 10:14 [PATCH 00/13] Coverity fixes for libxl Matthew Daley
2013-12-01 10:14 ` [PATCH 01/13] libxl: fix unsigned less-than-0 comparison in e820_sanitize Matthew Daley
2013-12-13 5:54 ` Matthew Daley
2013-12-13 13:23 ` Andrew Cooper
2013-12-13 17:31 ` Ian Jackson
2013-12-01 10:14 ` [PATCH 02/13] libxl: check for xc_domain_setmaxmem failure in libxl__build_pre Matthew Daley
2013-12-02 11:55 ` Ian Jackson
2013-12-02 12:11 ` [PATCH 02/13 v2] " Matthew Daley
2013-12-13 5:53 ` Matthew Daley
2013-12-13 10:17 ` Dario Faggioli
2013-12-13 17:23 ` Ian Jackson
2013-12-01 10:14 ` [PATCH 03/13] libxl: correct file open success check in libxl__device_pci_reset Matthew Daley
2013-12-02 11:57 ` Ian Jackson
2013-12-01 10:14 ` [PATCH 04/13] libxl: don't leak p in libxl__wait_for_backend Matthew Daley
2013-12-01 11:53 ` Andrew Cooper
2013-12-01 23:17 ` Matthew Daley
2013-12-02 0:27 ` [PATCH 04/13 v2] " Matthew Daley
2013-12-02 0:42 ` Andrew Cooper
2013-12-02 0:46 ` Matthew Daley
2013-12-02 0:52 ` Andrew Cooper
2013-12-02 12:00 ` Ian Jackson
2014-01-09 14:51 ` Ian Jackson
2013-12-01 10:14 ` [PATCH 05/13] libxl: remove unsigned less-than-0 comparison Matthew Daley
2013-12-02 12:05 ` Ian Jackson
2013-12-01 10:15 ` [PATCH 06/13] libxl: actually abort if initializing a ctx's lock fails Matthew Daley
2013-12-02 12:05 ` Ian Jackson
2013-12-01 10:15 ` [PATCH 07/13] libxl: don't leak output vcpu info on error in libxl_list_vcpu Matthew Daley
2013-12-02 12:05 ` Ian Jackson
2013-12-01 10:15 ` [PATCH 08/13] libxl: don't leak ptr in libxl_list_vm error case Matthew Daley
2013-12-01 12:20 ` Andrew Cooper
2013-12-02 0:30 ` Matthew Daley
2013-12-02 0:37 ` [PATCH 08/13 v2] " Matthew Daley
2013-12-02 0:39 ` Andrew Cooper
2013-12-02 2:58 ` [PATCH 08/13 v3] " Matthew Daley
2013-12-02 10:35 ` Andrew Cooper
2013-12-02 10:47 ` Matthew Daley
2013-12-02 10:50 ` Ian Campbell
2013-12-02 11:05 ` [PATCH 08/13 v4] " Matthew Daley
2013-12-02 11:10 ` Andrew Cooper
2013-12-02 12:08 ` Ian Jackson
2013-12-02 12:19 ` Matthew Daley
2013-12-02 15:03 ` Ian Jackson
2013-12-03 1:29 ` [PATCH 08/13 v5] " Matthew Daley
2013-12-03 10:21 ` Ian Campbell
2013-12-03 10:30 ` Andrew Cooper
2013-12-13 16:52 ` [PATCH 08/13 v5] libxl: don't leak ptr in libxl_list_vm error case [and 1 more messages] Ian Jackson
2013-12-13 17:05 ` Andrew Cooper
2013-12-13 17:21 ` Ian Jackson
2013-12-13 23:22 ` Matthew Daley
2013-12-13 23:26 ` Matthew Daley [this message]
2013-12-16 11:57 ` Ian Jackson
2013-12-14 1:15 ` [PATCH] xl: check for libxl_list_vm failure in print_uptime Matthew Daley
2013-12-16 11:57 ` Ian Jackson
2013-12-16 11:58 ` Ian Jackson
2013-12-13 5:52 ` [PATCH 08/13 v5] libxl: don't leak ptr in libxl_list_vm error case Matthew Daley
2013-12-01 10:15 ` [PATCH 09/13] libxl: don't leak pcidevs in libxl_pcidev_assignable Matthew Daley
2013-12-02 12:15 ` Ian Jackson
2013-12-01 10:15 ` [PATCH 10/13] libxl: don't try to fclose file twice on error in libxl_userdata_store Matthew Daley
2013-12-02 12:14 ` Ian Jackson
2013-12-02 12:24 ` Matthew Daley
2013-12-02 15:04 ` Ian Jackson
2013-12-02 23:56 ` [PATCH 10/13 v2] " Matthew Daley
2013-12-03 0:00 ` [PATCH 10/13 v3] " Matthew Daley
2013-12-03 17:28 ` Ian Jackson
2013-12-01 10:15 ` [PATCH 11/13] libxl: use pipe instead of temporary file for VNC viewer --autopass Matthew Daley
2013-12-02 12:22 ` Ian Jackson
2013-12-02 12:34 ` Matthew Daley
2013-12-01 10:15 ` [PATCH 12/13] libxl: don't leak buf in libxl_xen_console_read_start error handling Matthew Daley
2013-12-02 12:25 ` Ian Jackson
2013-12-03 1:01 ` [PATCH 12/13 v2] " Matthew Daley
2013-12-03 17:26 ` Ian Jackson
2013-12-01 10:15 ` [PATCH 13/13] libxl: replace for loop with more idiomatic do-while loop Matthew Daley
2013-12-02 12:26 ` Ian Jackson
2013-12-02 12:46 ` Matthew Daley
2013-12-01 12:22 ` [PATCH 00/13] Coverity fixes for libxl Andrew Cooper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD3CaneqXN-ys9FmgkLdOUAakkNLrdL5BtaoQ8VBd4g4rPCs=w@mail.gmail.com' \
--to=mattd@bugfuzz.com \
--cc=Ian.Campbell@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).