Re: [patch] Fix resource leak in netcat
Ping tech@,
Any developer can comment on this patch? I think it is indeed a bug.
Thanks!
On 9/30/2018 11:55 AM, Nan Xiao wrote:
> Hi tech@,
>
> The following patch fixed the resource leak when netcat works as a TLS
> server. Sorry if I am wrong, thanks!
>
>
> Index: netcat.c
> ===
> RCS file: /cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.194
> diff -u -p -r1.194 netcat.c
> --- netcat.c 7 Sep 2018 09:55:29 - 1.194
> +++ netcat.c 30 Sep 2018 03:50:03 -
> @@ -633,6 +633,11 @@ main(int argc, char *argv[])
> if (!kflag)
> break;
> }
> +
> + if (tls_ctx) {
> + tls_free(tls_ctx);
> + tls_ctx = NULL;
> + }
> } else if (family == AF_UNIX) {
> ret = 0;
>
--
Best Regards
Nan Xiao
Re: pfsync: avoid a recursion on PF_LOCK
On Thu, Sep 27, 2018 at 06:34:45PM +0200, Alexandr Nedvedicky wrote:
> OK to pfsync change?
OK bluhm@, just two style nits
> + if ((e = ip_output(m, NULL, NULL, IP_RAWOUTPUT, >sc_imo,
> + NULL, 0)) == 0)
Usually we call the error variable "error".
> + if (mq_enqueue(_mq, m) != 0) {
> + pfsyncstat_inc(pfsyncs_oerrors);
> + DPFPRINTF(LOG_DEBUG, "mq_enqueue() @ %s failed, queue full\n",
> + __func__);
> + }
> + else
The } and else should be on the same line.
Re: [patch] Fix resource leak in netcat
On Sun, Sep 30, 2018 at 11:55:58AM +0800, Nan Xiao wrote:
> The following patch fixed the resource leak when netcat works as a TLS
> server. Sorry if I am wrong, thanks!
There is another tls leak on the client side after
if (s == -1)
continue;
To fix both I would do the tls_free() after close() consistently.
bluhm
Index: usr.bin/nc/netcat.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.194
diff -u -p -r1.194 netcat.c
--- usr.bin/nc/netcat.c 7 Sep 2018 09:55:29 - 1.194
+++ usr.bin/nc/netcat.c 2 Oct 2018 00:35:03 -
@@ -543,8 +543,6 @@ main(int argc, char *argv[])
err(1, "pledge");
}
if (lflag) {
- struct tls *tls_cctx = NULL;
- int connfd;
ret = 0;
if (family == AF_UNIX) {
@@ -603,6 +601,9 @@ main(int argc, char *argv[])
readwrite(s, NULL);
} else {
+ struct tls *tls_cctx = NULL;
+ int connfd;
+
len = sizeof(cliaddr);
connfd = accept4(s, (struct sockaddr *),
, SOCK_NONBLOCK);
@@ -618,12 +619,10 @@ main(int argc, char *argv[])
readwrite(connfd, tls_cctx);
if (!usetls)
readwrite(connfd, NULL);
- if (tls_cctx) {
+ if (tls_cctx)
timeout_tls(s, tls_cctx, tls_close);
- tls_free(tls_cctx);
- tls_cctx = NULL;
- }
close(connfd);
+ tls_free(tls_cctx);
}
if (family == AF_UNIX && uflag) {
if (connect(s, NULL, 0) < 0)
@@ -657,6 +656,8 @@ main(int argc, char *argv[])
for (s = -1, i = 0; portlist[i] != NULL; i++) {
if (s != -1)
close(s);
+ tls_free(tls_ctx);
+ tls_ctx = NULL;
if (usetls) {
if ((tls_ctx = tls_client()) == NULL)
@@ -707,18 +708,15 @@ main(int argc, char *argv[])
tls_setup_client(tls_ctx, s, host);
if (!zflag)
readwrite(s, tls_ctx);
- if (tls_ctx) {
+ if (tls_ctx)
timeout_tls(s, tls_ctx, tls_close);
- tls_free(tls_ctx);
- tls_ctx = NULL;
- }
}
}
}
if (s != -1)
close(s);
-
+ tls_free(tls_ctx);
tls_config_free(tls_cfg);
return ret;
Re: Qcow2: External snapshots
Hi Ori,
On Sun, Sep 30, 2018 at 12:27:00PM -0700, Ori Bernstein wrote:
> I've added support to vmd for external snapshots. That is,
> snapshots that are derived from a base image. Data lookups
> start in the derived image, and if the derived image does not
> contain some data, the search proceeds ot the base image.
> Multiple derived images may exist off of a single base image.
>
Nice work! This will be quite useful, thanks.
I think I broke your diff as my last commit to derive the raw/qcow2
format introduced some conflicts. I had posted it on hackers@ and
forgot that your aren't on the internal list yet - sorry for that.
> A limitation of this format is that modifying the base image
> will corrupt the derived image.
>
> This change also adds support for creating disk derived disk
> images to vmctl. To use it:
>
> vmctl create derived.img -s 16G -b base.img -f qcow2
>
I removed -f fmt to be more consistent and the new syntax will be
vmctl create qcow2:derived.img -s 16G -b base.img
or
vmctl create derived.qcow2 -s 16G -b base.img
but we should be able to derive it from the base as well (there's now
base in raw images), so the following should work as well:
vmctl create derived.img -s 16G -b base.img
> The main implementation change is that we now probe base
> images before sending the disk FDs to the VM, which means that
> we can actually open the images.
>
> The base image paths may be relative. If they are relative,
> they are interpreted relative to the location of the derived
> image, and not relative to the directory where vmd happens to
> be running.
>
OK, that needs some care + review.
> For review, a bit of scrutiny could be directed to the
> messaging. It relies on imsg being in-order, which seems to
> be the case, but isn't documented in the manpage -- If I can't
> rely on that, the protocol needs to be tweaked.
>
imsgs are guaranteed to be in order as long as you don't mux them with
other messages from the same sender in an async way.
> After this change, we send imsgs to the same disk index
> repeatedly, and each message adds another base to the stack of
> images. So, for example, if I have 2images image that look
> like this:
>
> disk0 -> base0 -> base1
> disk1
>
> Then we send the following messages:
>
> VMDOP_START_VM_DISK (i=0, fd=open(disk0))
> VMDOP_START_VM_DISK (i=0, fd=open(base0))
> VMDOP_START_VM_DISK (i=0, fd=open(base1))
>
> VMDOP_START_VM_DISK (i=1, fd=open(disk1))
>
Makes sense.
> This also opens the door to ephemeral snapshots, which vmd can
> implicitly create when it starts a vm, and removes
> automatically on exit.
>
Please be extremely careful with the design here. Unlike qemu, a vmd
VM is not able to create new files itself and it should never be able
to do it. So when we create snapshots, we need to find a way that the
parent prepares the file, sends the fd, and asks the VM process to use
it.
> Testing has been the usual -- OpenBSD installs, a bit of catting,
> and some random 'dd'. Heavier use and testing would be appreciated.
>
I will test the updated diff that includes the second fix and the merge ;)
Initial comments inline below.
Reyk
>
>
> diff --git regress/usr.sbin/vmd/diskfmt/Makefile
> regress/usr.sbin/vmd/diskfmt/Makefile
> index c2a5f42d5f6..1f8673e0e26 100644
> --- regress/usr.sbin/vmd/diskfmt/Makefile
> +++ regress/usr.sbin/vmd/diskfmt/Makefile
> @@ -11,7 +11,7 @@
> VMD_DIR=$(BSDSRCDIR)/usr.sbin/vmd/
>
> PROG=vioscribble
> -SRCS=vioscribble.c $(VMD_DIR)/vioqcow2.c $(VMD_DIR)/vioraw.c
> +SRCS=vioscribble.c vioqcow2.c vioraw.c
> CFLAGS+=-I$(VMD_DIR) -pthread
> LDFLAGS+=-pthread
>
> @@ -26,3 +26,6 @@ scribble-images:
> .PHONY: ${REGRESS_TARGETS} scribble-images
>
> .include
> +
> +vioqcow2.c vioraw.c: $(VMD_DIR)/vioqcow2.c $(VMD_DIR)/vioraw.c
> + cp $(VMD_DIR)/vioqcow2.c $(VMD_DIR)/vioraw.c .
> diff --git regress/usr.sbin/vmd/diskfmt/vioscribble.c
> regress/usr.sbin/vmd/diskfmt/vioscribble.c
> index 14d720db652..1da8efedac7 100644
> --- regress/usr.sbin/vmd/diskfmt/vioscribble.c
> +++ regress/usr.sbin/vmd/diskfmt/vioscribble.c
> @@ -122,16 +122,18 @@ main(int argc, char **argv)
> verbose = !!getenv("VERBOSE");
> qcfd = open("scribble.qc2", O_RDWR);
> rawfd = open("scribble.raw", O_RDWR);
> - if (qcfd == -1 || virtio_init_qcow2(, , qcfd) == -1)
> + if (qcfd == -1)
> err(1, "unable to open qcow");
> - if (rawfd == -1 || virtio_init_raw(, , rawfd) == -1)
> + if (virtio_init_qcow2(, , , 1) == -1)
> + err(1, "unable to init qcow");
> + if (rawfd == -1 || virtio_init_raw(, , , 1) == -1)
> err(1, "unable to open raw");
>
> srandom_deterministic(123);
>
> /* scribble to both disks */
> printf("scribbling...\n");
> - for (i = 0; i < 16; i++) {
> + for (i = 0; i < 1024*16; i++) {
> off = (random() % DISKSZ);
>
Re: [PATCH] bgpd: expose ROA origin validation state in show rib
On Sun, Sep 30, 2018 at 01:49:45AM +, Job Snijders wrote:
> Dear all,
>
> This small patch exposes the origin validation state in 'bgpctl show
> rib' and 'bgpctl show rib detail'. This will help debugging, and draw
> attention to routing problems.
>
> I know we're weary of spending horizontal space, but I think spending 3
> chars to show the OV state (and as such make it a first class citizen)
> will be worth it. When I tried putting the ovs state in the 'flags'
> column it just didn't look right.
While I agree that mixing the flags and ovs together is not ideal but I
wonder if we can only spend 1 char instead of the 3 (two spaces and the
ovs state). E.g. by reducing the header for 'flags'
Also the 2nd detail line is now getting rather long, wonder if we should
split it up.
The diff is OK claudio@ (the output can be fixed in tree if there is a
good proposal).
> Example output:
>
> $ bgpctl show rib
> flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
>S = Stale, E = Error
> origin validation state: N = not-found, V = valid, ! = invalid
> origin: i = IGP, e = EGP, ? = Incomplete
>
> flags ovs destination gateway lpref med aspath origin
> *> V 192.0.2.0/24 100.64.2.3 100 0 2 i
> *> ! 192.0.2.0/26 100.64.2.3 100 0 2 i
> $ bgpctl show rib detail
>
> BGP routing table entry for 192.0.2.0/24
> 2
> Nexthop 100.64.2.3 (via 100.64.2.3) from 100.64.2.3 (192.147.168.1)
> Origin IGP, metric 0, localpref 100, weight 0, ovs valid, external,
> valid, best
> Last update: 00:02:19 ago
>
> BGP routing table entry for 192.0.2.0/26
> 2
> Nexthop 100.64.2.3 (via 100.64.2.3) from 100.64.2.3 (192.147.168.1)
> Origin IGP, metric 0, localpref 100, weight 0, ovs invalid, external,
> valid, best
> Last update: 00:02:19 ago
>
> OK?
>
> Kind regards,
>
> Job
>
> diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
> index 7275ea6a2c4..70648ed8f5b 100644
> --- usr.sbin/bgpctl/bgpctl.c
> +++ usr.sbin/bgpctl/bgpctl.c
> @@ -72,8 +72,9 @@ int show_nexthop_msg(struct imsg *);
> void show_interface_head(void);
> int show_interface_msg(struct imsg *);
> void show_rib_summary_head(void);
> -void print_prefix(struct bgpd_addr *, u_int8_t, u_int8_t);
> +void print_prefix(struct bgpd_addr *, u_int8_t, u_int8_t, u_int8_t);
> const char * print_origin(u_int8_t, int);
> +const char * print_ovs(u_int8_t, int);
> void print_flags(u_int8_t, int);
> int show_rib_summary_msg(struct imsg *);
> int show_rib_detail_msg(struct imsg *, int, int);
> @@ -183,6 +184,7 @@ main(int argc, char *argv[])
> ribreq.neighbor = neighbor;
> ribreq.aid = res->aid;
> ribreq.flags = res->flags;
> + ribreq.validation_state = res->validation_state;
> show_mrt.arg =
> if (!(res->flags & F_CTL_DETAIL))
> show_rib_summary_head();
> @@ -1183,17 +1185,20 @@ show_rib_summary_head(void)
> {
> printf("flags: * = Valid, > = Selected, I = via IBGP, A = Announced,\n"
> " S = Stale, E = Error\n");
> + printf("origin validation state: N = not-found, V = valid, ! =
> invalid\n");
> printf("origin: i = IGP, e = EGP, ? = Incomplete\n\n");
> - printf("%-5s %-20s %-15s %5s %5s %s\n", "flags", "destination",
> + printf("%-5s %3s %-20s %-15s %5s %5s %s\n", "flags", "ovs",
> "destination",
> "gateway", "lpref", "med", "aspath origin");
> }
>
> void
> -print_prefix(struct bgpd_addr *prefix, u_int8_t prefixlen, u_int8_t flags)
> +print_prefix(struct bgpd_addr *prefix, u_int8_t prefixlen, u_int8_t flags,
> +u_int8_t ovs)
> {
> char*p;
>
> print_flags(flags, 1);
> + printf("%3s ", print_ovs(ovs, 1));
> if (asprintf(, "%s/%u", log_addr(prefix), prefixlen) == -1)
> err(1, NULL);
> printf("%-20s", p);
> @@ -1252,6 +1257,19 @@ print_flags(u_int8_t flags, int sum)
> }
> }
>
> +const char *
> +print_ovs(u_int8_t validation_state, int sum)
> +{
> + switch (validation_state) {
> + case ROA_INVALID:
> + return (sum ? "!" : "invalid");
> + case ROA_VALID:
> + return (sum ? "V" : "valid");
> + default:
> + return (sum ? "N" : "not-found");
> + }
> +}
> +
> int
> show_rib_summary_msg(struct imsg *imsg)
> {
> @@ -1309,7 +1327,7 @@ show_rib_brief(struct ctl_show_rib *r, u_char *asdata)
> {
> char*aspath;
>
> - print_prefix(>prefix, r->prefixlen, r->flags);
> + print_prefix(>prefix, r->prefixlen, r->flags, r->validation_state);
> printf(" %-15s ", log_addr(>exit_nexthop));
> printf(" %5u %5u ", r->local_pref, r->med);
>
> @@ -1346,8 +1364,9 @@
Re: vmd losing VMs
Thanks Mike. I've upgraded from Sep 27th to Sep 29th snapshot and so far I haven't seen the problem with: OpenBSD 6.4-beta (GENERIC.MP) #336: Sat Sep 29 08:13:15 MDT 2018 dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP pd@ is saying he's not responsible for the fix, but maybe something by reyk@ is? I will apply the debugging tools should the problem recur. BTW, the memory copy-pasto fix is working well. I was prevented from running 4x2G VMs ;) Thanks Greg -- nest.cx is Gmail hosted, use PGP for anything private. Key: http://goo.gl/6dMsr Fingerprint: 5E2B 2D0E 1E03 2046 BEC3 4D50 0B15 42BD 8DF5 A1B0
Re: Qcow2: External snapshots
On Mon, 1 Oct 2018 12:55:12 +0200 Reyk Floeter wrote: > Hi Ori, > > On Sun, Sep 30, 2018 at 12:27:00PM -0700, Ori Bernstein wrote: > > I've added support to vmd for external snapshots. That is, > > snapshots that are derived from a base image. Data lookups > > start in the derived image, and if the derived image does not > > contain some data, the search proceeds ot the base image. > > Multiple derived images may exist off of a single base image. > > > > Nice work! This will be quite useful, thanks. > > I think I broke your diff as my last commit to derive the raw/qcow2 > format introduced some conflicts. I had posted it on hackers@ and > forgot that your aren't on the internal list yet - sorry for that. No worries -- I'll rebase it after work. > but we should be able to derive it from the base as well (there's now > base in raw images), so the following should work as well: > > vmctl create derived.img -s 16G -b base.img Will fold that in to this change. I also want the creation to automatically figure out the size here. > > The main implementation change is that we now probe base > > images before sending the disk FDs to the VM, which means that > > we can actually open the images. > > > > The base image paths may be relative. If they are relative, > > they are interpreted relative to the location of the derived > > image, and not relative to the directory where vmd happens to > > be running. > > > > OK, that needs some care + review. Indeed -- I hadn't gotten it quite right. > Please be extremely careful with the design here. Unlike qemu, a vmd > VM is not able to create new files itself and it should never be able > to do it. So when we create snapshots, we need to find a way that the > parent prepares the file, sends the fd, and asks the VM process to use > it. I was not planning on allowing snapshots of running images -- merely starting instances where all changes were transient. My plan was to make vmctl create a temporary disk image, and point vmd to that. -- Ori Bernstein
Re: Qcow2: External snapshots
Uh-oh... Don't mention hackers@ on tech@... (FWIW) :-)
/Alexander
On October 1, 2018 12:55:12 PM GMT+02:00, Reyk Floeter wrote:
>Hi Ori,
>
>On Sun, Sep 30, 2018 at 12:27:00PM -0700, Ori Bernstein wrote:
>> I've added support to vmd for external snapshots. That is,
>> snapshots that are derived from a base image. Data lookups
>> start in the derived image, and if the derived image does not
>> contain some data, the search proceeds ot the base image.
>> Multiple derived images may exist off of a single base image.
>>
>
>Nice work! This will be quite useful, thanks.
>
>I think I broke your diff as my last commit to derive the raw/qcow2
>format introduced some conflicts. I had posted it on hackers@ and
>forgot that your aren't on the internal list yet - sorry for that.
>
>> A limitation of this format is that modifying the base image
>> will corrupt the derived image.
>>
>> This change also adds support for creating disk derived disk
>> images to vmctl. To use it:
>>
>> vmctl create derived.img -s 16G -b base.img -f qcow2
>>
>
>I removed -f fmt to be more consistent and the new syntax will be
>
> vmctl create qcow2:derived.img -s 16G -b base.img
>
>or
>
> vmctl create derived.qcow2 -s 16G -b base.img
>
>but we should be able to derive it from the base as well (there's now
>base in raw images), so the following should work as well:
>
> vmctl create derived.img -s 16G -b base.img
>
>> The main implementation change is that we now probe base
>> images before sending the disk FDs to the VM, which means that
>> we can actually open the images.
>>
>> The base image paths may be relative. If they are relative,
>> they are interpreted relative to the location of the derived
>> image, and not relative to the directory where vmd happens to
>> be running.
>>
>
>OK, that needs some care + review.
>
>> For review, a bit of scrutiny could be directed to the
>> messaging. It relies on imsg being in-order, which seems to
>> be the case, but isn't documented in the manpage -- If I can't
>> rely on that, the protocol needs to be tweaked.
>>
>
>imsgs are guaranteed to be in order as long as you don't mux them with
>other messages from the same sender in an async way.
>
>> After this change, we send imsgs to the same disk index
>> repeatedly, and each message adds another base to the stack of
>> images. So, for example, if I have 2images image that look
>> like this:
>>
>> disk0 -> base0 -> base1
>> disk1
>>
>> Then we send the following messages:
>>
>> VMDOP_START_VM_DISK (i=0, fd=open(disk0))
>> VMDOP_START_VM_DISK (i=0, fd=open(base0))
>> VMDOP_START_VM_DISK (i=0, fd=open(base1))
>>
>> VMDOP_START_VM_DISK (i=1, fd=open(disk1))
>>
>
>Makes sense.
>
>> This also opens the door to ephemeral snapshots, which vmd can
>> implicitly create when it starts a vm, and removes
>> automatically on exit.
>>
>
>Please be extremely careful with the design here. Unlike qemu, a vmd
>VM is not able to create new files itself and it should never be able
>to do it. So when we create snapshots, we need to find a way that the
>parent prepares the file, sends the fd, and asks the VM process to use
>it.
>
>> Testing has been the usual -- OpenBSD installs, a bit of catting,
>> and some random 'dd'. Heavier use and testing would be appreciated.
>>
>
>I will test the updated diff that includes the second fix and the merge
>;)
>
>Initial comments inline below.
>
>Reyk
>
>>
>>
>> diff --git regress/usr.sbin/vmd/diskfmt/Makefile
>regress/usr.sbin/vmd/diskfmt/Makefile
>> index c2a5f42d5f6..1f8673e0e26 100644
>> --- regress/usr.sbin/vmd/diskfmt/Makefile
>> +++ regress/usr.sbin/vmd/diskfmt/Makefile
>> @@ -11,7 +11,7 @@
>> VMD_DIR=$(BSDSRCDIR)/usr.sbin/vmd/
>>
>> PROG=vioscribble
>> -SRCS=vioscribble.c $(VMD_DIR)/vioqcow2.c $(VMD_DIR)/vioraw.c
>> +SRCS=vioscribble.c vioqcow2.c vioraw.c
>> CFLAGS+=-I$(VMD_DIR) -pthread
>> LDFLAGS+=-pthread
>>
>> @@ -26,3 +26,6 @@ scribble-images:
>> .PHONY: ${REGRESS_TARGETS} scribble-images
>>
>> .include
>> +
>> +vioqcow2.c vioraw.c: $(VMD_DIR)/vioqcow2.c $(VMD_DIR)/vioraw.c
>> +cp $(VMD_DIR)/vioqcow2.c $(VMD_DIR)/vioraw.c .
>> diff --git regress/usr.sbin/vmd/diskfmt/vioscribble.c
>regress/usr.sbin/vmd/diskfmt/vioscribble.c
>> index 14d720db652..1da8efedac7 100644
>> --- regress/usr.sbin/vmd/diskfmt/vioscribble.c
>> +++ regress/usr.sbin/vmd/diskfmt/vioscribble.c
>> @@ -122,16 +122,18 @@ main(int argc, char **argv)
>> verbose = !!getenv("VERBOSE");
>> qcfd = open("scribble.qc2", O_RDWR);
>> rawfd = open("scribble.raw", O_RDWR);
>> -if (qcfd == -1 || virtio_init_qcow2(, , qcfd) == -1)
>> +if (qcfd == -1)
>> err(1, "unable to open qcow");
>> -if (rawfd == -1 || virtio_init_raw(, , rawfd) == -1)
>> +if (virtio_init_qcow2(, , , 1) == -1)
>> +err(1, "unable to init qcow");
>> +if (rawfd == -1 || virtio_init_raw(, , , 1) ==
>-1)
>> err(1, "unable to open raw");
>>
