Re: [patch] Fix resource leak in netcat
Hi bluhm,
Thanks very much for your reply!
Yes, your patch covers all the corner cases, thanks!
On 10/2/2018 8:38 AM, Alexander Bluhm wrote:
> 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;
>
--
Best Regards
Nan Xiao(肖楠)
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
[patch] Fix resource leak in netcat
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.c7 Sep 2018 09:55:29 - 1.194
+++ netcat.c30 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: [patch]Modify the example code in write(2) manual
Hi Ingo, Thanks very much for your time and detailed explanation! I have one more question: If write(2) indeed returns 0, the write(2) won't set errno variable, ring? Because from the manual, the errno is set only when the return value is -1. If this is true, the errno's value should be set by last system call which failed, and has no relation with current write(2). So it is not guaranteed that following words will be print: program: write: Undefined error: 0 It may print a totally unrelated description which can mislead the user. Does my concern make sense? Please correct me if I am wrong, thanks very much in advance! On 9/26/2018 10:33 PM, Ingo Schwarze wrote: > Hi, > > Nan Xiao wrote on Wed, Sep 26, 2018 at 09:42:02PM +0800: > >> Any developer can comment on this patch? Thanks! > > I think this change is a bad idea and should not be committed. > > No matter whether or not it can happen on OpenBSD, *if* some > implementation of write(2) sometimes returns 0 even for nbytes > 0, > your change is likely to cause an infinite busy loop, which is > quite bad. > > In that case, the current code would print: > > program: write: Undefined error: 0 > > That's admittedly not totally informative, but we can't do much > better because it is indeed not quite clear which kind of problem > the return value of 0 might indicate. And in any case, that edge > case seems uncommon enough that complicating the example code to > handle the case of a return value of zero separately would also > seem like a bad idea. > > Besides, the idiom documented here is actually widely used in the > tree, and it is indeed recommended, see for example > > /usr/src/bin/cat/cat.c > > Yours, > Ingo > > >> I am reading write(2) manual, and come across the following example: >> >> for (off = 0; off < bsz; off += nw) >> if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) >> err(1, "write"); >> >> I am just wondering when the write(2) will return 0? If in some cases, >> it will indeed return 0, according to the manual: >> >> Upon successful completion the number of bytes which were written is >> returned. Otherwise, a -1 is returned and the global variable errno is >> set to indicate the error. >> >> Because the errno is only set when return value is -1, if write(2) >> returns 0, the errno should be an undefined value, and "err(1, >> "write");" also won't print correct information. >> >> If write(2) won't return 0, my following patch fixes the example code: >> >> diff --git write.2 write.2 >> index c1686b1a910..db134959002 100644 >> --- write.2 >> +++ write.2 >> @@ -164,7 +164,7 @@ ssize_t nw; >> int d; >> >> for (off = 0; off < bsz; off += nw) >> -if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) >> +if ((nw = write(d, buf + off, bsz - off)) == -1) >> err(1, "write"); >> .Ed >> .Sh ERRORS -- Best Regards Nan Xiao(肖楠)
Re: [patch]Modify the example code in write(2) manual
ping tech@, Any developer can comment on this patch? Thanks! On 9/25/2018 10:10 PM, Nan Xiao wrote: > Hi tech@, > > I am reading write(2) manual, and come across the following example: > > for (off = 0; off < bsz; off += nw) > if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) > err(1, "write"); > > I am just wondering when the write(2) will return 0? If in some cases, > it will indeed return 0, according to the manual: > >> Upon successful completion the number of bytes which were written is > returned. Otherwise, a -1 is returned and the global variable errno is > set to indicate the error. > > Because the errno is only set when return value is -1, if write(2) > returns 0, the errno should be an undefined value, and "err(1, > "write");" also won't print correct information. > > If write(2) won't return 0, my following patch fixes the example code: > > diff --git write.2 write.2 > index c1686b1a910..db134959002 100644 > --- write.2 > +++ write.2 > @@ -164,7 +164,7 @@ ssize_t nw; > int d; > > for (off = 0; off < bsz; off += nw) > - if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) > + if ((nw = write(d, buf + off, bsz - off)) == -1) > err(1, "write"); > .Ed > .Sh ERRORS > > Thanks! > -- Best Regards Nan Xiao(肖楠)
[patch]Modify the example code in write(2) manual
Hi tech@, I am reading write(2) manual, and come across the following example: for (off = 0; off < bsz; off += nw) if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) err(1, "write"); I am just wondering when the write(2) will return 0? If in some cases, it will indeed return 0, according to the manual: > Upon successful completion the number of bytes which were written is returned. Otherwise, a -1 is returned and the global variable errno is set to indicate the error. Because the errno is only set when return value is -1, if write(2) returns 0, the errno should be an undefined value, and "err(1, "write");" also won't print correct information. If write(2) won't return 0, my following patch fixes the example code: diff --git write.2 write.2 index c1686b1a910..db134959002 100644 --- write.2 +++ write.2 @@ -164,7 +164,7 @@ ssize_t nw; int d; for (off = 0; off < bsz; off += nw) - if ((nw = write(d, buf + off, bsz - off)) == 0 || nw == -1) + if ((nw = write(d, buf + off, bsz - off)) == -1) err(1, "write"); .Ed .Sh ERRORS Thanks! -- Best Regards Nan Xiao
Re: [patch] Add IPv6 description for `-T' option in netcat manual
Hi Jason,
According to IPv6(https://en.wikipedia.org/wiki/IPv6_packet) and IPv4
TOS(https://en.wikipedia.org/wiki/Type_of_service), Traffic class and
TOS seem have the same structure: the first six bits are DS field and
the last two bits for Explicit Congestion Notification.
Thanks!
Best Regards
Nan Xiao
On Tue, Sep 25, 2018 at 2:37 PM Jason McIntyre wrote:
>
> On Tue, Sep 25, 2018 at 09:11:42AM +0800, Nan Xiao wrote:
> > Hi tech@,
> >
>
> morning.
>
> > According to netcat source code, the `-T' option not only takes effect
> > in IPv4 but also IPv6:
> >
> > if (Tflag != -1) {
> > if (af == AF_INET && setsockopt(s, IPPROTO_IP,
> > IP_TOS, , sizeof(Tflag)) == -1)
> > err(1, "set IP ToS");
> >
> > else if (af == AF_INET6 && setsockopt(s, IPPROTO_IPV6,
> > IPV6_TCLASS, , sizeof(Tflag)) == -1)
> > err(1, "set IPv6 traffic class");
> > }
> >
> > So I think the following patch should be more accurate:
> >
> > diff --git nc.1 nc.1
> > index cb391288f15..5588daf87ae 100644
> > --- nc.1
> > +++ nc.1
> > @@ -241,7 +241,7 @@ Cannot be used together with
> > or
> > .Fl x .
> > .It Fl T Ar keyword
> > -Change the IPv4 TOS value or the TLS options.
> > +Change the IPv4 TOS/IPv6 Traffic Class value or the TLS options.
> > .Pp
> > For TLS options,
> > .Ar keyword
> > @@ -269,7 +269,7 @@ for further details).
> > Specifying TLS options requires
> > .Fl c .
> > .Pp
> > -For the IPv4 TOS value,
> > +For the IPv4 TOS/IPv6 Traffic Class value,
> > .Ar keyword
> > may be one of
>
> one question: are the keywords identical for both v4 and 6?
>
> if so, anyone want to ok this?
> if not, the diff needs work.
>
> also, i think traffic class should be lower case.
>
> jmc
>
> > .Cm critical ,
> >
> >
> > Thanks!
> >
> > --
> > Best Regards
> > Nan Xiao
> >
>
[patch] Add IPv6 description for `-T' option in netcat manual
Hi tech@,
According to netcat source code, the `-T' option not only takes effect
in IPv4 but also IPv6:
if (Tflag != -1) {
if (af == AF_INET && setsockopt(s, IPPROTO_IP,
IP_TOS, , sizeof(Tflag)) == -1)
err(1, "set IP ToS");
else if (af == AF_INET6 && setsockopt(s, IPPROTO_IPV6,
IPV6_TCLASS, , sizeof(Tflag)) == -1)
err(1, "set IPv6 traffic class");
}
So I think the following patch should be more accurate:
diff --git nc.1 nc.1
index cb391288f15..5588daf87ae 100644
--- nc.1
+++ nc.1
@@ -241,7 +241,7 @@ Cannot be used together with
or
.Fl x .
.It Fl T Ar keyword
-Change the IPv4 TOS value or the TLS options.
+Change the IPv4 TOS/IPv6 Traffic Class value or the TLS options.
.Pp
For TLS options,
.Ar keyword
@@ -269,7 +269,7 @@ for further details).
Specifying TLS options requires
.Fl c .
.Pp
-For the IPv4 TOS value,
+For the IPv4 TOS/IPv6 Traffic Class value,
.Ar keyword
may be one of
.Cm critical ,
Thanks!
--
Best Regards
Nan Xiao
Re: [patch] Fix "Address already in use" issue when using netcat with UNIX-domain socket
ping tech@,
Very sorry for interrupting again! Anyone can give comment on this
issue? Thanks!
On 9/18/2018 6:37 PM, Nan Xiao wrote:
> Hi tech@,
>
> Assume I use netcat with UNIX-domain socket, and there is no
> temp_socket. Launch the server:
>
> # ./nc -U -l temp_socket
>
> It works normally. But after netcat exits, launch it again:
>
> # nc -U -l temp_socket
> nc: Address already in use
>
> The only method seems to delete temp_socket.
>
> I am not sure this behavior is as expected, and come out following patch
> may fix this issue, thanks!
>
> diff --git usr.bin/nc/netcat.c usr.bin/nc/netcat.c
> index 341e7e50485..3b2150a01dc 100644
> --- usr.bin/nc/netcat.c
> +++ usr.bin/nc/netcat.c
> @@ -749,6 +749,9 @@ unix_bind(char *path, int flags)
> return -1;
> }
>
> + if (lflag)
> + unlink(path);
> +
> if (bind(s, (struct sockaddr *)_un, sizeof(s_un)) < 0) {
> save_errno = errno;
> close(s);
>
--
Best Regards
Nan Xiao(肖楠)
Re: Maybe need to enrich `-T' option in netcat manual
Hi Jason,
Thanks very much for your response!
I check the ping & traceroute code, For ping:
if (options & F_TTL) {
if (IN_MULTICAST(ntohl(dst4.sin_addr.s_addr)))
moptions |= MULTICAST_TTL;
else
options |= F_HDRINCL;
}
For traceroute:
void
check_tos(struct ip *ip, int *last_tos)
{
struct icmp *icp;
struct ip *inner_ip;
icp = (struct icmp *) (((u_char *)ip)+(ip->ip_hl<<2));
inner_ip = (struct ip *) (((u_char *)icp)+8);
if (inner_ip->ip_tos != *last_tos)
printf (" (TOS=%d!)", inner_ip->ip_tos);
*last_tos = inner_ip->ip_tos;
}
They indeed don't handle IPv6. But for netcat, it actually hangle IPv6
case at leaet from code in preceding mail. If netcat doesn't want to
handle IPv6 intentionally, I think the IPv6 code should be removed,
thanks!
Best Regards
Nan Xiao
On Thu, Sep 20, 2018 at 7:45 PM Jason McIntyre wrote:
>
> On Wed, Sep 19, 2018 at 06:35:13PM +0800, Nan Xiao wrote:
> > Hi tech@,
> >
> > For `-T' option explanation in netcat manual:
> >
> > -T keyword
> > Change the IPv4 TOS value or the TLS options.
> >
> > But in fact, the netcat code not only processes IPv4 but also IPv6:
> >
> > if (Tflag != -1) {
> > if (af == AF_INET && setsockopt(s, IPPROTO_IP,
> > IP_TOS, , sizeof(Tflag)) == -1)
> > err(1, "set IP ToS");
> >
> > else if (af == AF_INET6 && setsockopt(s, IPPROTO_IPV6,
> > IPV6_TCLASS, , sizeof(Tflag)) == -1)
> > err(1, "set IPv6 traffic class");
> > }
> >
> > So I think maybe the netcat manual should be enriched at least for `-T'
> > option, thanks!
> >
>
> hi.
>
> i think if you submit a diff, there will be a better chance of getting
> an ok (or otherwise).
>
> i'm unsure about -T myself. i know that we synced the -T options for
> ping/nc/traceroute to keep them in sync with pf, but none of those other
> docs claim support for ip6 classes - actually quite the opposite.
> so i'm unsure if they work (have you tested?) or whether we want to
> document them.
>
> jmc
>
Maybe need to enrich `-T' option in netcat manual
Hi tech@,
For `-T' option explanation in netcat manual:
-T keyword
Change the IPv4 TOS value or the TLS options.
But in fact, the netcat code not only processes IPv4 but also IPv6:
if (Tflag != -1) {
if (af == AF_INET && setsockopt(s, IPPROTO_IP,
IP_TOS, , sizeof(Tflag)) == -1)
err(1, "set IP ToS");
else if (af == AF_INET6 && setsockopt(s, IPPROTO_IPV6,
IPV6_TCLASS, , sizeof(Tflag)) == -1)
err(1, "set IPv6 traffic class");
}
So I think maybe the netcat manual should be enriched at least for `-T'
option, thanks!
--
Best Regards
Nan Xiao
[patch] Fix "Address already in use" issue when using netcat with UNIX-domain socket
Hi tech@,
Assume I use netcat with UNIX-domain socket, and there is no
temp_socket. Launch the server:
# ./nc -U -l temp_socket
It works normally. But after netcat exits, launch it again:
# nc -U -l temp_socket
nc: Address already in use
The only method seems to delete temp_socket.
I am not sure this behavior is as expected, and come out following patch
may fix this issue, thanks!
diff --git usr.bin/nc/netcat.c usr.bin/nc/netcat.c
index 341e7e50485..3b2150a01dc 100644
--- usr.bin/nc/netcat.c
+++ usr.bin/nc/netcat.c
@@ -749,6 +749,9 @@ unix_bind(char *path, int flags)
return -1;
}
+ if (lflag)
+ unlink(path);
+
if (bind(s, (struct sockaddr *)_un, sizeof(s_un)) < 0) {
save_errno = errno;
close(s);
--
Best Regards
Nan Xiao
[patch] Use the same backlog parameter for listen() function in netcat.c
Hi tech@,
Since netcat can only process one connection at one time, maybe
UNIX-domain socket can use the same backlog parameter for listen()
function as network socket, thanks!
diff --git netcat.c netcat.c
index 341e7e5..b6fd199 100644
--- netcat.c
+++ netcat.c
@@ -894,7 +894,7 @@ unix_listen(char *path)
if ((s = unix_bind(path, 0)) < 0)
return -1;
- if (listen(s, 5) < 0) {
+ if (listen(s, 1) < 0) {
close(s);
return -1;
}
--
Best Regards
Nan Xiao
[patch] Use correct data type in ldd.c
Hi tech@,
Since both the return value of sizeof() and pread()'s parameter require
size_t type, I think using size_t instead of int to be size's type
should be more appropriate. Thanks!
Index: ldd.c
===
RCS file: /cvs/src/libexec/ld.so/ldd/ldd.c,v
retrieving revision 1.22
diff -u -p -r1.22 ldd.c
--- ldd.c 27 Oct 2017 16:47:08 - 1.22
+++ ldd.c 15 Sep 2018 01:50:16 -
@@ -96,7 +96,8 @@ doit(char *name)
{
Elf_Ehdr ehdr;
Elf_Phdr *phdr;
- int fd, i, size, status, interp=0;
+ int fd, i, status, interp=0;
+ size_t size;
char buf[PATH_MAX];
struct stat st;
void * dlhandle;
--
Best Regards
Nan Xiao
Re: [patch] Fix file descriptor leak in nohup.c
Hi bluhm,
Thanks very much for your time and detailed explanation!
I update the patch, and you can review it when you are free, thanks!
Index: nohup.c
===
RCS file: /cvs/src/usr.bin/nohup/nohup.c,v
retrieving revision 1.17
diff -u -p -r1.17 nohup.c
--- nohup.c 26 Apr 2018 12:42:51 - 1.17
+++ nohup.c 14 Sep 2018 07:04:10 -
@@ -78,13 +78,14 @@ main(int argc, char *argv[])
if (argc < 2)
usage();
- if (isatty(STDOUT_FILENO))
+ if (isatty(STDOUT_FILENO) || errno == EBADF)
dofile();
if (pledge("stdio exec", NULL) == -1)
err(1, "pledge");
- if (isatty(STDERR_FILENO) && dup2(STDOUT_FILENO, STDERR_FILENO) == -1) {
+ if ((isatty(STDERR_FILENO) || errno == EBADF) &&
+ dup2(STDOUT_FILENO, STDERR_FILENO) == -1) {
/* may have just closed stderr */
(void)fprintf(stdin, "nohup: %s\n", strerror(errno));
exit(EXIT_MISC);
@@ -125,6 +126,8 @@ dupit:
(void)lseek(fd, (off_t)0, SEEK_END);
if (dup2(fd, STDOUT_FILENO) == -1)
err(EXIT_MISC, NULL);
+ if (fd > STDERR_FILENO)
+ (void)close(fd);
(void)fprintf(stderr, "sending output to %s\n", p);
}
On 9/12/2018 8:17 PM, Alexander Bluhm wrote:
> On Sun, Sep 09, 2018 at 11:13:40AM +0800, Nan Xiao wrote:
>> Honestly, I am still a little confused:
>
> Let's explain more context. Recently I found some deamons that
> accidently closed stdin, so I am sensible for this kind of error.
>
> nohup(1) tries to use nohup.out for stdout or stderr in some cases.
> If the file descriptors are closed in advance, this does not work.
> To fix these additional cases, I think we should also redirect to
> nohup.out if stdout or stderr is closed.
>
>> "nohup true 2>&-" means close stderr of nohup, right?
>
> Yes.
>
>>> errno = 0;
>>> if (isatty(STDOUT_FILENO) || errno == EBADF)
>>
>> Since we close stderr, why do we need additional check of stdout here?
>
> We should check both stdout and stderr. This is just an example
> how the check could work. Logic should be to redirect stdout or
> stderr to nohup.out, if it is a tty or if it has been closed.
>
> bluhm
>
--
Best Regards
Nan Xiao
Re: [patch] Fix an error in chmod.1
Hi Philip, Very sorry for my fault! Thanks very much for your time and explanation! On 9/14/2018 1:28 PM, Philip Guenther wrote: > On Thu, Sep 13, 2018 at 8:17 PM Nan Xiao <mailto:n...@chinadtrace.org>> wrote: > > The following patch fixes an error in chmod.1, and very sorry if I am > wrong, thanks! > > ... > > --- chmod.1 7 Jun 2017 09:41:57 - 1.42 > +++ chmod.1 14 Sep 2018 05:10:44 - > @@ -243,7 +243,7 @@ If no value is supplied for > each permission bit specified in > .Ar perm , > for which the corresponding bit in the file mode creation mask > -is clear, is cleared. > +is set, is cleared. > Otherwise, the mode bits represented by the specified > .Ar who > and > > > The current documentation matches both the POSIX specification and the > utility behavior. Consider: > > $ touch file > $ chmod 666 file > $ mkdir -m 777 dir > $ ls -l > total 2 > drwxrwxrwx 2 guenther wheel 512 Sep 13 22:24 dir > -rw-rw-rw- 1 guenther wheel 0 Sep 13 22:24 file > $ umask > 002 > $ chmod -rw * > $ ls -l > total 2 > d--x--x-wx 2 guenther wheel 512 Sep 13 22:24 dir > w- 1 guenther wheel 0 Sep 13 22:24 file > $ > > The other-write bit was set in my file mode creation mask, so the "-rw" > was treated like "ug-rw,o-r" and did not change the other-write > permission on the file or directory. > > > Philip Guenther > -- Best Regards Nan Xiao(肖楠)
[patch] Fix an error in chmod.1
Hi tech@, The following patch fixes an error in chmod.1, and very sorry if I am wrong, thanks! Index: chmod.1 === RCS file: /cvs/src/bin/chmod/chmod.1,v retrieving revision 1.42 diff -u -p -r1.42 chmod.1 --- chmod.1 7 Jun 2017 09:41:57 - 1.42 +++ chmod.1 14 Sep 2018 05:10:44 - @@ -243,7 +243,7 @@ If no value is supplied for each permission bit specified in .Ar perm , for which the corresponding bit in the file mode creation mask -is clear, is cleared. +is set, is cleared. Otherwise, the mode bits represented by the specified .Ar who and -- Best Regards Nan Xiao
Re: [patch] Fix file descriptor leak in nohup.c
Hi bluhm, Thanks for your reply! Honestly, I am still a little confused: > But before you plugged the fd leak, "nohup true 2>&-" would write to nohup.out. "nohup true 2>&-" means close stderr of nohup, right? > errno = 0; > if (isatty(STDOUT_FILENO) || errno == EBADF) Since we close stderr, why do we need additional check of stdout here? Thanks in advacne! On 9/9/2018 7:39 AM, Alexander Bluhm wrote: > On Sat, Sep 08, 2018 at 08:47:28PM +0800, Nan Xiao wrote: >> I don't think need "if (fd > STDERR_FILENO)" cause the fd is a new >> opened file descriptor. If its value is less or equal than >> `STDERR_FILENO', it means the stdin or stderr is already closed >> intentionally. So it should be OK close fd again. > > In general it is a bad idea to start programs without opened 0 1 2 > file descriptors. Then the next open will fill the gap and a > printf() or err() may write to random files. > > Some paranoid daemons like syslogd(8) dup2 /dev/null to them. And > there is this check: > if (nullfd > 2) > close(nullfd); > > If course nohup(1) is not that paranoid. And the isatty() checks > prevent that the stdio file decriptor gaps get filled. But before > you plugged the fd leak, "nohup true 2>&-" would write to nohup.out. > > Perhaps we could make nohup(1) a bit more sane by checks like this. > > errno = 0; > if (isatty(STDOUT_FILENO) || errno == EBADF) > > bluhm > -- Best Regards Nan Xiao
Re: [patch] Fix file descriptor leak in nohup.c
Hi bluhm, I don't think need "if (fd > STDERR_FILENO)" cause the fd is a new opened file descriptor. If its value is less or equal than `STDERR_FILENO', it means the stdin or stderr is already closed intentionally. So it should be OK close fd again. Thanks! On 9/8/2018 8:26 PM, Alexander Bluhm wrote: > On Sat, Sep 08, 2018 at 11:05:15AM +0800, Nan Xiao wrote: >> The following patch fixes file descriptor leak in `dofile' function. >> Sorry if I am wrong, thanks! > > It is a leak. > >> @@ -125,6 +125,7 @@ dupit: >> (void)lseek(fd, (off_t)0, SEEK_END); >> if (dup2(fd, STDOUT_FILENO) == -1) >> err(EXIT_MISC, NULL); > > The paranoid would put an if (fd > STDERR_FILENO) here. > >> +(void)close(fd); >> (void)fprintf(stderr, "sending output to %s\n", p); >> } > > bluhm > -- Best Regards Nan Xiao(肖楠)
[patch] Fix file descriptor leak in nohup.c
Hi tech@, The following patch fixes file descriptor leak in `dofile' function. Sorry if I am wrong, thanks! Index: nohup.c === RCS file: /cvs/src/usr.bin/nohup/nohup.c,v retrieving revision 1.17 diff -u -p -r1.17 nohup.c --- nohup.c 26 Apr 2018 12:42:51 - 1.17 +++ nohup.c 8 Sep 2018 02:54:48 - @@ -125,6 +125,7 @@ dupit: (void)lseek(fd, (off_t)0, SEEK_END); if (dup2(fd, STDOUT_FILENO) == -1) err(EXIT_MISC, NULL); + (void)close(fd); (void)fprintf(stderr, "sending output to %s\n", p); } -- Best Regards Nan Xiao
[patch] Modify local_listen declaration in netcat.c
Hi tech@,
The following patch modify `local_listen' declaration to conform to
`getaddrinfo', thanks!
Index: netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.193
diff -u -p -r1.193 netcat.c
--- netcat.c6 Sep 2018 13:23:02 - 1.193
+++ netcat.c7 Sep 2018 09:37:02 -
@@ -122,7 +122,7 @@ voidatelnet(int, unsigned char *, unsig
intstrtoport(char *portstr, int udp);
void build_ports(char *);
void help(void) __attribute__((noreturn));
-intlocal_listen(char *, char *, struct addrinfo);
+intlocal_listen(const char *, const char *, struct addrinfo);
void readwrite(int, struct tls *);
void fdpass(int nfd) __attribute__((noreturn));
intremote_connect(const char *, const char *, struct addrinfo);
@@ -994,7 +994,7 @@ timeout_connect(int s, const struct sock
* address. Returns -1 on failure.
*/
int
-local_listen(char *host, char *port, struct addrinfo hints)
+local_listen(const char *host, const char *port, struct addrinfo hints)
{
struct addrinfo *res, *res0;
int s = -1, ret, x = 1, save_errno;
--
Best Regards
Nan Xiao
Re: [patch] Fix closing socket twice bug in netcat program
Hi bluhm,
Very sorry for my remiss! Updated, thanks!
Index: netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.192
diff -u -p -r1.192 netcat.c
--- netcat.c10 Aug 2018 17:15:22 - 1.192
+++ netcat.c6 Sep 2018 10:10:07 -
@@ -564,8 +564,11 @@ main(int argc, char *argv[])
}
/* Allow only one connection at a time, but stay alive. */
for (;;) {
- if (family != AF_UNIX)
+ if (family != AF_UNIX) {
+ if (s != -1)
+ close(s);
s = local_listen(host, uport, hints);
+ }
if (s < 0)
err(1, NULL);
if (uflag && kflag) {
@@ -622,9 +625,7 @@ main(int argc, char *argv[])
}
close(connfd);
}
- if (family != AF_UNIX)
- close(s);
- else if (uflag) {
+ if (family == AF_UNIX && uflag) {
if (connect(s, NULL, 0) < 0)
err(1, "connect");
}
On 9/6/2018 8:53 PM, Alexander Bluhm wrote:
> On Thu, Sep 06, 2018 at 06:27:15PM +0800, Nan Xiao wrote:
>> @@ -564,8 +564,11 @@ main(int argc, char *argv[])
>> }
>> /* Allow only one connection at a time, but stay alive. */
>> for (;;) {
>> -if (family != AF_UNIX)
>> +if (family != AF_UNIX) {
>> +if (s == -1)
>
> This has to be (s != -1).
>
>> +close(s);
>> s = local_listen(host, uport, hints);
>> +}
>> if (s < 0)
>> err(1, NULL);
>> if (uflag && kflag) {
>> @@ -622,9 +625,7 @@ main(int argc, char *argv[])
>> }
>> close(connfd);
>> }
>> -if (family != AF_UNIX)
>> -close(s);
>> -else if (uflag) {
>> +if (family == AF_UNIX && uflag) {
>> if (connect(s, NULL, 0) < 0)
>> err(1, "connect");
>> }
>
> otherwise OK bluhm@
>
--
Best Regards
Nan Xiao(肖楠)
Re: [patch] Fix closing socket twice bug in netcat program
Hi bluhm,
Thanks for your reply! I think your method is better, and I update the
patch:
Index: netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.192
diff -u -p -r1.192 netcat.c
--- netcat.c10 Aug 2018 17:15:22 - 1.192
+++ netcat.c6 Sep 2018 10:10:07 -
@@ -564,8 +564,11 @@ main(int argc, char *argv[])
}
/* Allow only one connection at a time, but stay alive. */
for (;;) {
- if (family != AF_UNIX)
+ if (family != AF_UNIX) {
+ if (s == -1)
+ close(s);
s = local_listen(host, uport, hints);
+ }
if (s < 0)
err(1, NULL);
if (uflag && kflag) {
@@ -622,9 +625,7 @@ main(int argc, char *argv[])
}
close(connfd);
}
- if (family != AF_UNIX)
- close(s);
- else if (uflag) {
+ if (family == AF_UNIX && uflag) {
if (connect(s, NULL, 0) < 0)
err(1, "connect");
}
Thanks!
On 9/6/2018 5:07 AM, Alexander Bluhm wrote:
> On Tue, Sep 04, 2018 at 01:01:38PM +0800, Nan Xiao wrote:
>> Before netcat program exits, it will check whether s is -1, and close
>> socket if s is not -1:
>>
>> if (s != -1)
>> close(s);
>>
>> The following patch fixes the issue that netcat will close socket twice
>> if it works as a server:
>
> I think it is a bug, but should be fixed differently. The netstat
> code has a lot of if and else for all the use cases. Adding a s =
> -1 at one place makes it even more confusing.
>
> In general main() does not reset s to -1, it isets it at the
> beginning. Look at the client side. There we close at the beginning
> of the loop:
>
> /* Cycle through portlist, connecting to each port. */
> for (s = -1, i = 0; portlist[i] != NULL; i++) {
> if (s != -1)
> close(s);
>
> So on the server side we should do the same, otherwise the code
> will get more and more messy. Use the same concept everywhere.
> I think this would be better:
>
> /* Allow only one connection at a time, but stay alive. */
> for (;;) {
> if (family != AF_UNIX) {
> if (s != -1)
> close(s);
> s = local_listen(host, uport, hints);
> }
>
> bluhm
>
--
Best Regards
Nan Xiao(肖楠)
Re: [patch] Fix closing socket twice bug in netcat program
Ping tech@,
Could anyone spare a minute to check this patch? I think it is indeed a bug.
On 9/4/2018 1:01 PM, Nan Xiao wrote:
> Hi tech@,
>
> Before netcat program exits, it will check whether s is -1, and close
> socket if s is not -1:
>
> if (s != -1)
> close(s);
>
> The following patch fixes the issue that netcat will close socket twice
> if it works as a server:
>
> Index: netcat.c
> ===
> RCS file: /cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.192
> diff -u -p -r1.192 netcat.c
> --- netcat.c 10 Aug 2018 17:15:22 - 1.192
> +++ netcat.c 4 Sep 2018 04:51:55 -
> @@ -622,9 +622,10 @@ main(int argc, char *argv[])
> }
> close(connfd);
> }
> - if (family != AF_UNIX)
> + if (family != AF_UNIX) {
> close(s);
> - else if (uflag) {
> + s = -1;
> + } else if (uflag) {
> if (connect(s, NULL, 0) < 0)
> err(1, "connect");
> }
>
> Thanks!
>
--
Best Regards
Nan Xiao(肖楠)
[patch] Fix closing socket twice bug in netcat program
Hi tech@,
Before netcat program exits, it will check whether s is -1, and close
socket if s is not -1:
if (s != -1)
close(s);
The following patch fixes the issue that netcat will close socket twice
if it works as a server:
Index: netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.192
diff -u -p -r1.192 netcat.c
--- netcat.c10 Aug 2018 17:15:22 - 1.192
+++ netcat.c4 Sep 2018 04:51:55 -
@@ -622,9 +622,10 @@ main(int argc, char *argv[])
}
close(connfd);
}
- if (family != AF_UNIX)
+ if (family != AF_UNIX) {
close(s);
- else if (uflag) {
+ s = -1;
+ } else if (uflag) {
if (connect(s, NULL, 0) < 0)
err(1, "connect");
}
Thanks!
--
Best Regards
Nan Xiao
[patch] A small modification in /usr.bin/nc/netcat.c
Hi tech@, The following patch uses hostname instead of nodename to be consistent with getaddrinfo declaration: int getaddrinfo(const char *hostname, const char *servname, const struct addrinfo *hints, struct addrinfo **res); Thanks! Index: netcat.c === RCS file: /cvs/src/usr.bin/nc/netcat.c,v retrieving revision 1.192 diff -u -p -r1.192 netcat.c --- netcat.c10 Aug 2018 17:15:22 - 1.192 +++ netcat.c3 Sep 2018 11:12:49 - @@ -999,7 +999,7 @@ local_listen(char *host, char *port, str int s = -1, ret, x = 1, save_errno; int error; - /* Allow nodename to be null. */ + /* Allow hostname to be null. */ hints.ai_flags |= AI_PASSIVE; /* -- Best Regards Nan Xiao
[patch] Use API to get params in lib/libtls/tls.c
Hi tech@,
This patch uses API to get params, sorry if I am wrong, thanks!
Index: tls.c
===
RCS file: /cvs/src/lib/libtls/tls.c,v
retrieving revision 1.80
diff -u -p -r1.80 tls.c
--- tls.c 7 Apr 2018 16:30:59 - 1.80
+++ tls.c 20 Aug 2018 10:50:20 -
@@ -437,7 +437,7 @@ tls_configure_ssl(struct tls *ctx, SSL_C
}
if (ctx->config->verify_time == 0) {
- X509_VERIFY_PARAM_set_flags(ssl_ctx->param,
+ X509_VERIFY_PARAM_set_flags(SSL_CTX_get0_param(ssl_ctx),
X509_V_FLAG_NO_CHECK_TIME);
}
@@ -547,7 +547,7 @@ tls_configure_ssl_verify(struct tls *ctx
}
xi->crl = NULL;
}
- X509_VERIFY_PARAM_set_flags(store->param,
+ X509_VERIFY_PARAM_set_flags(X509_STORE_get0_param(store),
X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
}
--
Best Regards
Nan Xiao
Remove unused variable in usr.bin/openssl/apps.c
Hi tech@,
The `free_out' variable seems redundant, so this patch removes it:
Index: apps.c
===
RCS file: /cvs/src/usr.bin/openssl/apps.c,v
retrieving revision 1.47
diff -u -p -r1.47 apps.c
--- apps.c 7 Feb 2018 08:57:25 - 1.47
+++ apps.c 16 Aug 2018 09:18:43 -
@@ -2050,11 +2050,9 @@ policies_print(BIO *out, X509_STORE_CTX
{
X509_POLICY_TREE *tree;
int explicit_policy;
- int free_out = 0;
if (out == NULL) {
out = BIO_new_fp(stderr, BIO_NOCLOSE);
- free_out = 1;
}
tree = X509_STORE_CTX_get0_policy_tree(ctx);
explicit_policy = X509_STORE_CTX_get_explicit_policy(ctx);
--
Best Regards
Nan Xiao
[patch] consistent verb form in nc.1
Hi tech@,
This patch uses consistent verb form ("specifies") in nc.1, thanks!
Index: nc.1
===
RCS file: /cvs/src/usr.bin/nc/nc.1,v
retrieving revision 1.88
diff -u -p -r1.88 nc.1
--- nc.128 Nov 2017 16:59:10 - 1.88
+++ nc.114 Aug 2018 09:45:31 -
@@ -115,7 +115,7 @@ Enable debugging on the socket.
.It Fl d
Do not attempt to read from stdin.
.It Fl e Ar name
-Specify the name that must be present in the peer certificate when
using TLS.
+Specifies the name that must be present in the peer certificate when
using TLS.
Illegal if not using TLS.
.It Fl F
Pass the first connected socket using
--
Best Regards
Nan Xiao
[patch] Add omitting period for ktrace(2) manual
Hi tech@, To be consistent with other trace points, this patch adds omitting period for KTRFAC_STRUCT, thanks! Index: ktrace.2 === RCS file: /cvs/src/lib/libc/sys/ktrace.2,v retrieving revision 1.35 diff -u -p -r1.35 ktrace.2 --- ktrace.228 Nov 2017 06:03:41 - 1.35 +++ ktrace.219 Jun 2018 08:10:38 - @@ -108,7 +108,7 @@ Trace all I/O .It Dv KTRFAC_PSIG Trace posted signals. .It Dv KTRFAC_STRUCT -Trace various structs +Trace various structs. .It Dv KTRFAC_USER Trace user data coming from .Xr utrace 2 -- Best Regards Nan Xiao
Re: [patch] Fix inaccurate comment in usr.bin/w/w.c
Hi Raf,
Yes, I am in favour of 'nflag' should be modified to "aflag", and revise
this patch. Thanks!
Index: w.c
===
RCS file: /cvs/src/usr.bin/w/w.c,v
retrieving revision 1.65
diff -u -p -r1.65 w.c
--- w.c 18 Dec 2017 05:51:53 - 1.65
+++ w.c 15 Jun 2018 09:43:52 -
@@ -71,9 +71,9 @@ struct winsizews;
kvm_t *kd;
time_t now;/* the current time of day */
intttywidth; /* width of tty */
-intargwidth; /* width of tty */
-intheader = 1; /* true if -h flag: don't print heading */
-intnflag = 1; /* true if -n flag: don't convert addrs */
+intargwidth; /* width of name and args of the current
process */
+intheader = 1; /* false if -h or -M flag: don't print heading
*/
+intaflag = 1; /* false if -a flag: don't convert addrs */
intsortidle; /* sort by idle time */
char *sel_user; /* login of particular user selected */
char domain[HOST_NAME_MAX+1];
@@ -144,7 +144,7 @@ main(int argc, char *argv[])
nlistf = optarg;
break;
case 'a':
- nflag = 0;
+ aflag = 0;
break;
case 'f': case 'l': case 's': case 'u': case 'w':
warnx("[-flsuw] no longer supported");
@@ -156,7 +156,7 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
- if (nflag == 0) {
+ if (aflag == 0) {
if (pledge("stdio tty rpath dns ps vminfo", NULL) == -1)
err(1, "pledge");
} else {
@@ -285,7 +285,7 @@ main(int argc, char *argv[])
}
}
- if (!nflag) {
+ if (!aflag) {
if (gethostname(domain, sizeof(domain)) < 0 ||
(p = strchr(domain, '.')) == 0)
domain[0] = '\0';
@@ -303,7 +303,7 @@ main(int argc, char *argv[])
*x++ = '\0';
break;
}
- if (!nflag && inet_aton(p, ) &&
+ if (!aflag && inet_aton(p, ) &&
(hp = gethostbyaddr((char *), sizeof(addr), AF_INET)))
{
if (domain[0] != '\0') {
p = hp->h_name;
On 6/15/2018 5:57 AM, Raf Czlonka wrote:
> On Thu, Jun 14, 2018 at 06:28:47AM BST, Nan Xiao wrote:
>> Hi tech@,
>>
>> The following patch fix some inaccurate comment in w.c. E.g., there is
>> no "-n" option, and "-a" instead. Sorry id I am wrong, thanks!
>>
>> Index: w.c
>> ===
>> RCS file: /cvs/src/usr.bin/w/w.c,v
>> retrieving revision 1.65
>> diff -u -p -r1.65 w.c
>> --- w.c 18 Dec 2017 05:51:53 - 1.65
>> +++ w.c 14 Jun 2018 05:17:00 -
>> @@ -71,9 +71,9 @@ struct winsize ws;
>> kvm_t *kd;
>> time_t now;/* the current time of day */
>> int ttywidth; /* width of tty */
>> -int argwidth; /* width of tty */
>> -int header = 1; /* true if -h flag: don't print heading */
>> -int nflag = 1; /* true if -n flag: don't convert addrs */
>> +int argwidth; /* width of name and args of the current
>> process */
>> +int header = 1; /* false if -h or -M flag: don't print heading
>> */
>> +int nflag = 1; /* false if -a flag: don't convert addrs */
>> int sortidle; /* sort by idle time */
>> char *sel_user; /* login of particular user selected */
>> chardomain[HOST_NAME_MAX+1];
>
> FYI, the '-n' to '-a' change happened nearly 22 years ago[0].
>
> Given that "-a flag" should clearly be in the comment, shouldn't
> there a mechanical nflag -> aflag change also take place?
>
> [0]
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/w/w.c.diff?r1=1.5=1.6=h
>
> Regards,
>
> Raf
>
--
Best Regards
Nan Xiao(肖楠)
[patch] Fix inaccurate comment in usr.bin/w/w.c
Hi tech@, The following patch fix some inaccurate comment in w.c. E.g., there is no "-n" option, and "-a" instead. Sorry id I am wrong, thanks! Index: w.c === RCS file: /cvs/src/usr.bin/w/w.c,v retrieving revision 1.65 diff -u -p -r1.65 w.c --- w.c 18 Dec 2017 05:51:53 - 1.65 +++ w.c 14 Jun 2018 05:17:00 - @@ -71,9 +71,9 @@ struct winsizews; kvm_t *kd; time_t now;/* the current time of day */ intttywidth; /* width of tty */ -intargwidth; /* width of tty */ -intheader = 1; /* true if -h flag: don't print heading */ -intnflag = 1; /* true if -n flag: don't convert addrs */ +intargwidth; /* width of name and args of the current process */ +intheader = 1; /* false if -h or -M flag: don't print heading */ +intnflag = 1; /* false if -a flag: don't convert addrs */ intsortidle; /* sort by idle time */ char *sel_user; /* login of particular user selected */ char domain[HOST_NAME_MAX+1]; -- Best Regards Nan Xiao
Re: [patch] Use snprintf to implement concat in etc.c
Hi Philip, Thanks for your comments! On 6/9/2018 12:35 PM, Philip Guenther wrote: > On Thu, Jun 7, 2018 at 12:34 AM Nan Xiao <mailto:n...@chinadtrace.org>> wrote: > > I think maybe one snprintf is more efficient and concise. Sorry if I am > wrong, thanks! > > > Seems unlikely. More productive would be to go up a level and look at > whether concat() is carrying its weight as an abstraction. One of the > calls is completely superfluous, to my eye. > > But really, ldconfig is pretty uninteresting, being run very, very > rarely, so polishing it is like sanding the rough spots on the bottoms > of table legs, where they touch the floor. We've spent more cycles > talking about it then it'll use in a year... > > > > Philip Guenther > -- Best Regards Nan Xiao(肖楠)
[patch] Use snprintf to implement concat in etc.c
HI tech@, I think maybe one snprintf is more efficient and concise. Sorry if I am wrong, thanks! Index: etc.c === RCS file: /cvs/src/libexec/ld.so/ldconfig/etc.c,v retrieving revision 1.7 diff -u -p -r1.7 etc.c --- etc.c 12 May 2006 23:35:16 - 1.7 +++ etc.c 7 Jun 2018 07:10:30 - @@ -5,6 +5,7 @@ #include #include +#include #include #include #include "ld.h" @@ -61,9 +62,7 @@ concat(const char *s1, const char *s2, c len = strlen(s1) + strlen(s2) + strlen(s3) + 1; str = xmalloc(len); - strlcpy(str, s1, len); - strlcat(str, s2, len); - strlcat(str, s3, len); + snprintf(str, len, "%s%s%s", s1, s2, s3); return (str); } -- Best Regards Nan Xiao
[patch] Add error check for fchmod in ldconfig.c
Hi tech@,
Maybe a error check of fchmod is necessary. Sorry if I am wrong, thanks!
Index: ldconfig.c
===
RCS file: /cvs/src/libexec/ld.so/ldconfig/ldconfig.c,v
retrieving revision 1.37
diff -u -p -r1.37 ldconfig.c
--- ldconfig.c 26 Apr 2018 12:42:50 - 1.37
+++ ldconfig.c 7 Jun 2018 07:04:45 -
@@ -386,7 +386,11 @@ buildhints(void)
warn("%s", tmpfilenam);
goto out;
}
- fchmod(fd, 0444);
+
+ if (fchmod(fd, 0444) == -1) {
+ warn("%s", tmpfilenam);
+ goto out;
+ }
if (write(fd, , sizeof(struct hints_header)) !=
sizeof(struct hints_header)) {
--
Best Regards
Nan Xiao
[patch] Remove unused variable in gnu/llvm/lib/Target/X86/X86FrameLowering.cpp
Hi tech@, The following patch fixes the build warning: .. variable 'CFIIndex' [-Wunused-variable] unsigned CFIIndex; ^ 1 warning generated. Index: X86FrameLowering.cpp === RCS file: /cvs/src/gnu/llvm/lib/Target/X86/X86FrameLowering.cpp,v retrieving revision 1.2 diff -u -p -r1.2 X86FrameLowering.cpp --- X86FrameLowering.cpp6 Jun 2018 00:14:29 - 1.2 +++ X86FrameLowering.cpp6 Jun 2018 07:34:44 - @@ -3185,7 +3185,6 @@ void X86FrameLowering::insertReturnProte if (!MFI.hasReturnProtector() || !MFI.hasReturnProtectorTempRegister()) return; - unsigned CFIIndex; MachineBasicBlock::instr_iterator MBBI = MBB.instr_begin(); DebugLoc MBBDL = MBB.findDebugLoc(MBBI); const Function = MF.getFunction(); -- Best Regards Nan Xiao
Re: [patch] Add kvm_close in mib_hrsystemprocs function
Hi Gerhard,
Thanks for your reply!
Yes, if no "kvm_close(kd);", there will be resource (memory, file
descriptor) leak. So hope you can commit it, thanks!
On 5/30/2018 4:49 PM, Gerhard Roth wrote:
> On Wed, 30 May 2018 16:25:55 +0800 Nan Xiao wrote:
>> Hi tech@,
>>
>> Maybe kvm_close is needed if kvm_getprocs returns NULL here? Sorry if I
>> am wrong, thanks!
>>
>> Index: mib.c
>> ===
>> RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
>> retrieving revision 1.87
>> diff -u -p -r1.87 mib.c
>> --- mib.c25 May 2018 08:23:15 - 1.87
>> +++ mib.c30 May 2018 08:15:19 -
>> @@ -516,8 +516,10 @@ mib_hrsystemprocs(struct oid *oid, struc
>> return (-1);
>>
>> if (kvm_getprocs(kd, KERN_PROC_ALL, 0,
>> -sizeof(struct kinfo_proc), ) == NULL)
>> +sizeof(struct kinfo_proc), ) == NULL) {
>> +kvm_close(kd);
>> return (-1);
>> +}
>>
>> *elm = ber_add_integer(*elm, val);
>> ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_GAUGE32);
>>
>
>
> Looks reasonable.
>
--
Best Regards
Nan Xiao
[patch] Add kvm_close in mib_hrsystemprocs function
Hi tech@,
Maybe kvm_close is needed if kvm_getprocs returns NULL here? Sorry if I
am wrong, thanks!
Index: mib.c
===
RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
retrieving revision 1.87
diff -u -p -r1.87 mib.c
--- mib.c 25 May 2018 08:23:15 - 1.87
+++ mib.c 30 May 2018 08:15:19 -
@@ -516,8 +516,10 @@ mib_hrsystemprocs(struct oid *oid, struc
return (-1);
if (kvm_getprocs(kd, KERN_PROC_ALL, 0,
- sizeof(struct kinfo_proc), ) == NULL)
+ sizeof(struct kinfo_proc), ) == NULL) {
+ kvm_close(kd);
return (-1);
+ }
*elm = ber_add_integer(*elm, val);
ber_set_header(*elm, BER_CLASS_APPLICATION, SNMP_T_GAUGE32);
--
Best Regards
Nan Xiao
Re: [patch] Reset sysload if sysctl call failed in usr.bin/top/machine.c
Hi Jeremie,
Yes, you are right. I modify the code which assumes if sysctl successes,
the sysload.fscale should not be 0. So this patch only handles sysctl
function failed case:
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.90
diff -u -p -r1.90 machine.c
--- machine.c 14 May 2018 12:31:21 - 1.90
+++ machine.c 25 May 2018 01:12:20 -
@@ -282,8 +282,11 @@ get_system_info(struct system_info *si)
}
size = sizeof(sysload);
- if (sysctl(sysload_mib, 2, , , NULL, 0) < 0)
+ if (sysctl(sysload_mib, 2, , , NULL, 0) < 0) {
warn("sysctl failed");
+ bzero(, sizeof(sysload));
+ sysload.fscale = 1;
+ }
infoloadp = si->load_avg;
for (i = 0; i < 3; i++)
*infoloadp++ = ((double) sysload.ldavg[i]) / sysload.fscale;
Thanks!
On 5/24/2018 7:54 PM, Jeremie Courreges-Anglas wrote:
> On Tue, May 22 2018, Nan Xiao <n...@chinadtrace.org> wrote:
>> Hi tech@,
>>
>> Below is the patch of resetting sysload if sysctl call failed in
>> usr.bin/top/machine.c, otherwise the memory of sysload is undetermined.
>> (The same process as uvmexp and bcstats).
>>
>> Index: machine.c
>> ===
>> RCS file: /cvs/src/usr.bin/top/machine.c,v
>> retrieving revision 1.90
>> diff -u -p -r1.90 machine.c
>> --- machine.c14 May 2018 12:31:21 - 1.90
>> +++ machine.c22 May 2018 09:49:39 -
>> @@ -282,8 +282,10 @@ get_system_info(struct system_info *si)
>> }
>>
>> size = sizeof(sysload);
>> -if (sysctl(sysload_mib, 2, , , NULL, 0) < 0)
>> +if (sysctl(sysload_mib, 2, , , NULL, 0) < 0) {
>> warn("sysctl failed");
>> +bzero(, sizeof(sysload));
>> +}
>> infoloadp = si->load_avg;
>> for (i = 0; i < 3; i++)
>> *infoloadp++ = ((double) sysload.ldavg[i]) / sysload.fscale;
>
> This line of code should not run if sysload.fscale == 0.
>
--
Best Regards
Nan Xiao
[patch] Reset sysload if sysctl call failed in usr.bin/top/machine.c
Hi tech@,
Below is the patch of resetting sysload if sysctl call failed in
usr.bin/top/machine.c, otherwise the memory of sysload is undetermined.
(The same process as uvmexp and bcstats).
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.90
diff -u -p -r1.90 machine.c
--- machine.c 14 May 2018 12:31:21 - 1.90
+++ machine.c 22 May 2018 09:49:39 -
@@ -282,8 +282,10 @@ get_system_info(struct system_info *si)
}
size = sizeof(sysload);
- if (sysctl(sysload_mib, 2, , , NULL, 0) < 0)
+ if (sysctl(sysload_mib, 2, , , NULL, 0) < 0) {
warn("sysctl failed");
+ bzero(, sizeof(sysload));
+ }
infoloadp = si->load_avg;
for (i = 0; i < 3; i++)
*infoloadp++ = ((double) sysload.ldavg[i]) / sysload.fscale;
--
Best Regards
Nan Xiao
[patch] Remove unnecessary timerclear in tcpbench
Hi tech@,
Following is a trivial modification which removes unnecessary
initialization of timeval. I am Sorry if this patch seems a little picky.
Index: tcpbench.c
===
RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v
retrieving revision 1.55
diff -u -p -r1.55 tcpbench.c
--- tcpbench.c 10 May 2018 14:29:17 - 1.55
+++ tcpbench.c 12 May 2018 02:06:54 -
@@ -253,7 +253,6 @@ set_slice_timer(int on)
if (on) {
if (evtimer_pending(, NULL))
return;
- timerclear();
/* XXX Is there a better way to do this ? */
tv.tv_sec = ptb->rflag / 1000;
tv.tv_usec = (ptb->rflag % 1000) * 1000;
--
Best Regards
Nan Xiao
[patch] Use err instead of errx for checking strdup failed
Hi tech@, Following is a trivial modification of tcpbench.c, and I think use err is more better than errx. Apologize if I am wrong, thanks! Index: tcpbench.c === RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v retrieving revision 1.52 diff -u -p -r1.52 tcpbench.c --- tcpbench.c 19 Sep 2016 18:58:39 - 1.52 +++ tcpbench.c 10 May 2018 09:41:42 - @@ -1041,7 +1041,7 @@ main(int argc, char **argv) exit(0); case 'k': if ((tmp = strdup(optarg)) == NULL) - errx(1, "strdup"); + err(1, "strdup"); ptb->kvars = check_prepare_kvars(tmp); free(tmp); break; -- Best Regards Nan Xiao
Re: [patch] Set TCP send buffer size only when tcpbench client works in TCP mode
Very sorry for forgetting to cc reply into tech@.
Forwarded Message
Subject: Re: [patch] Set TCP send buffer size only when tcpbench client
works in TCP mode
Date: Thu, 10 May 2018 17:56:25 +0800
From: Nan Xiao <n...@chinadtrace.org>
To: Alexander Bluhm <alexander.bl...@gmx.net>
Hi Bluhm,
Thanks for your reply!
Since from the comment of code (line 66):
struct {
int Sflag;/* Socket buffer size (tcp mode) */
..
}
So I think maybe this should work on TCP only. BTW, if it works on both
TCP/UDP, I think differentiate error message will be better:
Index: tcpbench.c
===
RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v
retrieving revision 1.52
diff -u -p -r1.52 tcpbench.c
--- tcpbench.c 19 Sep 2016 18:58:39 - 1.52
+++ tcpbench.c 10 May 2018 09:54:42 -
@@ -895,7 +895,7 @@ client_init(struct addrinfo *aitop, int
if (ptb->Sflag) {
if (setsockopt(sock, SOL_SOCKET, SO_SNDBUF,
>Sflag, sizeof(ptb->Sflag)) == -1)
- warn("set TCP send buffer size");
+ warn("set %s send buffer size",
TCP_MODE? "TCP" : "UDP");
}
if (connect(sock, ai->ai_addr, ai->ai_addrlen) != 0) {
if (ai->ai_next == NULL)
Thanks!
On 5/10/2018 5:39 PM, Alexander Bluhm wrote:
> On Thu, May 10, 2018 at 03:10:53PM +0800, Nan Xiao wrote:
>> Per my understanding, the tcpbench.Sflag (Socket buffer size) should
>> only take effect when working in TCP mode.
>
> Why? The kernel provides socket buffers for all protocols. Userland
> can set them and the kernel does the appropriate things. There is
> no reason to restrict setting socket buffer size to TCP.
>
>> But according to my testing,
>> it also works when tcpbench client is in UDP mode:
>>
>> (1) Set buffer size less than DEFAULT_UDP_PKT (1500 - 28):
>> # tcpbench -u -S 1471 127.0.0.1
>> tcpbench: write: Message too long
>
> Of course it works for UDP. That is the expected behavior. UDP
> is atomic and if the buffer is too small for a packet, you cannot
> send.
>
>> So I think maybe it is need to check whether client is in TCP mode
>> before setting buffer size, and apologize if I am wrong:
>
> I would be very surprized by this silent restriction of tcpbench.
> If I test UDP, I want to see what the kernel does if I change the
> socket buffer size.
>
> bluhm
>
--
Best Regards
Nan Xiao(肖楠)
[patch] Set TCP send buffer size only when tcpbench client works in TCP mode
Hi tech@,
Per my understanding, the tcpbench.Sflag (Socket buffer size) should
only take effect when working in TCP mode. But according to my testing,
it also works when tcpbench client is in UDP mode:
(1) Set buffer size less than DEFAULT_UDP_PKT (1500 - 28):
# tcpbench -u -S 1471 127.0.0.1
tcpbench: write: Message too long
(2) Set buffer size too long:
# tcpbench -u -S 1 127.0.0.1
tcpbench: set TCP send buffer size: No buffer space available
..
So I think maybe it is need to check whether client is in TCP mode
before setting buffer size, and apologize if I am wrong:
Index: tcpbench.c
===
RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v
retrieving revision 1.52
diff -u -p -r1.52 tcpbench.c
--- tcpbench.c 19 Sep 2016 18:58:39 - 1.52
+++ tcpbench.c 10 May 2018 06:55:50 -
@@ -892,7 +892,7 @@ client_init(struct addrinfo *aitop, int
>Tflag, sizeof(ptb->Tflag)))
err(1, "setsockopt IPV6_TCLASS");
}
- if (ptb->Sflag) {
+ if (TCP_MODE && ptb->Sflag) {
if (setsockopt(sock, SOL_SOCKET, SO_SNDBUF,
>Sflag, sizeof(ptb->Sflag)) == -1)
warn("set TCP send buffer size");
--
Best Regards
Nan Xiao
[patch] Unify format of tcpbench man page
Hi tech@, This patch unifies format of tcpbench man page. Apologize if I am wrong, thanks! Index: tcpbench.1 === RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.1,v retrieving revision 1.23 diff -u -p -r1.23 tcpbench.1 --- tcpbench.1 25 Sep 2016 23:31:50 - 1.23 +++ tcpbench.1 9 May 2018 08:20:25 - @@ -77,11 +77,11 @@ option. The options are as follows: .Bl -tag -width Ds .It Fl 4 -Forces +Force .Nm to use IPv4 addresses only. .It Fl 6 -Forces +Force .Nm to use IPv6 addresses only. .It Fl B Ar buf @@ -91,7 +91,7 @@ The default is 262144 bytes for TCP clie In UDP client mode this may be used to specify the packet size on the test stream. .It Fl b Ar addr -Specifies the IP address of the interface which is used to send the packets. +Specify the IP address of the interface which is used to send the packets. .It Fl k Ar kvars Specify one or more kernel variables to monitor; multiple variables must be separated with commas. -- Best Regards Nan Xiao
[patch] Remove leading space in lib/libcrypto/asn1/a_time_tm.c
Hi tech@
Remove leading space in ASN1_TIME_adj_internal function, apologize if I
am wrong, thanks!
Index: a_time_tm.c
===
RCS file: /cvs/src/lib/libcrypto/asn1/a_time_tm.c,v
retrieving revision 1.15
diff -u -p -r1.15 a_time_tm.c
--- a_time_tm.c 25 Apr 2018 11:48:21 - 1.15
+++ a_time_tm.c 8 May 2018 04:57:53 -
@@ -262,8 +262,8 @@ ASN1_TIME_adj_internal(ASN1_TIME *s, tim
size_t len;
char * p;
- if (gmtime_r(, ) == NULL)
- return (NULL);
+ if (gmtime_r(, ) == NULL)
+ return (NULL);
if (offset_day || offset_sec) {
if (!OPENSSL_gmtime_adj(, offset_day, offset_sec))
--
Best Regards
Nan Xiao
[patch] Exit directly when allocating memory failed in usr.bin/systat/cpu.c
Hi tech@,
In usr.bin/systat/cpu.c, if allocating memory failed, it seems no need
to return value, call "err()" function may be better (refer
usr.bin/systat/vmstat.c). Apologize if I'm wrong, thanks!
Index: cpu.c
===
RCS file: /cvs/src/usr.bin/systat/cpu.c,v
retrieving revision 1.5
diff -u -p -r1.5 cpu.c
--- cpu.c 2 Jan 2016 20:02:40 - 1.5
+++ cpu.c 7 May 2018 05:40:07 -
@@ -50,6 +50,7 @@
#include
#include
+#include
#include
#include
#include
@@ -200,16 +201,16 @@ initcpu(void)
return (-1);
if ((cpu_states = calloc(cpu_count,
CPUSTATES * sizeof(int64_t))) == NULL)
- return (-1);
+ err(2, NULL);
if ((cpu_tm = calloc(cpu_count, sizeof(int64_t *))) == NULL ||
(cpu_old = calloc(cpu_count, sizeof(int64_t *))) == NULL ||
(cpu_diff = calloc(cpu_count, sizeof(int64_t *))) == NULL)
- return (-1);
+ err(2, NULL);
for (i = 0; i < cpu_count; i++) {
if ((cpu_tm[i] = calloc(CPUSTATES, sizeof(int64_t))) == NULL ||
(cpu_old[i] = calloc(CPUSTATES, sizeof(int64_t))) == NULL ||
(cpu_diff[i] = calloc(CPUSTATES, sizeof(int64_t))) == NULL)
- return (-1);
+ err(2, NULL);
}
for (v = views_cpu; v->name != NULL; v++)
--
Best Regards
Nan Xiao
[patch] Test for sysctl call fail in usr.bin/systat/pigs.c
Hi tech@, Maybe it is need to check sysctl return value in usr.bin/systat/pigs.c. E.g., in read_pg() function: .. size = sizeof(ctimes); sysctl(cp_time_mib, 2, , , NULL, 0); t = 0; for (i = 0; i < CPUSTATES; i++) t += ctimes[i] - stime[i]; .. If sysctl() failed, the values in ctimes are indeterminate. Apologize if I am wrong. Thanks! Index: pigs.c === RCS file: /cvs/src/usr.bin/systat/pigs.c,v retrieving revision 1.30 diff -u -p -r1.30 pigs.c --- pigs.c 12 Sep 2015 15:59:36 - 1.30 +++ pigs.c 4 May 2018 05:14:13 - @@ -183,7 +183,8 @@ read_pg(void) * and for the imaginary "idle" process */ size = sizeof(ctimes); - sysctl(cp_time_mib, 2, , , NULL, 0); + if (sysctl(cp_time_mib, 2, , , NULL, 0) < 0) + return 1; t = 0; for (i = 0; i < CPUSTATES; i++) @@ -234,13 +235,16 @@ initpigs(void) fixpt_t ccpu; size = sizeof(stime); - sysctl(cp_time_mib, 2, , , NULL, 0); + if (sysctl(cp_time_mib, 2, , , NULL, 0) < 0) + return (-1); size = sizeof(sysload); - sysctl(sysload_mib, 2, , , NULL, 0); + if (sysctl(sysload_mib, 2, , , NULL, 0) < 0) + return (-1); size = sizeof(ccpu); - sysctl(ccpu_mib, 2, , , NULL, 0); + if (sysctl(ccpu_mib, 2, , , NULL, 0) < 0) + return (-1); lccpu = log((double) ccpu / sysload.fscale); -- Best Regards Nan Xiao
[patch] Check memory allocation error in usr.bin/systat/vmstat.c
Hi tech@,
A patch for usr.bin/systat/vmstat.c, apologize if I'm wrong.
Index: vmstat.c
===
RCS file: /cvs/src/usr.bin/systat/vmstat.c,v
retrieving revision 1.82
diff -u -p -r1.82 vmstat.c
--- vmstat.c18 Dec 2016 23:36:32 - 1.82
+++ vmstat.c3 May 2018 06:19:35 -
@@ -180,6 +180,8 @@ initvmstat(void)
intrloc = calloc(nintr, sizeof(long));
intrname = calloc(nintr, sizeof(char *));
+ if (intrloc == NULL || intrname == NULL)
+ errx(2, "out of memory");
for (i = 0; i < nintr; i++) {
char name[128];
--
Best Regards
Nan Xiao(肖楠)
Re: [patch] Add error handling in usr.bin/time/time.c
Hi Ingo,
> No. Read the manual page and /sys/kern/kern_time.c :
>
> With a valid clock id and a valid address, this function
> cannot fail, so you are just adding dead code. Worse,
> you are making future auditors wonder what the heck is
> going on here.
Sorry, I can't find the above words from manual
(https://man.openbsd.org/clock_gettime.2).
Anyway, thanks for pointing it out!
On 4/20/2018 8:00 PM, Ingo Schwarze wrote:
> Hi,
>
> Nan Xiao wrote on Fri, Apr 20, 2018 at 05:28:01PM +0800:
>
>> FYI, thanks!
>
> No. Read the manual page and /sys/kern/kern_time.c :
>
> With a valid clock id and a valid address, this function
> cannot fail, so you are just adding dead code. Worse,
> you are making future auditors wonder what the heck is
> going on here.
>
> Yours,
> Ingo
>
>
>> Index: time.c
>> ===
>> RCS file: /cvs/src/usr.bin/time/time.c,v
>> retrieving revision 1.25
>> diff -u -p -r1.25 time.c
>> --- time.c 21 Aug 2017 13:38:02 - 1.25
>> +++ time.c 20 Apr 2018 09:21:59 -
>> @@ -76,7 +76,8 @@ main(int argc, char *argv[])
>> if (argc < 1)
>> usage();
>>
>> -clock_gettime(CLOCK_MONOTONIC, );
>> +if (clock_gettime(CLOCK_MONOTONIC, ))
>> +err(1, "clock_gettime");
>> switch(pid = vfork()) {
>> case -1:/* error */
>> warn("fork");
>> @@ -92,7 +93,8 @@ main(int argc, char *argv[])
>> signal(SIGQUIT, SIG_IGN);
>> while (wait3(, 0, ) != pid)
>> ;
>> -clock_gettime(CLOCK_MONOTONIC, );
>> +if (clock_gettime(CLOCK_MONOTONIC, ))
>> +err(1, "clock_gettime");
>> if (WIFSIGNALED(status))
>> exitonsig = WTERMSIG(status);
>> if (!WIFEXITED(status))
--
Best Regards
Nan Xiao(肖楠)
Re: [patch] Remove redundant quotes in sys/sys/resource.h and sys/sys/sysctl.h
Hi Theo, I am very sorry for this wrong modification! Thanks! Best Regards Nan Xiao On Fri, Apr 20, 2018 at 4:57 PM, Theo Buehler <t...@openbsd.org> wrote: > I don't think these are "redundant quotes" but rather ditto marks: > > https://en.wikipedia.org/wiki/Ditto_mark > > this is quite clear here, for example: > >> u_int64_t p_uru_nvcsw; /* LONG: voluntary context switches. */ >> - u_int64_t p_uru_nivcsw; /* LONG: involuntary ". */ >> + u_int64_t p_uru_nivcsw; /* LONG: involuntary. */ >
[patch] Add error handling in usr.bin/time/time.c
Hi tech@,
FYI, thanks!
Index: time.c
===
RCS file: /cvs/src/usr.bin/time/time.c,v
retrieving revision 1.25
diff -u -p -r1.25 time.c
--- time.c 21 Aug 2017 13:38:02 - 1.25
+++ time.c 20 Apr 2018 09:21:59 -
@@ -76,7 +76,8 @@ main(int argc, char *argv[])
if (argc < 1)
usage();
- clock_gettime(CLOCK_MONOTONIC, );
+ if (clock_gettime(CLOCK_MONOTONIC, ))
+ err(1, "clock_gettime");
switch(pid = vfork()) {
case -1:/* error */
warn("fork");
@@ -92,7 +93,8 @@ main(int argc, char *argv[])
signal(SIGQUIT, SIG_IGN);
while (wait3(, 0, ) != pid)
;
- clock_gettime(CLOCK_MONOTONIC, );
+ if (clock_gettime(CLOCK_MONOTONIC, ))
+ err(1, "clock_gettime");
if (WIFSIGNALED(status))
exitonsig = WTERMSIG(status);
if (!WIFEXITED(status))
--
Best Regards
Nan Xiao
[patch] Remove redundant quotes in sys/sys/resource.h and sys/sys/sysctl.h
Hi tech@,
FYI, thanks!
Index: resource.h
===
RCS file: /cvs/src/sys/sys/resource.h,v
retrieving revision 1.14
diff -u -p -r1.14 resource.h
--- resource.h 25 Oct 2013 04:42:48 - 1.14
+++ resource.h 20 Apr 2018 08:43:10 -
@@ -61,8 +61,8 @@ structrusage {
longru_maxrss; /* max resident set size */
#defineru_firstru_ixrss
longru_ixrss; /* integral shared text memory size */
- longru_idrss; /* integral unshared data " */
- longru_isrss; /* integral unshared stack " */
+ longru_idrss; /* integral unshared data */
+ longru_isrss; /* integral unshared stack */
longru_minflt; /* page reclaims */
longru_majflt; /* page faults */
longru_nswap; /* swaps */
@@ -72,7 +72,7 @@ structrusage {
longru_msgrcv; /* messages received */
longru_nsignals;/* signals received */
longru_nvcsw; /* voluntary context switches */
- longru_nivcsw; /* involuntary " */
+ longru_nivcsw; /* involuntary */
#defineru_last ru_nivcsw
};
Index: sysctl.h
===
RCS file: /cvs/src/sys/sys/sysctl.h,v
retrieving revision 1.175
diff -u -p -r1.175 sysctl.h
--- sysctl.h12 Oct 2017 09:14:16 - 1.175
+++ sysctl.h20 Apr 2018 08:43:11 -
@@ -407,8 +407,8 @@ struct kinfo_proc {
u_int64_t p_uru_maxrss; /* LONG: max resident set size. */
u_int64_t p_uru_ixrss; /* LONG: integral shared memory size. */
- u_int64_t p_uru_idrss; /* LONG: integral unshared data ". */
- u_int64_t p_uru_isrss; /* LONG: integral unshared stack ". */
+ u_int64_t p_uru_idrss; /* LONG: integral unshared data. */
+ u_int64_t p_uru_isrss; /* LONG: integral unshared stack. */
u_int64_t p_uru_minflt; /* LONG: page reclaims. */
u_int64_t p_uru_majflt; /* LONG: page faults. */
u_int64_t p_uru_nswap; /* LONG: swaps. */
@@ -418,7 +418,7 @@ struct kinfo_proc {
u_int64_t p_uru_msgrcv; /* LONG: messages received. */
u_int64_t p_uru_nsignals; /* LONG: signals received. */
u_int64_t p_uru_nvcsw; /* LONG: voluntary context switches. */
- u_int64_t p_uru_nivcsw; /* LONG: involuntary ". */
+ u_int64_t p_uru_nivcsw; /* LONG: involuntary. */
u_int32_t p_uctime_sec; /* STRUCT TIMEVAL: child u+s time. */
u_int32_t p_uctime_usec;/* STRUCT TIMEVAL: child u+s time. */
--
Best Regards
Nan Xiao
[patch] Chane wfd to static variable in cat.c
Hi tech@,
It seems "wfd" only need get its value when first calling "raw_cat", so
maybe a static variable should be better. Sorry if I am wrong.
Thanks!
Index: cat.c
===
RCS file: /cvs/src/bin/cat/cat.c,v
retrieving revision 1.26
diff -u -p -r1.26 cat.c
--- cat.c 19 Oct 2016 18:20:25 - 1.26
+++ cat.c 11 Apr 2018 10:05:05 -
@@ -223,14 +223,14 @@ raw_args(char **argv)
void
raw_cat(int rfd)
{
- int wfd;
+ static int wfd;
ssize_t nr, nw, off;
static size_t bsize;
static char *buf = NULL;
struct stat sbuf;
- wfd = fileno(stdout);
if (buf == NULL) {
+ wfd = fileno(stdout);
if (fstat(wfd, ))
err(1, "stdout");
bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
--
Best Regards
Nan Xiao(肖楠)
The doubt about "link-layer header type" for loopback packet
Hi tech@, Now, when I use tcpdump file to capture loopback packets on OpenBSD, the "link-layer header type" in global header of pcap file is 0x0C, means "raw IP" packet. I download the file in Windows, and the Wireshark on Windows can't analyze it , and just shows " Raw packet data". After referring https://www.wireshark.org/lists/wireshark-bugs/201502/msg00295.html, I change the "link-layer header type" from 0x0C to 0x6C(108), the pcap file can be decoded successfully (About link type, you can refer http://www.tcpdump.org/linktypes.html). Concerning about the "link-layer header type" of loopback packet, is it reasonable to use "0x6C" instead of "0x0C"? Thanks very much in advance! Best Regards Nan Xiao
Re: [patch]Use BUFSIZE instead of hard-code in netcat.c
Ouch! I misunderstood the patch by @bluhm, please ignore my previous mail! I am very sorry for disrupting! Best Regards Nan Xiao On Tue, Oct 24, 2017 at 9:30 PM, Nan Xiao <xiaonan830...@gmail.com> wrote: > Actually, I think "plen = sizeof(buf);" may be better. > Best Regards > Nan Xiao > > > On Tue, Oct 24, 2017 at 8:52 PM, Alexander Bluhm > <alexander.bl...@gmx.net> wrote: >> On Tue, Oct 24, 2017 at 07:44:02PM +0800, Nan Xiao wrote: >>> Use BUFSIZE instead of hard-code in netcat.c, FYI. Thanks! >> >> As this buffer is used with MSG_PEEK and its content is discarded, >> the size does not really matter. The complicated logic seems to >> be a leftover from the -j jumbo option. >> >> I think this is simpler. >> >> bluhm >> >> Index: usr.bin/nc/netcat.c >> === >> RCS file: /data/mirror/openbsd/cvs/src/usr.bin/nc/netcat.c,v >> retrieving revision 1.187 >> diff -u -p -r1.187 netcat.c >> --- usr.bin/nc/netcat.c 15 Jul 2017 17:27:39 - 1.187 >> +++ usr.bin/nc/netcat.c 24 Oct 2017 12:41:38 - >> @@ -563,13 +563,12 @@ main(int argc, char *argv[]) >> * initially to wait for a caller, then use >> * the regular functions to talk to the >> caller. >> */ >> - int rv, plen; >> - char buf[16384]; >> + int rv; >> + char buf[2048]; >> struct sockaddr_storage z; >> >> len = sizeof(z); >> - plen = 2048; >> - rv = recvfrom(s, buf, plen, MSG_PEEK, >> + rv = recvfrom(s, buf, sizeof(buf), MSG_PEEK, >> (struct sockaddr *), ); >> if (rv < 0) >> err(1, "recvfrom");
Re: [patch]Use BUFSIZE instead of hard-code in netcat.c
Actually, I think "plen = sizeof(buf);" may be better. Best Regards Nan Xiao On Tue, Oct 24, 2017 at 8:52 PM, Alexander Bluhm <alexander.bl...@gmx.net> wrote: > On Tue, Oct 24, 2017 at 07:44:02PM +0800, Nan Xiao wrote: >> Use BUFSIZE instead of hard-code in netcat.c, FYI. Thanks! > > As this buffer is used with MSG_PEEK and its content is discarded, > the size does not really matter. The complicated logic seems to > be a leftover from the -j jumbo option. > > I think this is simpler. > > bluhm > > Index: usr.bin/nc/netcat.c > === > RCS file: /data/mirror/openbsd/cvs/src/usr.bin/nc/netcat.c,v > retrieving revision 1.187 > diff -u -p -r1.187 netcat.c > --- usr.bin/nc/netcat.c 15 Jul 2017 17:27:39 - 1.187 > +++ usr.bin/nc/netcat.c 24 Oct 2017 12:41:38 - > @@ -563,13 +563,12 @@ main(int argc, char *argv[]) > * initially to wait for a caller, then use > * the regular functions to talk to the > caller. > */ > - int rv, plen; > - char buf[16384]; > + int rv; > + char buf[2048]; > struct sockaddr_storage z; > > len = sizeof(z); > - plen = 2048; > - rv = recvfrom(s, buf, plen, MSG_PEEK, > + rv = recvfrom(s, buf, sizeof(buf), MSG_PEEK, > (struct sockaddr *), ); > if (rv < 0) > err(1, "recvfrom");
[patch]Use BUFSIZE instead of hard-code in netcat.c
Hi tech@, Use BUFSIZE instead of hard-code in netcat.c, FYI. Thanks! Best Regards Nan Xiao Index: netcat.c === RCS file: /cvs/src/usr.bin/nc/netcat.c,v retrieving revision 1.187 diff -u -p -r1.187 netcat.c --- netcat.c15 Jul 2017 17:27:39 - 1.187 +++ netcat.c24 Oct 2017 09:03:28 - @@ -564,7 +564,7 @@ main(int argc, char *argv[]) * the regular functions to talk to the caller. */ int rv, plen; - char buf[16384]; + char buf[BUFSIZE]; struct sockaddr_storage z; len = sizeof(z);
[patch] Fix wrong warn message in ldd.c
Hi tech@,
The following check can only know whether the file is ELF or not, can't
know whether it is shared object or executable. So I think the error
message should be more accurate.
Thanks!
Best Regards
Nan Xiao
Index: ldd.c
===
RCS file: /cvs/src/libexec/ld.so/ldd/ldd.c,v
retrieving revision 1.21
diff -u -p -r1.21 ldd.c
--- ldd.c 2 Jul 2017 19:06:12 - 1.21
+++ ldd.c 5 Oct 2017 02:02:31 -
@@ -125,7 +125,7 @@ doit(char *name)
if (memcmp(ehdr.e_ident, ELFMAG, SELFMAG) ||
ehdr.e_machine != ELF_TARG_MACH) {
- warnx("%s: not an ELF executable", name);
+ warnx("%s: not an ELF file", name);
close(fd);
return 1;
}
Why the executable file type is also "DYN", not "EXEC"?
Hi all, I find the type of executable file format on OpenBSD is "DYN", not "EXEC": # readelf -h /usr/bin/ldd ELF Header: Magic: 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 Class: ELF64 Data: 2's complement, little endian Version: 1 (current) OS/ABI:UNIX - System V ABI Version: 0 Type: DYN (Shared object file) .. Is there any special consideration for it? Thanks very much in advance! Best Regards Nan Xiao
[patch] Modify checking read ELF header file condition in ldd.c
Hi tech@,
I think check the actual read ELF header file size is better than just
"<0", thanks!
Best Reagrds
Nan Xiao
Index: ldd.c
===
RCS file: /cvs/src/libexec/ld.so/ldd/ldd.c,v
retrieving revision 1.21
diff -u -p -r1.21 ldd.c
--- ldd.c 2 Jul 2017 19:06:12 - 1.21
+++ ldd.c 4 Oct 2017 03:05:11 -
@@ -117,7 +117,7 @@ doit(char *name)
close(fd);
return 1;
}
- if (read(fd, , sizeof(ehdr)) < 0) {
+ if (read(fd, , sizeof(ehdr)) != sizeof(ehdr)) {
warn("read(%s)", name);
close(fd);
return 1;
[patch] Change size's type from int to size_t in ldd.c
Hi tech@,
I think the type of size variable should be more accurate to use size_t
instead of int.
Best Regards
Nan Xiao
Index: ldd.c
===
RCS file: /cvs/src/libexec/ld.so/ldd/ldd.c,v
retrieving revision 1.21
diff -u -p -r1.21 ldd.c
--- ldd.c 2 Jul 2017 19:06:12 - 1.21
+++ ldd.c 28 Sep 2017 03:16:10 -
@@ -96,7 +96,8 @@ doit(char *name)
{
Elf_Ehdr ehdr;
Elf_Phdr *phdr;
- int fd, i, size, status, interp=0;
+ int fd, i, status, interp=0;
+ size_t size;
char buf[PATH_MAX];
struct stat st;
void * dlhandle;
[patch] Close file descriptor before exiting ldd
Hi tech@,
FYI, thanks!
Best Regards
Nan Xiao
Index: ldd.c
===
RCS file: /cvs/src/libexec/ld.so/ldd/ldd.c,v
retrieving revision 1.21
diff -u -p -r1.21 ldd.c
--- ldd.c 2 Jul 2017 19:06:12 - 1.21
+++ ldd.c 27 Sep 2017 02:21:00 -
@@ -130,8 +130,10 @@ doit(char *name)
return 1;
}
- if ((phdr = reallocarray(NULL, ehdr.e_phnum, sizeof(Elf_Phdr))) == NULL)
+ if ((phdr = reallocarray(NULL, ehdr.e_phnum, sizeof(Elf_Phdr))) ==
NULL) {
+ close(fd);
err(1, "reallocarray");
+ }
size = ehdr.e_phnum * sizeof(Elf_Phdr);
if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
[Patch] Remove unnecessary assignment in pctr.c
Hi tech@ Remove unnecessary assignment. Thanks! Best Regards Nan Xiao Index: pctr.c === RCS file: /cvs/src/usr.bin/pctr/pctr.c,v retrieving revision 1.23 diff -u -p -r1.23 pctr.c --- pctr.c 10 Sep 2017 11:30:43 - 1.23 +++ pctr.c 11 Sep 2017 03:21:21 - @@ -190,7 +190,6 @@ pctr_cpu_creds(void) err(1, "CPU_CPUFEATURE"); /* Get the processor vendor */ - mib[0] = CTL_MACHDEP; mib[1] = CPU_CPUVENDOR; len = sizeof(vendor); if (sysctl(mib, 2, vendor, , NULL, 0) == -1)
Re: [patch] Remove redundant operation in pctr.c
Actually, it should be more efficient to remove bzero() function
and keep set NULL-terminate char.
Best Regards
Nan Xiao
Index: pctr.c
===
RCS file: /cvs/src/usr.bin/pctr/pctr.c,v
retrieving revision 1.22
diff -u -p -r1.22 pctr.c
--- pctr.c 8 Feb 2015 23:40:34 - 1.22
+++ pctr.c 9 Sep 2017 14:05:17 -
@@ -166,7 +166,6 @@ pctr_cpu_creds(void)
mib[0] = CTL_HW;
mib[1] = HW_MACHINE;
len = sizeof(arch) - 1;
- bzero(arch, sizeof(arch));
if (sysctl(mib, 2, arch, , NULL, 0) == -1)
err(1, "HW_MACHINE");
arch[len] = '\0';
@@ -195,7 +194,6 @@ pctr_cpu_creds(void)
mib[0] = CTL_MACHDEP;
mib[1] = CPU_CPUVENDOR;
len = sizeof(vendor) - 1;
- bzero(vendor, sizeof(vendor));
if (sysctl(mib, 2, vendor, , NULL, 0) == -1)
err(1, "CPU_CPUVENDOR");
vendor[len] = '\0';
On 9/9/2017 11:56 AM, Nan Xiao wrote:
> Hi tech@,
>
> Remove setting null-terminated string operation, since bzero()
> has done this operation before. Sorry if I miss some points.
>
> Thanks!
>
> Best Regards
> Nan Xiao
>
> Index: pctr.c
> ===
> RCS file: /cvs/src/usr.bin/pctr/pctr.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 pctr.c
> --- pctr.c8 Feb 2015 23:40:34 - 1.22
> +++ pctr.c9 Sep 2017 03:43:36 -
> @@ -169,7 +169,6 @@ pctr_cpu_creds(void)
> bzero(arch, sizeof(arch));
> if (sysctl(mib, 2, arch, , NULL, 0) == -1)
> err(1, "HW_MACHINE");
> - arch[len] = '\0';
>
> if (strcmp(arch, "i386") == 0)
> atype = ARCH_I386;
> @@ -198,7 +197,6 @@ pctr_cpu_creds(void)
> bzero(vendor, sizeof(vendor));
> if (sysctl(mib, 2, vendor, , NULL, 0) == -1)
> err(1, "CPU_CPUVENDOR");
> - vendor[len] = '\0';
>
> switch (atype) {
> case ARCH_I386:
>
Re: [patch] Initialize "cur" to avoid undefined behavior is dmesg.c
Hi Tom,
Updated, thanks!
Best Regards
Nan Xiao
Index: dmesg.c
===
RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
retrieving revision 1.29
diff -u -p -r1.29 dmesg.c
--- dmesg.c 1 Sep 2017 07:31:45 - 1.29
+++ dmesg.c 4 Sep 2017 13:17:01 -
@@ -65,12 +65,12 @@ main(int argc, char *argv[])
int ch, newl, skip, i;
char *p;
struct msgbuf cur;
- char *memf, *nlistf, *bufdata = NULL;
+ char *memf = NULL, *nlistf = NULL, *bufdata = NULL;
char *allocated = NULL;
int startupmsgs = 0;
char buf[5];
- memf = nlistf = NULL;
+ memset(, 0, sizeof(cur));
while ((ch = getopt(argc, argv, "sM:N:")) != -1)
switch(ch) {
case 's':
On 9/4/2017 8:07 PM, Tom Cosgrove wrote:
>> -free(allocated);
>> +if (allocated)
>> +free(allocated);
>
> This is unnecessary, since free(NULL) is clearly defined as a no-op.
> See the malloc(3) man page.
>
> Tom
>
>>>> Nan Xiao 4-Sep-17 12:11 >>>
>>
>> Hi tech@,
>>
>> This patch fixes the extreme case in dmesg.c: if memf or nlistf is not
>> NULL, and "NOKVM" macro is defined.
>>
>> Current code in dmesg.c:
>>
>> struct msgbuf cur;
>>
>> Since "cur" is not initialized, so the following code has undefined
>> behavior:
>>
>> if (cur.msg_bufx >= cur.msg_bufs)
>> cur.msg_bufx = 0;
>> /*
>> * The message buffer is circular; start at the read pointer, and
>> * go to the write pointer - 1.
>> */
>> for (newl = skip = i = 0, p = bufdata + cur.msg_bufx;
>> i < cur.msg_bufs; i++, p++) {
>> .
>> }
>>
>> My patch can skip the whole loop, and the "dmesg" program just prints
>> a newline:
>>
>> if (!newl)
>> putchar('\n');
>>
>> Best Regards
>> Nan Xiao
>>
>> Index: dmesg.c
>> ===
>> RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
>> retrieving revision 1.29
>> diff -u -p -r1.29 dmesg.c
>> --- dmesg.c 1 Sep 2017 07:31:45 - 1.29
>> +++ dmesg.c 4 Sep 2017 08:55:50 -
>> @@ -65,12 +65,12 @@ main(int argc, char *argv[])
>> int ch, newl, skip, i;
>> char *p;
>> struct msgbuf cur;
>> -char *memf, *nlistf, *bufdata = NULL;
>> +char *memf = NULL, *nlistf = NULL, *bufdata = NULL;
>> char *allocated = NULL;
>> int startupmsgs = 0;
>> char buf[5];
>>
>> -memf = nlistf = NULL;
>> +memset(, 0, sizeof(cur));
>> while ((ch = getopt(argc, argv, "sM:N:")) != -1)
>> switch(ch) {
>> case 's':
>> @@ -184,7 +184,8 @@ main(int argc, char *argv[])
>> }
>> if (!newl)
>> putchar('\n');
>> -free(allocated);
>> +if (allocated)
>> +free(allocated);
>> return (0);
>> }
[patch] Initialize "cur" to avoid undefined behavior is dmesg.c
Hi tech@,
This patch fixes the extreme case in dmesg.c: if memf or nlistf is not
NULL, and "NOKVM" macro is defined.
Current code in dmesg.c:
struct msgbuf cur;
Since "cur" is not initialized, so the following code has undefined
behavior:
if (cur.msg_bufx >= cur.msg_bufs)
cur.msg_bufx = 0;
/*
* The message buffer is circular; start at the read pointer, and
* go to the write pointer - 1.
*/
for (newl = skip = i = 0, p = bufdata + cur.msg_bufx;
i < cur.msg_bufs; i++, p++) {
.
}
My patch can skip the whole loop, and the "dmesg" program just prints
a newline:
if (!newl)
putchar('\n');
Best Regards
Nan Xiao
Index: dmesg.c
===
RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
retrieving revision 1.29
diff -u -p -r1.29 dmesg.c
--- dmesg.c 1 Sep 2017 07:31:45 - 1.29
+++ dmesg.c 4 Sep 2017 08:55:50 -
@@ -65,12 +65,12 @@ main(int argc, char *argv[])
int ch, newl, skip, i;
char *p;
struct msgbuf cur;
- char *memf, *nlistf, *bufdata = NULL;
+ char *memf = NULL, *nlistf = NULL, *bufdata = NULL;
char *allocated = NULL;
int startupmsgs = 0;
char buf[5];
- memf = nlistf = NULL;
+ memset(, 0, sizeof(cur));
while ((ch = getopt(argc, argv, "sM:N:")) != -1)
switch(ch) {
case 's':
@@ -184,7 +184,8 @@ main(int argc, char *argv[])
}
if (!newl)
putchar('\n');
- free(allocated);
+ if (allocated)
+ free(allocated);
return (0);
}
Is it possible for "NOKVM" defined in reality?
Hi tech@,
The code framework of dmesg.c:
..
struct msgbuf cur;
char *allocated = NULL;
..
if (memf == NULL && nlistf == NULL) {
..
} else {
#ifndef NOKVM
..
#endif
}
if (cur.msg_bufx >= cur.msg_bufs)
cur.msg_bufx = 0;
..
free(allocated);
..
In reality, is there possible to have "NOKVM" defined (in else branch)?
If so, the "cur" variable will be messy, and "allocated" will be NULL.
Hope to check it, thanks!
Best Regards
Nan Xiao
[patch] Fix memory leak in dmeg.c
Hi tech@,
I am not a nitpicker. Though all memory will be reclaimed after process
exits, I still think it's a good habit to release all memory in heap.
Best Regards
Nan Xiao
Index: dmesg.c
===
RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
retrieving revision 1.28
diff -u -p -r1.28 dmesg.c
--- dmesg.c 26 Aug 2017 08:53:20 - 1.28
+++ dmesg.c 31 Aug 2017 14:22:27 -
@@ -34,6 +34,7 @@
#include
#include
+#include
#include
#include
#include
@@ -183,6 +184,14 @@ main(int argc, char *argv[])
}
if (!newl)
putchar('\n');
+
+ if (memf == NULL && nlistf == NULL)
+ free(bufdata - offsetof(struct msgbuf, msg_bufc));
+ else {
+#ifndef NOKVM
+ free(bufdata);
+#endif
+ }
return (0);
}
