Re: flex: dead code around flex_die()
Ping, in case anyone would like to comment.
On Thu, Oct 31, 2019 at 08:55:46AM +0800, Michael Mikonos wrote:
> Hello,
>
> The macro flex_die(), defined in flexdef.h, never returns.
> Instead it does FLEX_EXIT() which longjmp()s back to flex_main()
> before exiting.
> The following patch removes some dead code, as statements
> after flex_die() are never reached. Does this look OK?
>
> - Michael
>
>
> Index: tables.c
> ===
> RCS file: /cvs/src/usr.bin/lex/tables.c,v
> retrieving revision 1.4
> diff -u -p -u -r1.4 tables.c
> --- tables.c 17 Aug 2017 19:27:09 - 1.4
> +++ tables.c 30 Oct 2019 12:32:59 -
> @@ -221,34 +221,26 @@ int yytbl_data_fwrite (struct yytbl_writ
> default:
> flex_die (_("invalid td_flags detected"));
> }
> - if (rv < 0) {
> + if (rv < 0)
> flex_die (_("error while writing tables"));
> - return -1;
> - }
> bwritten += rv;
> }
>
> /* Sanity check */
> - if (bwritten != (int) (12 + total_len * YYTDFLAGS2BYTES
> (td->td_flags))) {
> + if (bwritten != (int) (12 + total_len * YYTDFLAGS2BYTES (td->td_flags)))
> flex_die (_("insanity detected"));
> - return -1;
> - }
>
> /* add padding */
> - if ((rv = yytbl_write_pad64 (wr)) < 0) {
> + if ((rv = yytbl_write_pad64 (wr)) < 0)
> flex_die (_("pad64 failed"));
> - return -1;
> - }
> bwritten += rv;
>
> /* Now go back and update the th_hsize member */
> if (fgetpos (wr->out, ) != 0
> || fsetpos (wr->out, &(wr->th_ssize_pos)) != 0
> || yytbl_write32 (wr, wr->total_written) < 0
> - || fsetpos (wr->out, )) {
> + || fsetpos (wr->out, ))
> flex_die (_("get|set|fwrite32 failed"));
> - return -1;
> - }
> else
> /* Don't count the int we just wrote. */
> wr->total_written -= sizeof (flex_int32_t);
> @@ -346,7 +338,6 @@ static flex_int32_t yytbl_data_geti (con
> return ((flex_int32_t *) (tbl->td_data))[i];
> default:
> flex_die (_("invalid td_flags detected"));
> - break;
> }
> return 0;
> }
> @@ -374,7 +365,6 @@ static void yytbl_data_seti (const struc
> break;
> default:
> flex_die (_("invalid td_flags detected"));
> - break;
> }
> }
>
> @@ -433,10 +423,8 @@ void yytbl_data_compress (struct yytbl_d
> /* No change in this table needed. */
> return;
>
> - if (newsz > (int) YYTDFLAGS2BYTES (tbl->td_flags)) {
> + if (newsz > (int) YYTDFLAGS2BYTES (tbl->td_flags))
> flex_die (_("detected negative compression"));
> - return;
> - }
>
> total_len = yytbl_calc_total_len (tbl);
> newtbl.td_data = calloc (total_len, newsz);
>
flex: dead code around flex_die()
Hello,
The macro flex_die(), defined in flexdef.h, never returns.
Instead it does FLEX_EXIT() which longjmp()s back to flex_main()
before exiting.
The following patch removes some dead code, as statements
after flex_die() are never reached. Does this look OK?
- Michael
Index: tables.c
===
RCS file: /cvs/src/usr.bin/lex/tables.c,v
retrieving revision 1.4
diff -u -p -u -r1.4 tables.c
--- tables.c17 Aug 2017 19:27:09 - 1.4
+++ tables.c30 Oct 2019 12:32:59 -
@@ -221,34 +221,26 @@ int yytbl_data_fwrite (struct yytbl_writ
default:
flex_die (_("invalid td_flags detected"));
}
- if (rv < 0) {
+ if (rv < 0)
flex_die (_("error while writing tables"));
- return -1;
- }
bwritten += rv;
}
/* Sanity check */
- if (bwritten != (int) (12 + total_len * YYTDFLAGS2BYTES
(td->td_flags))) {
+ if (bwritten != (int) (12 + total_len * YYTDFLAGS2BYTES (td->td_flags)))
flex_die (_("insanity detected"));
- return -1;
- }
/* add padding */
- if ((rv = yytbl_write_pad64 (wr)) < 0) {
+ if ((rv = yytbl_write_pad64 (wr)) < 0)
flex_die (_("pad64 failed"));
- return -1;
- }
bwritten += rv;
/* Now go back and update the th_hsize member */
if (fgetpos (wr->out, ) != 0
|| fsetpos (wr->out, &(wr->th_ssize_pos)) != 0
|| yytbl_write32 (wr, wr->total_written) < 0
- || fsetpos (wr->out, )) {
+ || fsetpos (wr->out, ))
flex_die (_("get|set|fwrite32 failed"));
- return -1;
- }
else
/* Don't count the int we just wrote. */
wr->total_written -= sizeof (flex_int32_t);
@@ -346,7 +338,6 @@ static flex_int32_t yytbl_data_geti (con
return ((flex_int32_t *) (tbl->td_data))[i];
default:
flex_die (_("invalid td_flags detected"));
- break;
}
return 0;
}
@@ -374,7 +365,6 @@ static void yytbl_data_seti (const struc
break;
default:
flex_die (_("invalid td_flags detected"));
- break;
}
}
@@ -433,10 +423,8 @@ void yytbl_data_compress (struct yytbl_d
/* No change in this table needed. */
return;
- if (newsz > (int) YYTDFLAGS2BYTES (tbl->td_flags)) {
+ if (newsz > (int) YYTDFLAGS2BYTES (tbl->td_flags))
flex_die (_("detected negative compression"));
- return;
- }
total_len = yytbl_calc_total_len (tbl);
newtbl.td_data = calloc (total_len, newsz);
flex HAVE_DECL___FUNC__
Hello, When reviewing flex's config.h I think we could enable HAVE_DECL___FUNC__. This allows the flex_die() macro to include __func__ in its output. For example: flex: fatal internal error at main.c:146 (flex_main): spooky error Does this look OK? - Michael Index: config.h === RCS file: /cvs/src/usr.bin/lex/config.h,v retrieving revision 1.6 diff -u -p -u -r1.6 config.h --- config.h16 Sep 2019 17:30:16 - 1.6 +++ config.h29 Oct 2019 02:23:17 - @@ -136,6 +136,8 @@ #define HAVE_ASSERT_H 1 +#define HAVE_DECL___FUNC__ 1 + /* Define to 1 if the system has the type `_Bool'. */ #define HAVE__BOOL 1
Re: assert() triggered in flex
On Tue, Sep 17, 2019 at 11:56:26PM +0800, Michael Mikonos wrote:
> Hello,
>
> I was wondering why fuzz input was sometimes crashing flex(1).
> Now that assert() is enabled in the build I am able to
> trigger it with the following input.
>
> $ cat test.l
> %{
> hello
> %}
>
> %%
>
> )))
> world
> $ /usr/bin/lex -d - < test.l
> assertion "_sf_top_ix > 0" failed: file "scanflags.c", line 55, function
> "sf_pop"
> Abort trap (core dumped)
>
> $ gdb -c lex.core -e /usr/bin/lex
> GNU gdb 6.3
> Copyright 2004 Free Software Foundation, Inc.
> GDB is free software, covered by the GNU General Public License, and you are
> welcome to change it and/or distribute copies of it under certain conditions.
> Type "show copying" to see the conditions.
> There is absolutely no warranty for GDB. Type "show warranty" for details.
> This GDB was configured as "i386-unknown-openbsd6.6".
> Core was generated by `lex'.
> Program terminated with signal 6, Aborted.
> Loaded symbols for /usr/bin/lex.assert.debug
> Reading symbols from /usr/lib/libm.so.10.1...done.
> Loaded symbols for /usr/lib/libm.so.10.1
> Reading symbols from /usr/lib/libc.so.95.1...done.
> Loaded symbols for /usr/lib/libc.so.95.1
> Reading symbols from /usr/libexec/ld.so...done.
> Loaded symbols for /usr/libexec/ld.so
> #0 thrkill () at -:3
> 3 -: No such file or directory.
> in -
> (gdb) bt
> #0 thrkill () at -:3
> #1 0x0d0fce40 in _libc_raise (s=6) at /usr/src/lib/libc/gen/raise.c:37
> #2 0x0d104e3b in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51
> #3 0x0d0c3b30 in _libc___assert2 (file=0x1a14d90e "scanflags.c", line=55,
> func=0x1a1526a2 "sf_pop", failedexpr=0x1a158462 "_sf_top_ix > 0")
> at /usr/src/lib/libc/gen/assert.c:52
> #4 0x1a189024 in sf_pop () at scanflags.c:55
> #5 0x1a186eec in flexscan () at scan.l:744
> #6 0x1a18cfa6 in yylex () at yylex.c:52
> #7 0x1a17ffc5 in yyparse () at parse.c:687
> #8 0x1a179a97 in readin () at main.c:1445
> #9 0x1a178d81 in flex_main (argc=3, argv=0xcf7e8d74) at main.c:175
> #10 0x1a17b535 in main (argc=3, argv=0xcf7e8d74) at main.c:223
> Current language: auto; currently asm
>
> Stack trace from gdb shows flexscan() calls sf_pop() but the stack _sf_stk
> is empty and has nothing to pop.
> flexscan() calls sf_push() when it sees "(", but fails to check the size of
> _sf_stk before calling sf_pop() when ")" is seen.
> The diff below prints a syntax error message instead of aborting in this case.
> It looks like this for the test file:
>
> $ lex -d - < test.l
> :7: ')' with no matching '('
>
> Before assert() was enabled, calling sf_pop() would wrap the array
> index _sf_top_ix from 0 to 0x and a SEGV could happen later.
> Does this look OK?
>
> - Michael
>
>
> Index: scan.l
> ===
> RCS file: /cvs/src/usr.bin/lex/scan.l,v
> retrieving revision 1.12
> diff -u -p -u -r1.12 scan.l
> --- scan.l19 Nov 2015 23:34:56 - 1.12
> +++ scan.l17 Sep 2019 15:35:27 -
> @@ -741,7 +741,14 @@ nmstr[yyleng - 2 - end_is_ws] = '\0'; /
> return '(';
> }
> "(" sf_push(); return '(';
> -")" sf_pop(); return ')';
> +")" {
> +if (_sf_top_ix == 0) {
> +synerr( _("')' with no matching '('\n"));
> +FLEX_EXIT(EXIT_FAILURE);
> +}
> +sf_pop();
> +return ')';
> +}
>
> [/|*+?.(){}]return (unsigned char) yytext[0];
> . RETURNCHAR;
When I remembered to check if upstream flex had fixed the sf_pop()
issue I found their fix was a little different:
https://github.com/westes/flex/commit/9ba6e5283efd2fe454d3bc92eca960b3ebd91294
Notably, FLEX_EXIT() is not called so flexscan() won't immediately
terminate. This allows other syntax errors to be reported.
The following patch applies what they have.
Index: scan.l
===
RCS file: /cvs/src/usr.bin/lex/scan.l,v
retrieving revision 1.12
diff -u -p -u -r1.12 scan.l
--- scan.l 19 Nov 2015 23:34:56 - 1.12
+++ scan.l 21 Sep 2019 12:08:36 -
@@ -741,7 +741,13 @@ nmstr[yyleng - 2 - end_is_ws] = '\0'; /
return '(';
}
"(" sf_push(); return '(';
-")" sf_pop(); return ')';
+")" {
+if (_sf_top_ix > 0) {
+sf_pop();
+return ')';
+} else
+synerr(_("unbalanced parenthesis"));
+}
[/|*+?.(){}]return (unsigned char) yytext[0];
. RETURNCHAR;
assert() triggered in flex
Hello,
I was wondering why fuzz input was sometimes crashing flex(1).
Now that assert() is enabled in the build I am able to
trigger it with the following input.
$ cat test.l
%{
hello
%}
%%
)))
world
$ /usr/bin/lex -d - < test.l
assertion "_sf_top_ix > 0" failed: file "scanflags.c", line 55, function
"sf_pop"
Abort trap (core dumped)
$ gdb -c lex.core -e /usr/bin/lex
GNU gdb 6.3
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB. Type "show warranty" for details.
This GDB was configured as "i386-unknown-openbsd6.6".
Core was generated by `lex'.
Program terminated with signal 6, Aborted.
Loaded symbols for /usr/bin/lex.assert.debug
Reading symbols from /usr/lib/libm.so.10.1...done.
Loaded symbols for /usr/lib/libm.so.10.1
Reading symbols from /usr/lib/libc.so.95.1...done.
Loaded symbols for /usr/lib/libc.so.95.1
Reading symbols from /usr/libexec/ld.so...done.
Loaded symbols for /usr/libexec/ld.so
#0 thrkill () at -:3
3 -: No such file or directory.
in -
(gdb) bt
#0 thrkill () at -:3
#1 0x0d0fce40 in _libc_raise (s=6) at /usr/src/lib/libc/gen/raise.c:37
#2 0x0d104e3b in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51
#3 0x0d0c3b30 in _libc___assert2 (file=0x1a14d90e "scanflags.c", line=55,
func=0x1a1526a2 "sf_pop", failedexpr=0x1a158462 "_sf_top_ix > 0")
at /usr/src/lib/libc/gen/assert.c:52
#4 0x1a189024 in sf_pop () at scanflags.c:55
#5 0x1a186eec in flexscan () at scan.l:744
#6 0x1a18cfa6 in yylex () at yylex.c:52
#7 0x1a17ffc5 in yyparse () at parse.c:687
#8 0x1a179a97 in readin () at main.c:1445
#9 0x1a178d81 in flex_main (argc=3, argv=0xcf7e8d74) at main.c:175
#10 0x1a17b535 in main (argc=3, argv=0xcf7e8d74) at main.c:223
Current language: auto; currently asm
Stack trace from gdb shows flexscan() calls sf_pop() but the stack _sf_stk
is empty and has nothing to pop.
flexscan() calls sf_push() when it sees "(", but fails to check the size of
_sf_stk before calling sf_pop() when ")" is seen.
The diff below prints a syntax error message instead of aborting in this case.
It looks like this for the test file:
$ lex -d - < test.l
:7: ')' with no matching '('
Before assert() was enabled, calling sf_pop() would wrap the array
index _sf_top_ix from 0 to 0x and a SEGV could happen later.
Does this look OK?
- Michael
Index: scan.l
===
RCS file: /cvs/src/usr.bin/lex/scan.l,v
retrieving revision 1.12
diff -u -p -u -r1.12 scan.l
--- scan.l 19 Nov 2015 23:34:56 - 1.12
+++ scan.l 17 Sep 2019 15:35:27 -
@@ -741,7 +741,14 @@ nmstr[yyleng - 2 - end_is_ws] = '\0'; /
return '(';
}
"(" sf_push(); return '(';
-")" sf_pop(); return ')';
+")" {
+if (_sf_top_ix == 0) {
+synerr( _("')' with no matching '('\n"));
+FLEX_EXIT(EXIT_FAILURE);
+}
+sf_pop();
+return ')';
+}
[/|*+?.(){}]return (unsigned char) yytext[0];
. RETURNCHAR;
Re: assert() missing in flex
On Mon, Sep 16, 2019 at 11:38:30AM +0200, Marc Espie wrote:
> On Mon, Sep 16, 2019 at 05:07:21PM +0800, Michael Mikonos wrote:
> > Hello,
> >
> > When building flex on clang 8.0.1 (i386) I noticed that assert()
> > expands to nothing. This happens because of a fallback declaration
> > of assert() in flexdef.h when HAVE_ASSERT_H is not set. Instead of
> > changing flexdef.h the following patch follows the existing pattern
> > in Makefile of adding -DHAVE_SOMETHING. Does this look OK?
> >
> > - Michael
> >
> >
> > Index: Makefile
> > ===
> > RCS file: /cvs/src/usr.bin/lex/Makefile,v
> > retrieving revision 1.17
> > diff -u -p -u -r1.17 Makefile
> > --- Makefile30 Apr 2017 20:30:39 - 1.17
> > +++ Makefile16 Sep 2019 08:55:42 -
> > @@ -12,7 +12,7 @@
> > # To bootstrap lex, cp initscan.c to scan.c and run make.
> >
> > PROG= lex
> > -CFLAGS+=-I. -I${.CURDIR} -DHAVE_CONFIG_H
> > +CFLAGS+=-I. -I${.CURDIR} -DHAVE_CONFIG_H -DHAVE_ASSERT_H
> > SRCS= buf.c ccl.c dfa.c ecs.c filter.c gen.c main.c misc.c \
> > nfa.c options.c parse.y regex.c scan.l scanflags.c \
> > scanopt.c skel.c sym.c tables.c tables_shared.c \
>
> Nope, you want to fix config.h
Thanks for the suggestion. I missed that config.h already contained
other HAVE_* definitions. This patch has the same effect; config.h
is updated manually but it has happened before in r1.5.
Index: config.h
===
RCS file: /cvs/src/usr.bin/lex/config.h,v
retrieving revision 1.5
diff -u -p -u -r1.5 config.h
--- config.h19 Nov 2015 23:48:06 - 1.5
+++ config.h16 Sep 2019 09:46:36 -
@@ -137,6 +137,8 @@
/* Define to 1 if the system has the type `_Bool'. */
#define HAVE__BOOL 1
+#define HAVE_ASSERT_H 1
+
/* Define to the sub-directory in which libtool stores uninstalled libraries.
*/
#define LT_OBJDIR ".libs/"
assert() missing in flex
Hello,
When building flex on clang 8.0.1 (i386) I noticed that assert()
expands to nothing. This happens because of a fallback declaration
of assert() in flexdef.h when HAVE_ASSERT_H is not set. Instead of
changing flexdef.h the following patch follows the existing pattern
in Makefile of adding -DHAVE_SOMETHING. Does this look OK?
- Michael
Index: Makefile
===
RCS file: /cvs/src/usr.bin/lex/Makefile,v
retrieving revision 1.17
diff -u -p -u -r1.17 Makefile
--- Makefile30 Apr 2017 20:30:39 - 1.17
+++ Makefile16 Sep 2019 08:55:42 -
@@ -12,7 +12,7 @@
# To bootstrap lex, cp initscan.c to scan.c and run make.
PROG= lex
-CFLAGS+=-I. -I${.CURDIR} -DHAVE_CONFIG_H
+CFLAGS+=-I. -I${.CURDIR} -DHAVE_CONFIG_H -DHAVE_ASSERT_H
SRCS= buf.c ccl.c dfa.c ecs.c filter.c gen.c main.c misc.c \
nfa.c options.c parse.y regex.c scan.l scanflags.c \
scanopt.c skel.c sym.c tables.c tables_shared.c \
Re: flex {c,m}alloc() checks
> I'd say go for the x* solution,
>
> -Otto
Sure. When I looked at this again there are also realloc() return
value checks missing, so I added xcalloc(), xmalloc() and xrealloc().
An old (unused) function yy_flex_xmalloc() gets removed. When building
this I checked the resulting .o files using nm(1) to make sure I didn't
miss any direct calls to calloc/malloc/realloc. However, scan.o and
parse.o were left alone. Does this look better?
Index: flexdef.h
===
RCS file: /cvs/src/usr.bin/lex/flexdef.h,v
retrieving revision 1.15
diff -u -r1.15 flexdef.h
--- flexdef.h 19 Nov 2015 23:48:06 - 1.15
+++ flexdef.h 28 Aug 2019 13:23:15 -
@@ -920,8 +920,9 @@
/* Output a yy_trans_info structure. */
extern void transition_struct_out PROTO ((int, int));
-/* Only needed when using certain broken versions of bison to build parse.c. */
-extern void *yy_flex_xmalloc PROTO ((int));
+extern void *xmalloc PROTO ((size_t));
+extern void *xcalloc PROTO ((size_t, size_t));
+extern void *xrealloc PROTO ((void *, size_t));
/* from file nfa.c */
Index: buf.c
===
RCS file: /cvs/src/usr.bin/lex/buf.c,v
retrieving revision 1.7
diff -u -r1.7 buf.c
--- buf.c 20 Nov 2015 18:54:49 - 1.7
+++ buf.c 28 Aug 2019 13:23:15 -
@@ -79,9 +79,7 @@
size_t tsz;
tsz = strlen(fmt) + strlen(s) + 1;
- t = malloc(tsz);
- if (!t)
- flexfatal(_("Allocation of buffer to print string failed"));
+ t = xmalloc(tsz);
snprintf(t, tsz, fmt, s);
buf = buf_strappend(buf, t);
free(t);
@@ -105,9 +103,7 @@
2 * strlen(filename) + /* filename with possibly all
backslashes escaped */
(int) (1 + log10(abs(lineno))) +/* line number */
1; /* NUL */
- t = malloc(tsz);
- if (!t)
- flexfatal(_("Allocation of buffer for line directive failed"));
+ t = xmalloc(tsz);
dst = t + snprintf(t, tsz, "#line %d \"", lineno);
for (src = filename; *src; *dst++ = *src++)
if (*src == '\\') /* escape backslashes */
@@ -181,10 +177,7 @@
val = val ? val : "";
strsz = strlen(fmt) + strlen(def) + strlen(val) + 2;
- str = malloc(strsz);
- if (!str)
- flexfatal(_("Allocation of buffer for m4 def failed"));
-
+ str = xmalloc(strsz);
snprintf(str, strsz, fmt, def, val);
buf_append(buf, , 1);
return buf;
@@ -203,10 +196,7 @@
size_t strsz;
strsz = strlen(fmt) + strlen(def) + 2;
- str = malloc(strsz);
- if (!str)
- flexfatal(_("Allocation of buffer for m4 undef failed"));
-
+ str = xmalloc(strsz);
snprintf(str, strsz, fmt, def);
buf_append(buf, , 1);
return buf;
Index: dfa.c
===
RCS file: /cvs/src/usr.bin/lex/dfa.c,v
retrieving revision 1.8
diff -u -r1.8 dfa.c
--- dfa.c 19 Nov 2015 23:20:34 - 1.8
+++ dfa.c 28 Aug 2019 13:23:15 -
@@ -523,15 +523,12 @@
* So we'll have to realloc() on the way...
* we'll wait until we can calculate yynxt_tbl->td_hilen.
*/
- yynxt_tbl =
- (struct yytbl_data *) calloc (1,
- sizeof (struct
- yytbl_data));
+ yynxt_tbl = xcalloc(1, sizeof(struct yytbl_data));
yytbl_data_init (yynxt_tbl, YYTD_ID_NXT);
yynxt_tbl->td_hilen = 1;
yynxt_tbl->td_lolen = num_full_table_rows;
yynxt_tbl->td_data = yynxt_data =
- (flex_int32_t *) calloc (yynxt_tbl->td_lolen *
+ xcalloc(yynxt_tbl->td_lolen *
yynxt_tbl->td_hilen,
sizeof (flex_int32_t));
yynxt_curr = 0;
@@ -715,7 +712,7 @@
/* Each time we hit here, it's another td_hilen, so we
realloc. */
yynxt_tbl->td_hilen++;
yynxt_tbl->td_data = yynxt_data =
- (flex_int32_t *) realloc (yynxt_data,
+ xrealloc(yynxt_data,
yynxt_tbl->td_hilen *
yynxt_tbl->td_lolen *
sizeof (flex_int32_t));
Index: filter.c
===
RCS file: /cvs/src/usr.bin/lex/filter.c,v
retrieving revision 1.9
diff -u -r1.9 filter.c
--- filter.c30 Aug 2017 02:54:07 - 1.9
+++ filter.c28 Aug 2019
Re: flex {c,m}alloc() checks
On Sun, Aug 25, 2019 at 02:58:47PM +0200, Otto Moerbeek wrote: > On Sun, Aug 25, 2019 at 08:32:04PM +0800, Michael Mikonos wrote: > > > Hello, > > > > I noticed that flex is too trusting and assumes > > calloc/malloc will always succeed. Hopefully I > > caught all of them. > > I tried to follow the existing idiom of > > calling flexerror() and passing strings via > > the _() macro. OK? > > Does upstream have anything like this? You could consider using the > xmalloc idiom (i.e. have separate functions that do the checks). Upstream has the _() macro and also calls flexerror() on allocation failure. To me it is also nicer adding an xmalloc/xcalloc. That would be a bigger patch though as the calls currently checking malloc/calloc return value get modified too. - Michael
flex {c,m}alloc() checks
Hello,
I noticed that flex is too trusting and assumes
calloc/malloc will always succeed. Hopefully I
caught all of them.
I tried to follow the existing idiom of
calling flexerror() and passing strings via
the _() macro. OK?
- Michael
Index: dfa.c
===
RCS file: /cvs/src/usr.bin/lex/dfa.c,v
retrieving revision 1.8
diff -u -p -U4 -r1.8 dfa.c
--- dfa.c 19 Nov 2015 23:20:34 - 1.8
+++ dfa.c 25 Aug 2019 12:09:54 -
@@ -526,15 +526,19 @@ void ntod ()
yynxt_tbl =
(struct yytbl_data *) calloc (1,
sizeof (struct
yytbl_data));
+ if (yynxt_tbl == NULL)
+ flexerror(_("calloc failed"));
yytbl_data_init (yynxt_tbl, YYTD_ID_NXT);
yynxt_tbl->td_hilen = 1;
yynxt_tbl->td_lolen = num_full_table_rows;
yynxt_tbl->td_data = yynxt_data =
(flex_int32_t *) calloc (yynxt_tbl->td_lolen *
yynxt_tbl->td_hilen,
sizeof (flex_int32_t));
+ if (yynxt_tbl->td_data == NULL)
+ flexerror(_("calloc failed"));
yynxt_curr = 0;
buf_prints (_buf,
"\t{YYTD_ID_NXT, (void**)_nxt, sizeof(%s)},\n",
Index: gen.c
===
RCS file: /cvs/src/usr.bin/lex/gen.c,v
retrieving revision 1.15
diff -u -p -U4 -r1.15 gen.c
--- gen.c 19 Nov 2015 23:28:03 - 1.15
+++ gen.c 25 Aug 2019 12:09:55 -
@@ -111,13 +111,17 @@ mkeoltbl(void)
flex_int8_t *tdata = NULL;
struct yytbl_data *tbl;
tbl = calloc(1, sizeof(struct yytbl_data));
+ if (tbl == NULL)
+ flexerror(_("calloc failed"));
yytbl_data_init(tbl, YYTD_ID_RULE_CAN_MATCH_EOL);
tbl->td_flags = YYTD_DATA8;
tbl->td_lolen = num_rules + 1;
tbl->td_data = tdata =
calloc(tbl->td_lolen, sizeof(flex_int8_t));
+ if (tbl->td_data == NULL)
+ flexerror(_("calloc failed"));
for (i = 1; i <= num_rules; i++)
tdata[i] = rule_has_nl[i] ? 1 : 0;
@@ -223,15 +227,19 @@ mkctbl(void)
((tblend + numecs + 1) >= INT16_MAX
|| long_align) ? "flex_int32_t" : "flex_int16_t");
tbl = calloc(1, sizeof(struct yytbl_data));
+ if (tbl == NULL)
+ flexerror(_("calloc failed"));
yytbl_data_init(tbl, YYTD_ID_TRANSITION);
tbl->td_flags = YYTD_DATA32 | YYTD_STRUCT;
tbl->td_hilen = 0;
tbl->td_lolen = tblend + numecs + 1;/* number of structs */
tbl->td_data = tdata =
calloc(tbl->td_lolen * 2, sizeof(flex_int32_t));
+ if (tbl->td_data == NULL)
+ flexerror(_("calloc failed"));
/*
* We want the transition to be represented as the offset to the next
* state, not the actual state number, which is what it currently is.
@@ -318,15 +326,19 @@ mkssltbl(void)
flex_int32_t *tdata = NULL;
flex_int32_t i;
tbl = calloc(1, sizeof(struct yytbl_data));
+ if (tbl == NULL)
+ flexerror(_("calloc failed"));
yytbl_data_init(tbl, YYTD_ID_START_STATE_LIST);
tbl->td_flags = YYTD_DATA32 | YYTD_PTRANS;
tbl->td_hilen = 0;
tbl->td_lolen = lastsc * 2 + 1;
tbl->td_data = tdata =
calloc(tbl->td_lolen, sizeof(flex_int32_t));
+ if (tbl->td_data == NULL)
+ flexerror(_("calloc failed"));
for (i = 0; i <= lastsc * 2; ++i)
tdata[i] = base[i];
@@ -452,15 +464,19 @@ mkecstbl(void)
struct yytbl_data *tbl = NULL;
flex_int32_t *tdata = NULL;
tbl = calloc(1, sizeof(struct yytbl_data));
+ if (tbl == NULL)
+ flexerror(_("calloc failed"));
yytbl_data_init(tbl, YYTD_ID_EC);
tbl->td_flags |= YYTD_DATA32;
tbl->td_hilen = 0;
tbl->td_lolen = csize;
tbl->td_data = tdata =
calloc(tbl->td_lolen, sizeof(flex_int32_t));
+ if (tbl->td_data == NULL)
+ flexerror(_("calloc failed"));
for (i = 1; i < csize; ++i) {
ecgroup[i] = ABS(ecgroup[i]);
tdata[i] = ecgroup[i];
@@ -659,16 +675,19 @@ mkftbl(void)
struct yytbl_data *tbl;
flex_int32_t *tdata = NULL;
tbl = calloc(1, sizeof(struct yytbl_data));
+ if (tbl == NULL)
+ flexerror(_("calloc failed"));
yytbl_data_init(tbl, YYTD_ID_ACCEPT);
tbl->td_flags |= YYTD_DATA32;
tbl->td_hilen = 0; /* it's a one-dimensional array */
tbl->td_lolen = lastdfa + 1;
ansify flex
Hello,
Upstream flex already updated function declarations to ANSI.
The following patch applies this change to the in-tree version.
Does it look OK?
- Michael
Index: ccl.c
===
RCS file: /cvs/src/usr.bin/lex/ccl.c,v
retrieving revision 1.8
diff -u -p -u -r1.8 ccl.c
--- ccl.c 19 Nov 2015 22:55:13 - 1.8
+++ ccl.c 25 Aug 2019 08:43:59 -
@@ -55,9 +55,7 @@ ccl_contains(const int cclp, const int c
/* ccladd - add a single character to a ccl */
void
-ccladd(cclp, ch)
- int cclp;
- int ch;
+ccladd(int cclp, int ch)
{
int ind, len, newpos, i;
@@ -190,7 +188,7 @@ ccl_set_union(int a, int b)
/* cclinit - return an empty ccl */
int
-cclinit()
+cclinit(void)
{
if (++lastccl >= current_maxccls) {
current_maxccls += MAX_CCLS_INCREMENT;
@@ -231,8 +229,7 @@ cclinit()
/* cclnegate - negate the given ccl */
void
-cclnegate(cclp)
- int cclp;
+cclnegate(int cclp)
{
cclng[cclp] = 1;
ccl_has_nl[cclp] = !ccl_has_nl[cclp];
@@ -247,9 +244,7 @@ cclnegate(cclp)
*/
void
-list_character_set(file, cset)
- FILE *file;
- int cset[];
+list_character_set(FILE *file, int cset[])
{
int i;
Index: dfa.c
===
RCS file: /cvs/src/usr.bin/lex/dfa.c,v
retrieving revision 1.8
diff -u -p -u -r1.8 dfa.c
--- dfa.c 19 Nov 2015 23:20:34 - 1.8
+++ dfa.c 25 Aug 2019 08:43:59 -
@@ -51,9 +51,8 @@ int symfollowset PROTO ((int[], int, int
* indexed by equivalence class.
*/
-void check_for_backing_up (ds, state)
- int ds;
- int state[];
+void
+check_for_backing_up(int ds, int state[])
{
if ((reject && !dfaacc[ds].dfaacc_set) || (!reject &&
!dfaacc[ds].dfaacc_state)) { /* state is non-accepting */
++num_backing_up;
@@ -98,10 +97,8 @@ void check_for_backing_up (ds, state)
*accset[1 .. nacc] is the list of accepting numbers for the DFA state.
*/
-void check_trailing_context (nfa_states, num_states, accset, nacc)
- int*nfa_states, num_states;
- int*accset;
- int nacc;
+void
+check_trailing_context(int *nfa_states, int num_states, int *accset, int nacc)
{
int i, j;
@@ -139,9 +136,8 @@ void check_trailing_context (nfa_states,
* and writes a report to the given file.
*/
-void dump_associated_rules (file, ds)
- FILE *file;
- int ds;
+void
+dump_associated_rules(FILE *file, int ds)
{
int i, j;
int num_associated_rules = 0;
@@ -189,9 +185,8 @@ void dump_associated_rules (file, ds)
* is done to the given file.
*/
-void dump_transitions (file, state)
- FILE *file;
- int state[];
+void
+dump_transitions(FILE *file, int state[])
{
int i, ec;
int out_char_set[CSIZE];
@@ -237,8 +232,8 @@ void dump_transitions (file, state)
* hashval is the hash value for the dfa corresponding to the state set.
*/
-int*epsclosure (t, ns_addr, accset, nacc_addr, hv_addr)
- int*t, *ns_addr, accset[], *nacc_addr, *hv_addr;
+int *
+epsclosure(int *t, int *ns_addr, int accset[], int *nacc_addr, int *hv_addr)
{
int stkpos, ns, tsp;
int numstates = *ns_addr, nacc, hashval, transsym, nfaccnum;
@@ -353,7 +348,8 @@ ADD_STATE(state); \
/* increase_max_dfas - increase the maximum number of DFAs */
-void increase_max_dfas ()
+void
+increase_max_dfas(void)
{
current_max_dfas += MAX_DFAS_INCREMENT;
@@ -380,7 +376,8 @@ void increase_max_dfas ()
* dfa starts out in state #1.
*/
-void ntod ()
+void
+ntod(void)
{
int*accset, ds, nacc, newds;
int sym, hashval, numstates, dsize;
@@ -822,8 +819,9 @@ void ntod ()
* On return, the dfa state number is in newds.
*/
-int snstods (sns, numstates, accset, nacc, hashval, newds_addr)
- int sns[], numstates, accset[], nacc, hashval, *newds_addr;
+int
+snstods(int sns[], int numstates, int accset[], int nacc, int hashval,
+ int *newds_addr)
{
int didsort = 0;
int i, j;
@@ -944,8 +942,8 @@ int snstods (sns, numstates, accset, nac
* int transsym, int nset[current_max_dfa_size] );
*/
-int symfollowset (ds, dsize, transsym, nset)
- int ds[], dsize, transsym, nset[];
+int
+symfollowset(int ds[], int dsize, int transsym, int nset[])
{
int ns, tsp, sym, i, j, lenccl, ch, numstates, ccllist;
@@ -1022,9 +1020,8 @@ int symfollowset (ds, dsize, transsym, n
* int symlist[numecs], int duplist[numecs] );
*/
-void sympartition (ds, numstates, symlist, duplist)
- int ds[], numstates;
- int symlist[], duplist[];
+void
+sympartition(int ds[], int numstates, int symlist[], int duplist[])
{
int tch, i, j, k, ns, dupfwd[CSIZE + 1], lenccl, cclp, ich;
Index: gen.c
===
Re: rip_usrreq() style tweak
On Thu, Feb 21, 2019 at 04:58:08PM +0100, Jeremie Courreges-Anglas wrote:
> On Thu, Feb 21 2019, Michael Mikonos wrote:
> > Hello,
> >
> > Is it OK to merge the PRU_CONNECT2 case into the generic
> > "not supported" case in {rip,rip6}_usrreq()?
> > If anything this allows the reader to see all the
> > unsupported requests in one place.
>
> ok with me, one tweak,
>
> > - Michael
> >
> >
> > Index: netinet/raw_ip.c
> > ===
> > RCS file: /cvs/src/sys/netinet/raw_ip.c,v
> > retrieving revision 1.119
> > diff -u -p -u -r1.119 raw_ip.c
> > --- netinet/raw_ip.c4 Feb 2019 21:40:52 - 1.119
> > +++ netinet/raw_ip.c21 Feb 2019 07:15:08 -
> > @@ -493,10 +493,6 @@ rip_usrreq(struct socket *so, int req, s
> > break;
> > }
> >
> > - case PRU_CONNECT2:
> > - error = EOPNOTSUPP;
> > - break;
> > -
> > /*
> > * Mark the connection as being incapable of further input.
> > */
> > @@ -549,6 +545,7 @@ rip_usrreq(struct socket *so, int req, s
> > /*
> > * Not supported.
> > */
> > + case PRU_CONNECT2:
> > case PRU_LISTEN:
> > case PRU_ACCEPT:
> > case PRU_SENDOOB:
> > Index: netinet6/raw_ip6.c
> > ===
> > RCS file: /cvs/src/sys/netinet6/raw_ip6.c,v
> > retrieving revision 1.134
> > diff -u -p -u -r1.134 raw_ip6.c
> > --- netinet6/raw_ip6.c 4 Feb 2019 21:40:52 - 1.134
> > +++ netinet6/raw_ip6.c 21 Feb 2019 07:15:09 -
> > @@ -618,10 +618,6 @@ rip6_usrreq(struct socket *so, int req,
> > break;
> > }
> >
> > - case PRU_CONNECT2:
> > - error = EOPNOTSUPP;
> > - break;
> > -
> > /*
> > * Mark the connection as being incapable of futther input.
>
> Can you please s/futther/further/ while here?
Fixed that.
>
> > */
> > @@ -672,6 +668,7 @@ rip6_usrreq(struct socket *so, int req,
> > /*
> > * Not supported.
> > */
> > + case PRU_CONNECT2:
> > case PRU_LISTEN:
> > case PRU_ACCEPT:
> > case PRU_SENDOOB:
> >
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
rip_usrreq() style tweak
Hello,
Is it OK to merge the PRU_CONNECT2 case into the generic
"not supported" case in {rip,rip6}_usrreq()?
If anything this allows the reader to see all the
unsupported requests in one place.
- Michael
Index: netinet/raw_ip.c
===
RCS file: /cvs/src/sys/netinet/raw_ip.c,v
retrieving revision 1.119
diff -u -p -u -r1.119 raw_ip.c
--- netinet/raw_ip.c4 Feb 2019 21:40:52 - 1.119
+++ netinet/raw_ip.c21 Feb 2019 07:15:08 -
@@ -493,10 +493,6 @@ rip_usrreq(struct socket *so, int req, s
break;
}
- case PRU_CONNECT2:
- error = EOPNOTSUPP;
- break;
-
/*
* Mark the connection as being incapable of further input.
*/
@@ -549,6 +545,7 @@ rip_usrreq(struct socket *so, int req, s
/*
* Not supported.
*/
+ case PRU_CONNECT2:
case PRU_LISTEN:
case PRU_ACCEPT:
case PRU_SENDOOB:
Index: netinet6/raw_ip6.c
===
RCS file: /cvs/src/sys/netinet6/raw_ip6.c,v
retrieving revision 1.134
diff -u -p -u -r1.134 raw_ip6.c
--- netinet6/raw_ip6.c 4 Feb 2019 21:40:52 - 1.134
+++ netinet6/raw_ip6.c 21 Feb 2019 07:15:09 -
@@ -618,10 +618,6 @@ rip6_usrreq(struct socket *so, int req,
break;
}
- case PRU_CONNECT2:
- error = EOPNOTSUPP;
- break;
-
/*
* Mark the connection as being incapable of futther input.
*/
@@ -672,6 +668,7 @@ rip6_usrreq(struct socket *so, int req,
/*
* Not supported.
*/
+ case PRU_CONNECT2:
case PRU_LISTEN:
case PRU_ACCEPT:
case PRU_SENDOOB:
installboot: explicit free() in bootstrap()
Hello, On hppa and landisk, bootstrap() is called from md_installboot(). md_installboot() is the last action before returning from main(), but bootstrap() can explicitly free the buffer it calloc'd (boot). I don't have access to hppa or landisk. Is someone able to check that this doesn't break anything? - Michael Index: bootstrap.c === RCS file: /cvs/src/usr.sbin/installboot/bootstrap.c,v retrieving revision 1.10 diff -u -p -u -r1.10 bootstrap.c --- bootstrap.c 1 Sep 2018 16:55:29 - 1.10 +++ bootstrap.c 7 Nov 2018 15:15:31 - @@ -124,7 +124,9 @@ bootstrap(int devfd, char *dev, char *bo fprintf(stderr, "%s bootstrap to disk\n", (nowrite ? "would write" : "writing")); if (nowrite) - return; + goto done; if (pwrite(devfd, boot, bootsize, 0) != (ssize_t)bootsize) err(1, "pwrite"); +done: + free(boot); }
Re: parse.y: strndup() in cmdline_symset()
ping?
On Thu, Nov 01, 2018 at 04:14:53PM +0800, Michael Mikonos wrote:
> Hello,
>
> When I updated cmdline_symset() in parse.y in the following commit
> I missed src/sbin/{iked,ipsecctl,pfctl}/parse.y. OK to update them?
>
> https://marc.info/?l=openbsd-cvs=153631079505256=2
>
>
> Index: iked/parse.y
> ===
> RCS file: /cvs/src/sbin/iked/parse.y,v
> retrieving revision 1.76
> diff -u -p -u -r1.76 parse.y
> --- iked/parse.y 1 Nov 2018 00:18:44 - 1.76
> +++ iked/parse.y 1 Nov 2018 07:03:31 -
> @@ -1660,17 +1660,13 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
>
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> err(1, "%s", __func__);
> -
> - strlcpy(sym, s, len);
> -
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: ipsecctl/parse.y
> ===
> RCS file: /cvs/src/sbin/ipsecctl/parse.y,v
> retrieving revision 1.174
> diff -u -p -u -r1.174 parse.y
> --- ipsecctl/parse.y 1 Nov 2018 00:18:44 - 1.174
> +++ ipsecctl/parse.y 1 Nov 2018 07:03:32 -
> @@ -1426,17 +1426,13 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
>
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> err(1, "%s", __func__);
> -
> - strlcpy(sym, s, len);
> -
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: pfctl/parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.685
> diff -u -p -u -r1.685 parse.y
> --- pfctl/parse.y 1 Nov 2018 00:18:44 - 1.685
> +++ pfctl/parse.y 1 Nov 2018 07:03:34 -
> @@ -5564,11 +5564,9 @@ pfctl_cmdline_symset(char *s)
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
>
> - if ((sym = malloc(strlen(s) - strlen(val) + 1)) == NULL)
> + sym = strndup(s, val - s);
> + if (sym == NULL);
> err(1, "%s", __func__);
> -
> - strlcpy(sym, s, strlen(s) - strlen(val) + 1);
> -
> ret = symset(sym, val + 1, 1);
> free(sym);
>
Re: installboot: double free
On Tue, Nov 06, 2018 at 10:20:34AM +0100, Otto Moerbeek wrote:
> On Tue, Nov 06, 2018 at 04:35:05PM +0800, Michael Mikonos wrote:
>
> > Hello,
> >
> > In installboot's fileprefix() function r is the return value
> > of realpath(). If snprintf() fails free(r) happens twice---
> > the second time is at label "err". From what I see the behavior
> > was introduced in util.c revision 1.12.
> > Does this fix look OK?
>
> Yes, but I perfer to move the free call to just before the no-error return.
>
Sure, updated diff.
Index: util.c
===
RCS file: /cvs/src/usr.sbin/installboot/util.c,v
retrieving revision 1.12
diff -u -p -r1.12 util.c
--- util.c 3 Jul 2018 20:14:41 - 1.12
+++ util.c 6 Nov 2018 09:29:04 -
@@ -124,11 +124,11 @@ fileprefix(const char *base, const char
goto err;
}
n = snprintf(s, PATH_MAX, "%s/%s", r, b);
- free(r);
if (n < 1 || n >= PATH_MAX) {
warn("snprintf");
goto err;
}
+ free(r);
return (s);
err:
installboot: double free
Hello,
In installboot's fileprefix() function r is the return value
of realpath(). If snprintf() fails free(r) happens twice---
the second time is at label "err". From what I see the behavior
was introduced in util.c revision 1.12.
Does this fix look OK?
- Michael
Index: util.c
===
RCS file: /cvs/src/usr.sbin/installboot/util.c,v
retrieving revision 1.12
diff -u -p -r1.12 util.c
--- util.c 3 Jul 2018 20:14:41 - 1.12
+++ util.c 6 Nov 2018 08:26:45 -
@@ -125,6 +125,7 @@ fileprefix(const char *base, const char
}
n = snprintf(s, PATH_MAX, "%s/%s", r, b);
free(r);
+ r = NULL;
if (n < 1 || n >= PATH_MAX) {
warn("snprintf");
goto err;
parse.y: strndup() in cmdline_symset()
Hello,
When I updated cmdline_symset() in parse.y in the following commit
I missed src/sbin/{iked,ipsecctl,pfctl}/parse.y. OK to update them?
https://marc.info/?l=openbsd-cvs=153631079505256=2
Index: iked/parse.y
===
RCS file: /cvs/src/sbin/iked/parse.y,v
retrieving revision 1.76
diff -u -p -u -r1.76 parse.y
--- iked/parse.y1 Nov 2018 00:18:44 - 1.76
+++ iked/parse.y1 Nov 2018 07:03:31 -
@@ -1660,17 +1660,13 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
+ sym = strndup(s, val - s);
+ if (sym == NULL)
err(1, "%s", __func__);
-
- strlcpy(sym, s, len);
-
ret = symset(sym, val + 1, 1);
free(sym);
Index: ipsecctl/parse.y
===
RCS file: /cvs/src/sbin/ipsecctl/parse.y,v
retrieving revision 1.174
diff -u -p -u -r1.174 parse.y
--- ipsecctl/parse.y1 Nov 2018 00:18:44 - 1.174
+++ ipsecctl/parse.y1 Nov 2018 07:03:32 -
@@ -1426,17 +1426,13 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
+ sym = strndup(s, val - s);
+ if (sym == NULL)
err(1, "%s", __func__);
-
- strlcpy(sym, s, len);
-
ret = symset(sym, val + 1, 1);
free(sym);
Index: pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.685
diff -u -p -u -r1.685 parse.y
--- pfctl/parse.y 1 Nov 2018 00:18:44 - 1.685
+++ pfctl/parse.y 1 Nov 2018 07:03:34 -
@@ -5564,11 +5564,9 @@ pfctl_cmdline_symset(char *s)
if ((val = strrchr(s, '=')) == NULL)
return (-1);
- if ((sym = malloc(strlen(s) - strlen(val) + 1)) == NULL)
+ sym = strndup(s, val - s);
+ if (sym == NULL);
err(1, "%s", __func__);
-
- strlcpy(sym, s, strlen(s) - strlen(val) + 1);
-
ret = symset(sym, val + 1, 1);
free(sym);
Re: Qcow2: Clean up logging/error handling
On Tue, Oct 23, 2018 at 09:44:24PM -0700, Ori Bernstein wrote:
> This patch turns most warnings into errors, and uses the
> appropriate fatal/fatalx so that we don't print bogus error
> strings. It also adds checks for unsupported refcount sizes
> and writes that clobber the header.
>
> Ok?
Hello, two comments below.
>
> diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
> index 3a215599d49..e550f3b84b5 100644
> --- usr.sbin/vmd/vioqcow2.c
> +++ usr.sbin/vmd/vioqcow2.c
> @@ -137,6 +137,12 @@ virtio_init_qcow2(struct virtio_backing *file, off_t
> *szp, int *fd, size_t nfd)
> return 0;
> }
>
> +/*
> + * Return the path to the base image given a disk image.
> + *
> + * This is used when resolving base images from vmd, so it should avoid
> + * fatalx'ing, or we will bring down multiple vms on a corrupt disk.
> + */
> ssize_t
> virtio_qcow2_get_base(int fd, char *path, size_t npath, const char *dpath)
> {
> @@ -151,7 +157,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
> const char *dpath)
> return -1;
> }
> if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) {
> - log_warn("%s: invalid magic numbers", __func__);
> + log_warnx("%s: invalid magic numbers", __func__);
> return -1;
> }
> backingoff = be64toh(header.backingoff);
> @@ -160,7 +166,7 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
> const char *dpath)
> return 0;
>
> if (backingsz >= npath - 1) {
> - log_warn("%s: snapshot path too long", __func__);
> + log_warnx("%s: snapshot path too long", __func__);
> return -1;
> }
> if (pread(fd, path, backingsz, backingoff) != backingsz) {
> @@ -178,20 +184,19 @@ virtio_qcow2_get_base(int fd, char *path, size_t npath,
> const char *dpath)
> if (path[0] == '/') {
> if (realpath(path, expanded) == NULL ||
> strlcpy(path, expanded, npath) >= npath) {
> - log_warn("unable to resolve %s", path);
> + log_warnx("unable to resolve %s", path);
> return -1;
> }
> } else {
> s = dirname(dpath);
> if (snprintf(expanded, sizeof(expanded),
> "%s/%s", s, path) >= (int)sizeof(expanded)) {
> - log_warn("path too long: %s/%s",
> - s, path);
> + log_warnx("path too long: %s/%s", s, path);
> return -1;
> }
> if (npath < PATH_MAX ||
> realpath(expanded, path) == NULL) {
> - log_warn("unable to resolve %s", path);
> + log_warnx("unable to resolve %s", path);
> return -1;
> }
> }
> @@ -216,15 +221,10 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd)
> disk->base = NULL;
> disk->l1 = NULL;
>
> - if (pread(fd, , sizeof(header), 0) != sizeof(header)) {
> - log_warn("%s: short read on header", __func__);
> - goto error;
> - }
> - if (strncmp(header.magic,
> - VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0) {
> - log_warn("%s: invalid magic numbers", __func__);
> - goto error;
> - }
> + if (pread(fd, , sizeof(header), 0) != sizeof(header))
> + fatalx("%s: short read on header", __func__);
> + if (strncmp(header.magic, VM_MAGIC_QCOW, strlen(VM_MAGIC_QCOW)) != 0)
> + fatalx("%s: invalid magic numbers", __func__);
>
> disk->clustersz = (1ull << be32toh(header.clustershift));
> disk->disksz= be64toh(header.disksz);
> @@ -249,79 +249,59 @@ qc2_open(struct qcdisk *disk, int *fds, size_t nfd)
> /*
>* We only know about the dirty or corrupt bits here.
>*/
> - if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) {
> - log_warnx("%s: unsupported features %llx", __func__,
> + if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT))
> + fatalx("%s: unsupported features %llx", __func__,
> disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT));
> - goto error;
> - }
> + if (be32toh(header.reforder) != 4)
> + fatalx("%s: unsupported refcount size\n", __func__);
>
> disk->l1 = calloc(disk->l1sz, sizeof(*disk->l1));
> if (!disk->l1)
> - goto error;
> + fatalx("%s: could not allocate l1 table", __func__);
> if (pread(disk->fd, disk->l1, 8 * disk->l1sz, disk->l1off)
> - != 8 * disk->l1sz) {
> - log_warn("%s: unable to read qcow2 L1 table", __func__);
> - goto error;
> - }
> + != 8 * disk->l1sz)
> + fatalx("%s: unable to read qcow2 L1 table", __func__);
> for (i = 0; i < disk->l1sz; i++)
>
smtpd.conf manpage typo
OK? Index: smtpd.conf.5 === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v retrieving revision 1.205 diff -u -p -u -r1.205 smtpd.conf.5 --- smtpd.conf.524 Sep 2018 16:14:34 - 1.205 +++ smtpd.conf.58 Oct 2018 05:38:55 - @@ -336,7 +336,7 @@ for incoming connections, using the same The .Ar interface parameter may also be an interface group, an IP address, or a domain name. -Listening can optionally be resticted to a specific address +Listening can optionally be restricted to a specific address .Ar family , which can be either .Cm inet4
Re: yacc + unveil
Hello,
Forwarding a newer patch that I came up with. This time unveil()s are
done before pledge() so no subsequent pledge() is needed to remove
the unveil promise.
* temporary files are created & unlinked in /tmp, so unveil the directory
* output_file_name is either -o PATH or the default of CWD/y.tab.c;
open it for write
* code_file_name is CWD/y.code.c if -r option is set, otherwise
code_file_name==output_file_name; add separate unveil for -r case
* input_file_name is the input path, or empty string for filename of "-";
open it for read if not an empty string
* verbose_file_name is CWD/y.output if -v option is set, otherwise NULL;
open it for write if not NULL
* defines_file_name is a path in the same directory as output_file_name
if option -d is set, otherwise NULL; open it for write if not NULL
getargs() and create_file_names() are responsible for setting the
global variables for the various paths based on argv, so move them to
the top. create_file_names() is being lifted from open_files() which
actually performs fopen().
Index: main.c
===
RCS file: /cvs/src/usr.bin/yacc/main.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 main.c
--- main.c 25 May 2017 20:11:03 - 1.29
+++ main.c 1 Oct 2018 07:11:47 -
@@ -34,6 +34,7 @@
*/
#include
+#include
#include
#include
#include
@@ -302,8 +303,6 @@ open_files(void)
{
int fd;
- create_file_names();
-
if (input_file == 0) {
input_file = fopen(input_file_name, "r");
if (input_file == 0)
@@ -346,11 +345,34 @@ open_files(void)
int
main(int argc, char *argv[])
{
+ getargs(argc, argv);
+ create_file_names();
+
+ if (unveil(_PATH_TMP, "wrc") == -1)
+ err(1, "unveil");
+ if (unveil(output_file_name, "wc") == -1)
+ err(1, "unveil");
+ if (code_file_name != output_file_name) {
+ if (unveil(code_file_name, "wc") == -1)
+ err(1, "unveil");
+ }
+ if (verbose_file_name != NULL) {
+ if (unveil(verbose_file_name, "wc") == -1)
+ err(1, "unveil");
+ }
+ if (defines_file_name != NULL) {
+ if (unveil(defines_file_name, "wc") == -1)
+ err(1, "unveil");
+ }
+ if (strlen(input_file_name) > 0) {
+ if (unveil(input_file_name, "r") == -1)
+ err(1, "unveil");
+ }
+
if (pledge("stdio rpath wpath cpath", NULL) == -1)
fatal("pledge: invalid arguments");
set_signals();
- getargs(argc, argv);
open_files();
reader();
lr0();
Re: csh: memory leak in setDolp()
BTW the leak happens when a pattern in variable modifier s///
is not found.
$ set a="test"
$ echo $a:s/badpattern//
test
Any objections if I commit this?
On Thu, Sep 20, 2018 at 12:30:05PM +0800, Michael Mikonos wrote:
> Hello,
>
> In setDolp() pointers cp and dp initially point to the same
> copied string, but later dp can become NULL if Strstr() finds
> no match. The copied string is not freed in this case.
> NetBSD added this fix in their dol.c revision 1.23 (2006).
> OK?
>
> - Michael
>
>
> Index: dol.c
> ===
> RCS file: /cvs/src/bin/csh/dol.c,v
> retrieving revision 1.24
> diff -u -p -u -r1.24 dol.c
> --- dol.c 18 Sep 2018 06:56:09 - 1.24
> +++ dol.c 20 Sep 2018 04:14:37 -
> @@ -766,8 +766,10 @@ setDolp(Char *cp)
> addla(dp);
> free(dp);
> }
> -else
> +else {
> addla(cp);
> + free(cp);
> +}
>
> dolp = STRNULL;
> if (seterr)
unveil DPRINTF()
Hello,
As done in other parts of the kernel, introduce DPRINTF() macro
to unveil. I think this is worth doing because the code is slightly
more readable. OK?
- Michael
Index: kern_unveil.c
===
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.15
diff -u -p -u -r1.15 kern_unveil.c
--- kern_unveil.c 25 Sep 2018 19:24:17 - 1.15
+++ kern_unveil.c 26 Sep 2018 15:02:51 -
@@ -36,6 +36,11 @@
#include
/* #define DEBUG_UNVEIL */
+#ifdef DEBUG_UNVEIL
+#define DPRINTF(x...) printf(x)
+#else
+#define DPRINTF(x...)
+#endif
#define UNVEIL_MAX_VNODES 128
#define UNVEIL_MAX_NAMES 128
@@ -111,9 +116,7 @@ unveil_delete_names(struct unveil *uv)
ret++;
}
rw_exit_write(>uv_lock);
-#ifdef DEBUG_UNVEIL
- printf("deleted %d names\n", ret);
-#endif
+ DPRINTF("deleted %d names\n", ret);
return ret;
}
@@ -126,9 +129,7 @@ unveil_add_name(struct unveil *uv, char
unvn = unvname_new(name, strlen(name) + 1, flags);
RBT_INSERT(unvname_rbt, >uv_names, unvn);
rw_exit_write(>uv_lock);
-#ifdef DEBUG_UNVEIL
- printf("added name %s underneath vnode %p\n", name, uv->uv_vp);
-#endif
+ DPRINTF("added name %s underneath vnode %p\n", name, uv->uv_vp);
}
struct unvname *
@@ -138,10 +139,8 @@ unveil_namelookup(struct unveil *uv, cha
rw_enter_read(>uv_lock);
-#ifdef DEBUG_UNVEIL
- printf("unveil_namelookup: looking up name %s (%p) in vnode %p\n",
+ DPRINTF("unveil_namelookup: looking up name %s (%p) in vnode %p\n",
name, name, uv->uv_vp);
-#endif
KASSERT(uv->uv_vp != NULL);
@@ -175,11 +174,9 @@ unveil_destroy(struct process *ps)
/* skip any vnodes zapped by unveil_removevnode */
if (vp != NULL) {
vp->v_uvcount--;
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): removing vnode %p uvcount %d "
+ DPRINTF("unveil: %s(%d): removing vnode %p uvcount %d "
"in position %ld\n",
ps->ps_comm, ps->ps_pid, vp, vp->v_uvcount, i);
-#endif
vrele(vp);
}
ps->ps_uvncount -= unveil_delete_names(uv);
@@ -270,10 +267,8 @@ unveil_lookup(struct vnode *vp, struct p
/* clear the cwd unveil when we .. past it */
if (pr->ps_uvpcwd && (vp == pr->ps_uvpcwd->uv_vp)) {
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): nuking cwd traversing vnode %p\n",
+ DPRINTF("unveil: %s(%d): nuking cwd traversing vnode %p\n",
p->p_p->ps_comm, p->p_p->ps_pid, vp);
-#endif
p->p_p->ps_uvpcwd = NULL;
p->p_p->ps_uvpcwdgone = 0;
}
@@ -292,10 +287,8 @@ unveil_lookup(struct vnode *vp, struct p
r = pr->ps_uvvcount - 1;
while (l <= r) {
size_t m = l + (r - l)/2;
-#ifdef DEBUG_UNVEIL
- printf("unveil: checking vnode %p vs. unveil vnode %p\n",
+ DPRINTF("unveil: checking vnode %p vs. unveil vnode %p\n",
vp, uv[m].uv_vp);
-#endif
if (vp == uv[m].uv_vp) {
KASSERT(uv[m].uv_vp->v_uvcount > 0);
KASSERT(uv[m].uv_vp->v_usecount > 0);
@@ -342,9 +335,7 @@ unveil_setflags(u_char *flags, u_char nf
{
#if 0
if (((~(*flags)) & nflags) != 0) {
-#ifdef DEBUG_UNVEIL
- printf("Flags escalation %llX -> %llX\n", *flags, nflags);
-#endif
+ DPRINTF("Flags escalation %llX -> %llX\n", *flags, nflags);
return 1;
}
#endif
@@ -451,11 +442,9 @@ unveil_add(struct proc *p, struct nameid
* unrestrict it.
*/
if (directory_add) {
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): updating directory vnode %p"
+ DPRINTF("unveil: %s(%d): updating directory vnode %p"
" to unrestricted uvcount %d\n",
pr->ps_comm, pr->ps_pid, vp, vp->v_uvcount);
-#endif
if (!unveil_setflags(>uv_flags, flags))
ret = EPERM;
else
@@ -471,12 +460,10 @@ unveil_add(struct proc *p, struct nameid
struct unvname *tname;
if ((tname = unveil_namelookup(uv,
ndp->ni_cnd.cn_nameptr)) != NULL) {
-#ifdef DEBUG_UNVEIL
- printf("unveil: %s(%d): changing flags for %s"
+ DPRINTF("unveil: %s(%d): changing flags for %s"
"in vnode %p, uvcount %d\n",
pr->ps_comm, pr->ps_pid, tname->un_name, vp,
vp->v_uvcount);
-#endif
unveil manpage tweak 2
Re-reading the unveil manual I found a typo which isn't flagged by a spell checker. Does anyone prefer just doing s/paths// though? Index: unveil.2 === RCS file: /cvs/src/lib/libc/sys/unveil.2,v retrieving revision 1.10 diff -u -p -u -r1.10 unveil.2 --- unveil.226 Sep 2018 02:54:34 - 1.10 +++ unveil.226 Sep 2018 05:44:09 - @@ -108,7 +108,7 @@ This means that a directory that is remo .Fn unveil will appear to not exist. .Pp -Non-directories paths are remembered by name within their containing +Non-directory paths are remembered by name within their containing directory, and so may be created, removed, or re-created after a call to .Fn unveil and still appear to exist.
Re: yacc + unveil
On Tue, Sep 25, 2018 at 11:42:26PM +0800, Michael Mikonos wrote:
> On Tue, Sep 25, 2018 at 05:25:54PM +0200, Sebastien Marie wrote:
> > On Tue, Sep 25, 2018 at 11:15:43PM +0800, Michael Mikonos wrote:
> > > On Tue, Sep 25, 2018 at 03:22:38PM +0100, Ricardo Mestre wrote:
> > > > This is an example of better to start at just hoisting the code that
> > > > opens the many fds and put them all inside open_files(). After that it's
> > > > just a matter of calling pledge("stdio") and we are done.
> > > >
> > > > Of course that after this is done we can still make a list of all the
> > > > files
> > > > we need to open and unveil them, but not the way it's done here.
> > > >
> > > > Once I get back home from $DAYJOB I'll try to have a look at this.
> > >
> > > After open_files() the wpath pledge can be dropped. rpath is still
> > > needed because /tmp files are reopened for read in output(). cpath
> > > is needed because /tmp files are unlinked at the end. This patch
> > > adds a pledge call, but is it better to just move the first pledge()
> > > down?
> > >
> >
> > you could try with the "tmppath" promise. I will allow opening/creating
> > files on /tmp and unlinking them (but not sure it will cover all yacc
> > need as it is designed for mkstemp(3) family). Unveil for such
> > operations are fine too, without explicit unveil(2) call.
> >
>
> Ah, I see what you mean. pledging "tmppath" is kind of like unveil
> because the allowed operations only work under /tmp.
> It's possible to do this after calling open_files() because the only
> files (re)opened later are in /tmp.
My description was wrong. tmppath allows unlink of /tmp files at the
end. rpath is still needed to reopen the /tmp files.
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/yacc/main.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 main.c
> --- main.c25 May 2017 20:11:03 - 1.29
> +++ main.c25 Sep 2018 15:38:18 -
> @@ -346,12 +346,16 @@ open_files(void)
> int
> main(int argc, char *argv[])
> {
> - if (pledge("stdio rpath wpath cpath", NULL) == -1)
> + if (pledge("stdio rpath wpath cpath tmppath", NULL) == -1)
> fatal("pledge: invalid arguments");
>
> set_signals();
> getargs(argc, argv);
> open_files();
> +
> + if (pledge("stdio rpath tmppath", NULL) == -1)
> + fatal("pledge: invalid arguments");
> +
> reader();
> lr0();
> lalr();
Re: yacc + unveil
On Tue, Sep 25, 2018 at 05:25:54PM +0200, Sebastien Marie wrote:
> On Tue, Sep 25, 2018 at 11:15:43PM +0800, Michael Mikonos wrote:
> > On Tue, Sep 25, 2018 at 03:22:38PM +0100, Ricardo Mestre wrote:
> > > This is an example of better to start at just hoisting the code that
> > > opens the many fds and put them all inside open_files(). After that it's
> > > just a matter of calling pledge("stdio") and we are done.
> > >
> > > Of course that after this is done we can still make a list of all the
> > > files
> > > we need to open and unveil them, but not the way it's done here.
> > >
> > > Once I get back home from $DAYJOB I'll try to have a look at this.
> >
> > After open_files() the wpath pledge can be dropped. rpath is still
> > needed because /tmp files are reopened for read in output(). cpath
> > is needed because /tmp files are unlinked at the end. This patch
> > adds a pledge call, but is it better to just move the first pledge()
> > down?
> >
>
> you could try with the "tmppath" promise. I will allow opening/creating
> files on /tmp and unlinking them (but not sure it will cover all yacc
> need as it is designed for mkstemp(3) family). Unveil for such
> operations are fine too, without explicit unveil(2) call.
>
Ah, I see what you mean. pledging "tmppath" is kind of like unveil
because the allowed operations only work under /tmp.
It's possible to do this after calling open_files() because the only
files (re)opened later are in /tmp.
Index: main.c
===
RCS file: /cvs/src/usr.bin/yacc/main.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 main.c
--- main.c 25 May 2017 20:11:03 - 1.29
+++ main.c 25 Sep 2018 15:38:18 -
@@ -346,12 +346,16 @@ open_files(void)
int
main(int argc, char *argv[])
{
- if (pledge("stdio rpath wpath cpath", NULL) == -1)
+ if (pledge("stdio rpath wpath cpath tmppath", NULL) == -1)
fatal("pledge: invalid arguments");
set_signals();
getargs(argc, argv);
open_files();
+
+ if (pledge("stdio rpath tmppath", NULL) == -1)
+ fatal("pledge: invalid arguments");
+
reader();
lr0();
lalr();
Re: yacc + unveil
On Tue, Sep 25, 2018 at 03:22:38PM +0100, Ricardo Mestre wrote:
> This is an example of better to start at just hoisting the code that
> opens the many fds and put them all inside open_files(). After that it's
> just a matter of calling pledge("stdio") and we are done.
>
> Of course that after this is done we can still make a list of all the files
> we need to open and unveil them, but not the way it's done here.
>
> Once I get back home from $DAYJOB I'll try to have a look at this.
After open_files() the wpath pledge can be dropped. rpath is still
needed because /tmp files are reopened for read in output(). cpath
is needed because /tmp files are unlinked at the end. This patch
adds a pledge call, but is it better to just move the first pledge()
down?
Index: main.c
===
RCS file: /cvs/src/usr.bin/yacc/main.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 main.c
--- main.c 25 May 2017 20:11:03 - 1.29
+++ main.c 25 Sep 2018 15:07:35 -
@@ -352,6 +352,10 @@ main(int argc, char *argv[])
set_signals();
getargs(argc, argv);
open_files();
+
+ if (pledge("stdio rpath cpath", NULL) == -1)
+ fatal("pledge: invalid arguments");
+
reader();
lr0();
lalr();
unveil manpage tweak
This patch aligns unveil.2 with pledge.2, so the unveil manual will explicitly mention errno is set. OK? Index: unveil.2 === RCS file: /cvs/src/lib/libc/sys/unveil.2,v retrieving revision 1.9 diff -u -p -u -r1.9 unveil.2 --- unveil.230 Jul 2018 15:21:36 - 1.9 +++ unveil.225 Sep 2018 06:25:35 - @@ -134,8 +134,7 @@ of the interfaces called. In most cases it is best practice to unveil the directories in which an application makes use of files. .Sh RETURN VALUES -.Fn unveil -returns 0 on success or -1 on failure. +.Rv -std .Sh ERRORS .Bl -tag -width Er .It E2BIG
Re: yacc + unveil
On Mon, Sep 24, 2018 at 10:53:47PM -0600, Theo de Raadt wrote:
> Ugh. A diff which doens't check error returns. Averting my gaze
> is similar to "no way". Hope you have another quarter, because you
> need to try again
Oops... new coin inserted. I decided to create a fatal_perror()
function which behaves like perror(). yacc's error.c didn't seem
to have anything like that. Now if either pledge() or unveil()
fails we see the appropriate error string instead of always seeing
"invalid arguments" for pledge().
Index: defs.h
===
RCS file: /cvs/src/usr.bin/yacc/defs.h,v
retrieving revision 1.18
diff -u -p -u -r1.18 defs.h
--- defs.h 2 Dec 2014 15:56:22 - 1.18
+++ defs.h 25 Sep 2018 05:34:27 -
@@ -307,6 +307,7 @@ extern void closure(short *, int);
extern void finalize_closure(void);
extern __dead void fatal(char *);
+extern __dead void fatal_perror(char *);
extern void reflexive_transitive_closure(unsigned *, int);
extern __dead void done(int);
Index: error.c
===
RCS file: /cvs/src/usr.bin/yacc/error.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 error.c
--- error.c 8 Mar 2014 01:05:39 - 1.14
+++ error.c 25 Sep 2018 05:34:27 -
@@ -35,6 +35,8 @@
/* routines for printing error messages */
+#include
+
#include "defs.h"
@@ -45,6 +47,12 @@ fatal(char *msg)
done(2);
}
+void
+fatal_perror(char *msg)
+{
+ fprintf(stderr, "%s: %s\n", msg, strerror(errno));
+ done(2);
+}
void
no_space(void)
Index: main.c
===
RCS file: /cvs/src/usr.bin/yacc/main.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 main.c
--- main.c 25 May 2017 20:11:03 - 1.29
+++ main.c 25 Sep 2018 05:34:27 -
@@ -305,10 +305,14 @@ open_files(void)
create_file_names();
if (input_file == 0) {
+ if (unveil(input_file_name, "r") == -1)
+ fatal_perror("unveil");
input_file = fopen(input_file_name, "r");
if (input_file == 0)
open_error(input_file_name);
}
+ if (unveil("/tmp", "crw") == -1)
+ fatal_perror("unveil");
fd = mkstemp(action_file_name);
if (fd == -1 || (action_file = fdopen(fd, "w")) == NULL)
open_error(action_file_name);
@@ -318,11 +322,15 @@ open_files(void)
open_error(text_file_name);
if (vflag) {
+ if (unveil(verbose_file_name, "cw") == -1)
+ fatal_perror("unveil");
verbose_file = fopen(verbose_file_name, "w");
if (verbose_file == 0)
open_error(verbose_file_name);
}
if (dflag) {
+ if (unveil(defines_file_name, "cw") == -1)
+ fatal_perror("unveil");
defines_file = fopen(defines_file_name, "w");
if (defines_file == NULL)
open_write_error(defines_file_name);
@@ -330,24 +338,30 @@ open_files(void)
if (fd == -1 || (union_file = fdopen(fd, "w")) == NULL)
open_error(union_file_name);
}
+ if (unveil(output_file_name, "cw") == -1)
+ fatal_perror("unveil");
output_file = fopen(output_file_name, "w");
if (output_file == 0)
open_error(output_file_name);
if (rflag) {
+ if (unveil(code_file_name, "cw") == -1)
+ fatal_perror("unveil");
code_file = fopen(code_file_name, "w");
if (code_file == 0)
open_error(code_file_name);
} else
code_file = output_file;
+ if (unveil(NULL, NULL) == -1)
+ fatal_perror("unveil");
}
int
main(int argc, char *argv[])
{
- if (pledge("stdio rpath wpath cpath", NULL) == -1)
- fatal("pledge: invalid arguments");
+ if (pledge("stdio rpath wpath cpath unveil", NULL) == -1)
+ fatal_perror("pledge");
set_signals();
getargs(argc, argv);
yacc + unveil
Hello,
I haven't tried using unveil() before but yacc cleanly annotates
all the files it needs in open_files(). The options -d -r -v each
cause an extra file to be written. unveil() is only needed for
the input file if not reading from stdin. Temporary files are
always under /tmp because TMPDIR environment variable was previously
removed. OK, or any suggestions?
- Michael
Index: main.c
===
RCS file: /cvs/src/usr.bin/yacc/main.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 main.c
--- main.c 25 May 2017 20:11:03 - 1.29
+++ main.c 25 Sep 2018 03:43:23 -
@@ -305,10 +305,12 @@ open_files(void)
create_file_names();
if (input_file == 0) {
+ unveil(input_file_name, "r");
input_file = fopen(input_file_name, "r");
if (input_file == 0)
open_error(input_file_name);
}
+ unveil("/tmp", "crw");
fd = mkstemp(action_file_name);
if (fd == -1 || (action_file = fdopen(fd, "w")) == NULL)
open_error(action_file_name);
@@ -318,11 +320,13 @@ open_files(void)
open_error(text_file_name);
if (vflag) {
+ unveil(verbose_file_name, "cw");
verbose_file = fopen(verbose_file_name, "w");
if (verbose_file == 0)
open_error(verbose_file_name);
}
if (dflag) {
+ unveil(defines_file_name, "cw");
defines_file = fopen(defines_file_name, "w");
if (defines_file == NULL)
open_write_error(defines_file_name);
@@ -330,23 +334,26 @@ open_files(void)
if (fd == -1 || (union_file = fdopen(fd, "w")) == NULL)
open_error(union_file_name);
}
+ unveil(output_file_name, "cw");
output_file = fopen(output_file_name, "w");
if (output_file == 0)
open_error(output_file_name);
if (rflag) {
+ unveil(code_file_name, "cw");
code_file = fopen(code_file_name, "w");
if (code_file == 0)
open_error(code_file_name);
} else
code_file = output_file;
+ unveil(NULL, NULL);
}
int
main(int argc, char *argv[])
{
- if (pledge("stdio rpath wpath cpath", NULL) == -1)
+ if (pledge("stdio rpath wpath cpath unveil", NULL) == -1)
fatal("pledge: invalid arguments");
set_signals();
csh: quoted string treated as a glob in switch
Hello,
As an exercise I tried writing a csh script and I found strange
behaviour in its parsing of switch statements.
The following snippet causes a syntax error (switch: Missing ].):
set c="["
switch ("$c")
case "[":
echo match
breaksw
endsw
The call tree is main() -> process() -> execute() -> execute() ->
func() -> doswitch() -> search() -> Gmatch() -> pmatch().
Gmatch() is performing a glob match and pmatch() is looking for
matching []s in a glob. In csh a switch case can be a glob
pattern, but the "case" argument is treated as a glob even if
it is quoted. This is at least inconsistent with other glob
usages in csh, where quoted strings are *not* treated as globs.
works as a glob:
ls [ab]*.c
does not work as a glob:
ls "[ab]*.c"
The patch below removes a call to strip(), which was causing
the string (Char *) returned by Dfix1() to lose its QUOTE flag.
When Gmatch()/pmatch() sees the QUOTE flag it knows to not treat
the case argument as a glob.
Thanks to Prashant Satish for helping to debug this.
OK?
- Michael
Index: func.c
===
RCS file: /cvs/src/bin/csh/func.c,v
retrieving revision 1.38
diff -u -p -u -r1.38 func.c
--- func.c 8 Sep 2018 01:28:39 - 1.38
+++ func.c 23 Sep 2018 14:51:25 -
@@ -657,7 +657,7 @@ search(int type, int level, Char *goal)
(void) getword(aword);
if (lastchr(aword) == ':')
aword[Strlen(aword) - 1] = 0;
- cp = strip(Dfix1(aword));
+ cp = Dfix1(aword);
if (Gmatch(goal, cp))
level = -1;
free(cp);
csh: memory leak in setDolp()
Hello,
In setDolp() pointers cp and dp initially point to the same
copied string, but later dp can become NULL if Strstr() finds
no match. The copied string is not freed in this case.
NetBSD added this fix in their dol.c revision 1.23 (2006).
OK?
- Michael
Index: dol.c
===
RCS file: /cvs/src/bin/csh/dol.c,v
retrieving revision 1.24
diff -u -p -u -r1.24 dol.c
--- dol.c 18 Sep 2018 06:56:09 - 1.24
+++ dol.c 20 Sep 2018 04:14:37 -
@@ -766,8 +766,10 @@ setDolp(Char *cp)
addla(dp);
free(dp);
}
-else
+else {
addla(cp);
+ free(cp);
+}
dolp = STRNULL;
if (seterr)
Re: csh: avoid using uninitialized stat buffer
On Wed, Sep 19, 2018 at 09:19:22AM -0600, Todd C. Miller wrote:
> When getcwd() fails, the stat buffer 'swd' is used uninitialized
> by the else clause. Since it is used in both clauses we should
> perform the stat before the if().
>
> However, fixing this causes 'cp' to be unitialized in some case so
> initialize cp to NULL and move the "cp == NULL" check out of the
> first if() clause now that it can be true in either case.
>
> Found by clang analyzer.
>
> - todd
I had to stare at this one for a while, but overall I like the patch.
OK miko@.
>
> Index: bin/csh/dir.c
> ===
> RCS file: /cvs/src/bin/csh/dir.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 dir.c
> --- bin/csh/dir.c 26 Dec 2015 13:48:38 - 1.21
> +++ bin/csh/dir.c 19 Sep 2018 15:14:43 -
> @@ -64,7 +64,7 @@ void
> dinit(Char *hp)
> {
> char *tcp;
> -Char *cp;
> +Char *cp = NULL;
> struct directory *dp;
> charpath[PATH_MAX];
> static const char emsg[] = "csh: Trying to start from \"%s\"\n";
> @@ -75,45 +75,45 @@ dinit(Char *hp)
> (void) fprintf(csherr, "csh: %s\n", strerror(errno));
> if (hp && *hp) {
> tcp = short2str(hp);
> - if (chdir(tcp) == -1)
> - cp = NULL;
> - else
> + if (chdir(tcp) == 0)
> cp = hp;
> (void) fprintf(csherr, emsg, vis_str(hp));
> }
> - else
> - cp = NULL;
> - if (cp == NULL) {
> - (void) fprintf(csherr, emsg, "/");
> - if (chdir("/") == -1)
> - /* I am not even try to print an error message! */
> - xexit(1);
> - cp = SAVE("/");
> - }
> }
> else {
> struct stat swd, shp;
>
> - /*
> - * See if $HOME is the working directory we got and use that
> - */
> - if (hp && *hp &&
> - stat(tcp, ) != -1 && stat(short2str(hp), ) != -1 &&
> - swd.st_dev == shp.st_dev && swd.st_ino == shp.st_ino)
> - cp = hp;
> - else {
> - char *cwd;
> -
> + if (stat(tcp, ) == -1) {
> + (void) fprintf(csherr, "csh: %s: %s\n", tcp, strerror(errno));
> + } else {
> /*
> - * use PWD if we have it (for subshells)
> + * See if $HOME is the working directory we got and use that
>*/
> - if ((cwd = getenv("PWD")) != NULL) {
> - if (stat(cwd, ) != -1 && swd.st_dev == shp.st_dev &&
> - swd.st_ino == shp.st_ino)
> - tcp = cwd;
> + if (hp && *hp && stat(short2str(hp), ) != -1 &&
> + swd.st_dev == shp.st_dev && swd.st_ino == shp.st_ino)
> + cp = hp;
> + else {
> + char *cwd;
> +
> + /*
> + * use PWD if we have it (for subshells)
> + */
> + if ((cwd = getenv("PWD")) != NULL) {
> + if (stat(cwd, ) != -1 && swd.st_dev == shp.st_dev &&
> + swd.st_ino == shp.st_ino)
> + tcp = cwd;
> + }
> + cp = dcanon(SAVE(tcp), STRNULL);
> }
> - cp = dcanon(SAVE(tcp), STRNULL);
> }
> +}
> +
> +if (cp == NULL) {
> + (void) fprintf(csherr, emsg, "/");
> + if (chdir("/") == -1)
> + /* I am not even try to print an error message! */
> + xexit(1);
> + cp = SAVE("/");
> }
>
> dp = xcalloc(1, sizeof(struct directory));
>
smtpctl.8 typo
Hello, My spell-checker found the following typo this morning. OK? - Michael Index: smtpctl.8 === RCS file: /cvs/src/usr.sbin/smtpd/smtpctl.8,v retrieving revision 1.63 diff -u -p -u -r1.63 smtpctl.8 --- smtpctl.8 14 May 2018 15:23:05 - 1.63 +++ smtpctl.8 18 Sep 2018 03:32:22 - @@ -204,7 +204,7 @@ Current runstate: either "pending" or "i .Xr smtpd 8 is running, or "offline" otherwise. .It -Delay in seconds before the next attempt if pending, or time ellapsed +Delay in seconds before the next attempt if pending, or time elapsed if currently running. This field is blank if .Xr smtpd 8
csh: remove macros xmalloc,xcalloc,xreallocarray
Hello,
In csh(1) the functions Malloc(), Calloc() & Reallocarray()
are always called via the macros (which don't really do anything).
This patch renames the functions and deletes the macros.
Does this look OK?
- Michael
Index: csh.h
===
RCS file: /cvs/src/bin/csh/csh.h,v
retrieving revision 1.30
diff -u -p -u -r1.30 csh.h
--- csh.h 30 Aug 2017 06:42:21 - 1.30
+++ csh.h 17 Sep 2018 15:45:21 -
@@ -69,10 +69,6 @@ typedef void *ioctl_t; /* Third arg of
#include "char.h"
#include "error.h"
-#define xmalloc(i) Malloc(i)
-#define xreallocarray(p, i, j) Reallocarray(p, i, j)
-#define xcalloc(n, s) Calloc(n, s)
-
#include
FILE *cshin, *cshout, *csherr;
Index: extern.h
===
RCS file: /cvs/src/bin/csh/extern.h,v
retrieving revision 1.27
diff -u -p -u -r1.27 extern.h
--- extern.h15 Sep 2018 12:15:32 - 1.27
+++ extern.h17 Sep 2018 15:45:21 -
@@ -281,9 +281,9 @@ voidpsecs(long);
/*
* alloc.c
*/
-void * Malloc(size_t);
-void * Reallocarray(void *, size_t, size_t);
-void * Calloc(size_t, size_t);
+void * xmalloc(size_t);
+void * xreallocarray(void *, size_t, size_t);
+void * xcalloc(size_t, size_t);
/*
* str.c:
Index: alloc.c
===
RCS file: /cvs/src/bin/csh/alloc.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 alloc.c
--- alloc.c 26 Dec 2015 13:48:38 - 1.17
+++ alloc.c 17 Sep 2018 15:45:21 -
@@ -39,7 +39,7 @@
#include "extern.h"
void *
-Malloc(size_t n)
+xmalloc(size_t n)
{
void *ptr;
@@ -51,7 +51,7 @@ Malloc(size_t n)
}
void *
-Reallocarray(void * p, size_t c, size_t n)
+xreallocarray(void * p, size_t c, size_t n)
{
void *ptr;
@@ -63,7 +63,7 @@ Reallocarray(void * p, size_t c, size_t
}
void *
-Calloc(size_t s, size_t n)
+xcalloc(size_t s, size_t n)
{
void *ptr;
Re: csh: simplify strsave()
On Mon, Sep 17, 2018 at 09:06:16AM -0600, Todd C. Miller wrote: > On Mon, 17 Sep 2018 15:53:06 +0200, Martijn van Duren wrote: > > > It should be safe. There are 5 instances where any() isn't called with > > a string literal: > > Thanks for checking. OK millert@ for the diff. > > - todd Also OK miko@ as I didn't hit any cases where the source string is NULL when running this patch.
Re: csh: simplify strsave()
On Sat, Sep 15, 2018 at 06:16:42AM -0600, Todd C. Miller wrote: > On Sat, 15 Sep 2018 12:42:22 +0200, Martijn van Duren wrote: > > > While here, should we also remove any in favour of strchr? Only > > difference seems to be the return type (bool vs pointer). > > Note that any(NULL, ch) is safe whereas strchr(NULL, ch) will crash. > It is hard to say whether or not there are actual calls to any() > with a NULL string (most use a constant string) but this needs to > be checked before committing. > > - todd >From what I see the questionable any() calls look like any(short2str(a), b). The function short2str() can return NULL if its parameter is NULL. On my system I changed short2str() to do if (src == NULL) abort(); and the same for any(): if (s == NULL) abort(); I didn't hit an abort() so far, but it's too early to have any() confidence in the change.
Re: csh: simplify strsave()
On Sat, Sep 08, 2018 at 10:13:35AM +0200, Martijn van Duren wrote:
> On 09/08/18 04:57, Michael Mikonos wrote:
> > Hello,
> >
> > The function strsave() in csh(1) is practically strdup(3).
> > The only difference is memory allocation failure results in
> > calling the stderror() error handler, which will later exit.
> > This patch makes the code (IMO) clearer by removing two loops.
> > xmalloc() behaves the same as xreallocarray() in terms of
> > calling stderror(). Does this look OK?
> >
> > - Michael
> >
> Why not use strdup(3) altogether then? This way it's even more
> clear what's intended. Maybe we should even rename the function
> to xstrdup?
>
> martijn@
Your patch was better. Here is a version with the function renamed
and const added to the param list to match strdup(3).
Index: extern.h
===
RCS file: /cvs/src/bin/csh/extern.h,v
retrieving revision 1.26
diff -u -p -r1.26 extern.h
--- extern.h22 Jul 2017 09:37:21 - 1.26
+++ extern.h14 Sep 2018 14:18:23 -
@@ -201,7 +201,7 @@ int prefix(Char *, Char *);
Char **saveblk(Char **);
Char*strip(Char *);
Char*quote(Char *);
-char*strsave(char *);
+char*xstrdup(const char *);
char*strspl(char *, char *);
void udvar(Char *);
Index: error.c
===
RCS file: /cvs/src/bin/csh/error.c,v
retrieving revision 1.14
diff -u -p -r1.14 error.c
--- error.c 8 Sep 2018 01:28:39 - 1.14
+++ error.c 14 Sep 2018 14:18:23 -
@@ -289,7 +289,7 @@ seterror(int id, ...)
vsnprintf(berr, sizeof(berr), errorlist[id], va);
va_end(va);
- seterr = strsave(berr);
+ seterr = xstrdup(berr);
}
}
Index: misc.c
===
RCS file: /cvs/src/bin/csh/misc.c,v
retrieving revision 1.20
diff -u -p -r1.20 misc.c
--- misc.c 20 Jun 2017 16:44:06 - 1.20
+++ misc.c 14 Sep 2018 14:18:23 -
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include "csh.h"
#include "extern.h"
@@ -53,18 +54,16 @@ any(char *s, int c)
}
char *
-strsave(char *s)
+xstrdup(const char *s)
{
-char *n;
-char *p;
+char *n;
if (s == NULL)
s = "";
-for (p = s; *p++;)
- continue;
-n = p = xreallocarray(NULL, (p - s), sizeof(char));
-while ((*p++ = *s++) != '\0')
- continue;
+if ((n = strdup(s)) == NULL) {
+ child++;
+ stderror(ERR_NOMEM);
+}
return (n);
}
Index: str.c
===
RCS file: /cvs/src/bin/csh/str.c,v
retrieving revision 1.19
diff -u -p -r1.19 str.c
--- str.c 26 Oct 2015 16:31:09 - 1.19
+++ str.c 14 Sep 2018 14:18:23 -
@@ -77,7 +77,7 @@ short2blk(Char **src)
sdst = dst = xreallocarray(NULL, n + 1, sizeof(char *));
for (; *src != NULL; src++)
- *dst++ = strsave(short2str(*src));
+ *dst++ = xstrdup(short2str(*src));
*dst = NULL;
return (sdst);
}
Re: vmd: add some NULL checks after {c,m}alloc()
On Thu, Sep 13, 2018 at 11:10:50PM -0700, Ori Bernstein wrote:
> On Fri, 14 Sep 2018 13:50:40 +0800, Michael Mikonos wrote:
>
> > On Thu, Sep 13, 2018 at 10:08:16PM -0700, Ori Bernstein wrote:
> > > On Thu, 13 Sep 2018 10:30:54 +0800, Michael Mikonos wrote:
> > >
> > I think a check for !disk->base belongs here as done above for disk->l1.
> >
>
> I think that co
OK miko@, in case someone with a current vmd setup wants to commit this.
>
> ---
> usr.sbin/vmd/vioqcow2.c | 61 +++--
> usr.sbin/vmd/vioraw.c | 2 ++
> usr.sbin/vmd/virtio.c | 11
> usr.sbin/vmd/virtio.h | 1 +
> usr.sbin/vmd/vm.c | 3 ++
> 5 files changed, 57 insertions(+), 21 deletions(-)
>
> diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
> index 7c7e4857695..b3e4e1ec058 100644
> --- usr.sbin/vmd/vioqcow2.c
> +++ usr.sbin/vmd/vioqcow2.c
> @@ -77,7 +77,6 @@ struct qcdisk {
>
> int fd;
> uint64_t *l1;
> - char *scratch;
> off_t end;
> uint32_t clustersz;
> off_t disksz; /* In bytes */
> @@ -161,17 +160,19 @@ qc2_open(struct qcdisk *disk, int fd)
> size_t i;
> int version;
>
> + pthread_rwlock_init(>lock, NULL);
> + disk->fd = fd;
> + disk->base = NULL;
> + disk->l1 = NULL;
> +
> if (pread(fd, , sizeof header, 0) != sizeof header) {
> log_warn("%s: short read on header", __func__);
> - return -1;
> + goto error;
> }
> if (strncmp(header.magic, "QFI\xfb", 4) != 0) {
> log_warn("%s: invalid magic numbers", __func__);
> - return -1;
> + goto error;
> }
> - pthread_rwlock_init(>lock, NULL);
> - disk->fd = fd;
> - disk->base = NULL;
>
> disk->clustersz = (1ull << be32toh(header.clustershift));
> disk->disksz= be64toh(header.disksz);
> @@ -182,6 +183,7 @@ qc2_open(struct qcdisk *disk, int fd)
> disk->refoff= be64toh(header.refoff);
> disk->nsnap = be32toh(header.snapcount);
> disk->snapoff = be64toh(header.snapsz);
> +
> /*
>* The additional features here are defined as 0 in the v2 format,
>* so as long as we clear the buffer before parsing, we don't need
> @@ -196,23 +198,25 @@ qc2_open(struct qcdisk *disk, int fd)
>* We only know about the dirty or corrupt bits here.
>*/
> if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) {
> - log_warn("%s: unsupported features %llx", __func__,
> + log_warnx("%s: unsupported features %llx", __func__,
> disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT));
> - return -1;
> + goto error;
> }
>
> disk->l1 = calloc(disk->l1sz, sizeof *disk->l1);
> - if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off)
> + if (!disk->l1)
> + goto error;
> + if (pread(disk->fd, disk->l1, 8*disk->l1sz, disk->l1off)
> != 8*disk->l1sz) {
> - free(disk->l1);
> - return -1;
> + log_warn("%s: unable to read qcow2 L1 table", __func__);
> + goto error;
> }
> for (i = 0; i < disk->l1sz; i++)
> disk->l1[i] = be64toh(disk->l1[i]);
> version = be32toh(header.version);
> if (version != 2 && version != 3) {
> log_warn("%s: unknown qcow2 version %d", __func__, version);
> - return -1;
> + goto error;
> }
>
> backingoff = be64toh(header.backingoff);
> @@ -223,34 +227,47 @@ qc2_open(struct qcdisk *disk, int fd)
>* otherwise we just crash with a pledge violation.
>*/
> log_warn("%s: unsupported external snapshot images", __func__);
> - return -1;
> + goto error;
>
> if (backingsz >= sizeof basepath - 1) {
> log_warn("%s: snapshot path too long", __func__);
> - return -1;
> + goto error;
> }
> if (pread(fd, basepath, backingsz, backingoff) != backingsz) {
> log_warn("%s: could not read snapshot base name",
> __func__);
> - return -1;
> + goto error;
>
Re: vmd: add some NULL checks after {c,m}alloc()
On Thu, Sep 13, 2018 at 10:08:16PM -0700, Ori Bernstein wrote:
> On Thu, 13 Sep 2018 10:30:54 +0800, Michael Mikonos wrote:
>
> > Thanks for the explanation. Overall it seems better having the
> > clean-up code localised in qc2_close() so I'm happy to OK the
> > patch with the close() call put back in (could you please forward
> > the updated patch to the list in case someone else wants to look).
>
> Updated.
>
> ---
> usr.sbin/vmd/vioqcow2.c | 59 ++---
> usr.sbin/vmd/vioraw.c | 2 ++
> usr.sbin/vmd/virtio.c | 11
> usr.sbin/vmd/virtio.h | 1 +
> usr.sbin/vmd/vm.c | 3 +++
> 5 files changed, 55 insertions(+), 21 deletions(-)
>
> diff --git usr.sbin/vmd/vioqcow2.c usr.sbin/vmd/vioqcow2.c
> index 7c7e4857695..0a0dbc93b07 100644
> --- usr.sbin/vmd/vioqcow2.c
> +++ usr.sbin/vmd/vioqcow2.c
> @@ -77,7 +77,6 @@ struct qcdisk {
>
> int fd;
> uint64_t *l1;
> - char *scratch;
> off_t end;
> uint32_t clustersz;
> off_t disksz; /* In bytes */
> @@ -161,17 +160,19 @@ qc2_open(struct qcdisk *disk, int fd)
> size_t i;
> int version;
>
> + pthread_rwlock_init(>lock, NULL);
> + disk->fd = fd;
> + disk->base = NULL;
> + disk->l1 = NULL;
> +
> if (pread(fd, , sizeof header, 0) != sizeof header) {
> log_warn("%s: short read on header", __func__);
> - return -1;
> + goto error;
> }
> if (strncmp(header.magic, "QFI\xfb", 4) != 0) {
> log_warn("%s: invalid magic numbers", __func__);
> - return -1;
> + goto error;
> }
> - pthread_rwlock_init(>lock, NULL);
> - disk->fd = fd;
> - disk->base = NULL;
>
> disk->clustersz = (1ull << be32toh(header.clustershift));
> disk->disksz= be64toh(header.disksz);
> @@ -182,6 +183,7 @@ qc2_open(struct qcdisk *disk, int fd)
> disk->refoff= be64toh(header.refoff);
> disk->nsnap = be32toh(header.snapcount);
> disk->snapoff = be64toh(header.snapsz);
> +
> /*
>* The additional features here are defined as 0 in the v2 format,
>* so as long as we clear the buffer before parsing, we don't need
> @@ -196,23 +198,25 @@ qc2_open(struct qcdisk *disk, int fd)
>* We only know about the dirty or corrupt bits here.
>*/
> if (disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT)) {
> - log_warn("%s: unsupported features %llx", __func__,
> + log_warnx("%s: unsupported features %llx", __func__,
> disk->incompatfeatures & ~(QCOW2_DIRTY|QCOW2_CORRUPT));
> - return -1;
> + goto error;
> }
>
> disk->l1 = calloc(disk->l1sz, sizeof *disk->l1);
> - if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off)
> + if (!disk->l1)
> + goto error;
> + if (pread(disk->fd, disk->l1, 8*disk->l1sz, disk->l1off)
> != 8*disk->l1sz) {
> - free(disk->l1);
> - return -1;
> + log_warn("%s: unable to read qcow2 L1 table", __func__);
> + goto error;
> }
> for (i = 0; i < disk->l1sz; i++)
> disk->l1[i] = be64toh(disk->l1[i]);
> version = be32toh(header.version);
> if (version != 2 && version != 3) {
> log_warn("%s: unknown qcow2 version %d", __func__, version);
> - return -1;
> + goto error;
> }
>
> backingoff = be64toh(header.backingoff);
> @@ -223,34 +227,45 @@ qc2_open(struct qcdisk *disk, int fd)
>* otherwise we just crash with a pledge violation.
>*/
> log_warn("%s: unsupported external snapshot images", __func__);
> - return -1;
> + goto error;
>
> if (backingsz >= sizeof basepath - 1) {
> log_warn("%s: snapshot path too long", __func__);
> - return -1;
> + goto error;
> }
> if (pread(fd, basepath, backingsz, backingoff) != backingsz) {
> log_warn("%s: could not read snapshot base name",
> __func__);
> - return -1;
> + goto error;
> }
> basepath[back
Re: vmd: add some NULL checks after {c,m}alloc()
On Wed, Sep 12, 2018 at 11:33:11AM -0700, Ori Bernstein wrote: > On Wed, 12 Sep 2018 15:36:32 +0800 > Michael Mikonos wrote: > > > On Wed, Sep 12, 2018 at 12:13:35AM -0700, Ori Bernstein wrote: > > > On Tue, 11 Sep 2018 23:29:53 -0700, Ori Bernstein > > > wrote: > > > > > > > static ssize_t > > > > @@ -362,8 +377,9 @@ qc2_close(void *p) > > > > struct qcdisk *disk; > > > > > > > > disk = p; > > > > - pwrite(disk->fd, disk->l1, disk->l1sz, disk->l1off); > > > > - close(disk->fd); > > > > + if (disk->base) > > > > + qc2_close(disk->base); > > > > + free(disk->l1); > > > > free(disk); > > > > } > > > > > > > > > > Er, of course, the close should still exist here. > > > > I was curious about qc2_close() calling itself for disk->base. > > We open it recursively for multi-disk snapshots, where the disk > specifies the base to write deltas off of. We should close it > the same way. > > > Also, is it worth setting disk->fd = -1 after close(), and > > adding a check to prevent closing it again? > > The data is in a malloc'ed structure we're freeing on the next > line. I'm not sure that there's much point in setting the fd > to -1 and checking it: if we try closing the same fd again, we've > got bigger problems. Thanks for the explanation. Overall it seems better having the clean-up code localised in qc2_close() so I'm happy to OK the patch with the close() call put back in (could you please forward the updated patch to the list in case someone else wants to look). > > -- > Ori Bernstein
Re: vmd: add some NULL checks after {c,m}alloc()
On Wed, Sep 12, 2018 at 12:13:35AM -0700, Ori Bernstein wrote: > On Tue, 11 Sep 2018 23:29:53 -0700, Ori Bernstein wrote: > > > static ssize_t > > @@ -362,8 +377,9 @@ qc2_close(void *p) > > struct qcdisk *disk; > > > > disk = p; > > - pwrite(disk->fd, disk->l1, disk->l1sz, disk->l1off); > > - close(disk->fd); > > + if (disk->base) > > + qc2_close(disk->base); > > + free(disk->l1); > > free(disk); > > } > > > > Er, of course, the close should still exist here. I was curious about qc2_close() calling itself for disk->base. Also, is it worth setting disk->fd = -1 after close(), and adding a check to prevent closing it again? > > -- > Ori Bernstein >
Re: vmd: add some NULL checks after {c,m}alloc()
On Tue, Sep 11, 2018 at 11:25:52AM -0700, Ori Bernstein wrote:
> On Tue, 11 Sep 2018 15:36:49 +0800
> Michael Mikonos wrote:
>
> > Hello,
> >
> > Sometimes vmd doesn't seem to check the result of malloc/calloc.
> > I tried to preserve the existing behavour w.r.t. return values
> > for the functions modified; some functions returned 1 on error
> > while others return -1. Does this look correct?
> >
> > - Michael
> >
> >
>
> > Index: vioqcow2.c
> > ===
> > RCS file: /cvs/src/usr.sbin/vmd/vioqcow2.c,v
> > retrieving revision 1.2
> > diff -u -p -u -r1.2 vioqcow2.c
> > --- vioqcow2.c 11 Sep 2018 04:06:32 - 1.2
> > +++ vioqcow2.c 11 Sep 2018 07:29:10 -
> > @@ -202,6 +202,9 @@ qc2_open(struct qcdisk *disk, int fd)
> > }
> >
> > disk->l1 = calloc(disk->l1sz, sizeof *disk->l1);
> > + if (disk->l1 == NULL)
> > + return -1;
> > +
> > if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off)
> > != 8*disk->l1sz) {
> > free(disk->l1);
> > @@ -237,6 +240,8 @@ qc2_open(struct qcdisk *disk, int fd)
> > basepath[backingsz] = 0;
> >
> > disk->base = calloc(1, sizeof(struct qcdisk));
> > + if (disk->base == NULL)
> > + return -1;
>
> This early return leaks disk->l1. The other vioqcow2/vioraw changes
> look fine to me.
Thanks for the feedback.
The other eary returns which currently free disk->base don't free
disk->l1. I can change those error cases, and this calloc() one, to free
disk->l1 if that's what is intended.
>
> > if (qc2_openpath(disk->base, basepath, O_RDONLY) == -1) {
> > free(disk->base);
> > return -1;
> > Index: vioraw.c
> > ===
> > RCS file: /cvs/src/usr.sbin/vmd/vioraw.c,v
> > retrieving revision 1.1
> > diff -u -p -u -r1.1 vioraw.c
> > --- vioraw.c25 Aug 2018 04:16:09 - 1.1
> > +++ vioraw.c11 Sep 2018 07:29:10 -
> > @@ -62,6 +62,8 @@ virtio_init_raw(struct virtio_backing *f
> > return -1;
> >
> > fdp = malloc(sizeof(int));
> > + if (fdp == NULL)
> > + return -1;
> > *fdp = fd;
> > file->p = fdp;
> > file->pread = raw_pread;
> >
>
>
> --
> Ori Bernstein
>
Re: vmd: add some NULL checks after {c,m}alloc()
On Tue, Sep 11, 2018 at 03:36:49PM +0800, Michael Mikonos wrote:
> Hello,
>
> Sometimes vmd doesn't seem to check the result of malloc/calloc.
> I tried to preserve the existing behavour w.r.t. return values
> for the functions modified; some functions returned 1 on error
> while others return -1. Does this look correct?
I think the previous patch was too simple minded. Please see new
patch below.
In elf{32,64}_exec() phdr is freed before anything else is
allocated, so later error blocks don't need to free it.
shp is allocated before shstr so free(shp) if allocating shstr
fails.
In qc2_open() disk is passed as a parameter and disk->l1 is
only free()d if pread() fails. Zero the disk->l1 and disk->base
pointers if they are free()d on error.
Index: loadfile_elf.c
===
RCS file: /cvs/src/usr.sbin/vmd/loadfile_elf.c,v
retrieving revision 1.30
diff -u -p -u -r1.30 loadfile_elf.c
--- loadfile_elf.c 17 Jul 2018 13:47:06 - 1.30
+++ loadfile_elf.c 11 Sep 2018 12:22:36 -
@@ -716,6 +716,8 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
sz = elf->e_phnum * sizeof(Elf64_Phdr);
phdr = malloc(sz);
+ if (phdr == NULL)
+ return 1;
if (fseeko(fp, (off_t)elf->e_phoff, SEEK_SET) == -1) {
free(phdr);
@@ -813,6 +815,8 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
}
sz = elf->e_shnum * sizeof(Elf64_Shdr);
shp = malloc(sz);
+ if (shp == NULL)
+ return 1;
if (fread(shp, 1, sz, fp) != sz) {
free(shp);
@@ -824,6 +828,10 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
size_t shstrsz = shp[elf->e_shstrndx].sh_size;
char *shstr = malloc(shstrsz);
+ if (shstr == NULL) {
+ free(shp);
+ return 1;
+ }
if (fseeko(fp, (off_t)shp[elf->e_shstrndx].sh_offset,
SEEK_SET) == -1) {
free(shstr);
@@ -938,6 +946,8 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
sz = elf->e_phnum * sizeof(Elf32_Phdr);
phdr = malloc(sz);
+ if (phdr == NULL)
+ return 1;
if (fseeko(fp, (off_t)elf->e_phoff, SEEK_SET) == -1) {
free(phdr);
@@ -1035,6 +1045,8 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
}
sz = elf->e_shnum * sizeof(Elf32_Shdr);
shp = malloc(sz);
+ if (shp == NULL)
+ return 1;
if (fread(shp, 1, sz, fp) != sz) {
free(shp);
@@ -1046,6 +1058,10 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
size_t shstrsz = shp[elf->e_shstrndx].sh_size;
char *shstr = malloc(shstrsz);
+ if (shstr == NULL) {
+ free(shp);
+ return 1;
+ }
if (fseeko(fp, (off_t)shp[elf->e_shstrndx].sh_offset,
SEEK_SET) == -1) {
free(shstr);
Index: vioqcow2.c
===
RCS file: /cvs/src/usr.sbin/vmd/vioqcow2.c,v
retrieving revision 1.2
diff -u -p -u -r1.2 vioqcow2.c
--- vioqcow2.c 11 Sep 2018 04:06:32 - 1.2
+++ vioqcow2.c 11 Sep 2018 12:22:36 -
@@ -202,9 +202,13 @@ qc2_open(struct qcdisk *disk, int fd)
}
disk->l1 = calloc(disk->l1sz, sizeof *disk->l1);
+ if (disk->l1 == NULL)
+ return -1;
+
if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off)
!= 8*disk->l1sz) {
free(disk->l1);
+ disk->l1 = NULL;
return -1;
}
for (i = 0; i < disk->l1sz; i++)
@@ -237,14 +241,18 @@ qc2_open(struct qcdisk *disk, int fd)
basepath[backingsz] = 0;
disk->base = calloc(1, sizeof(struct qcdisk));
+ if (disk->base == NULL)
+ return -1;
if (qc2_openpath(disk->base, basepath, O_RDONLY) == -1) {
free(disk->base);
+ disk->base = NULL;
return -1;
}
if (disk->base->clustersz != disk->clustersz) {
log_warn("%s: all disks must share clustersize",
__func__);
free(disk->base);
+ disk->base = NULL;
return -1;
}
}
Index: vioraw.c
===
RCS file: /cvs/src/usr.sbin/vmd/vioraw.c,v
retrieving revision 1.1
diff -u -p -u -r1.1 vioraw.c
--- vioraw.c
vmd: add some NULL checks after {c,m}alloc()
Hello,
Sometimes vmd doesn't seem to check the result of malloc/calloc.
I tried to preserve the existing behavour w.r.t. return values
for the functions modified; some functions returned 1 on error
while others return -1. Does this look correct?
- Michael
Index: loadfile_elf.c
===
RCS file: /cvs/src/usr.sbin/vmd/loadfile_elf.c,v
retrieving revision 1.30
diff -u -p -u -r1.30 loadfile_elf.c
--- loadfile_elf.c 17 Jul 2018 13:47:06 - 1.30
+++ loadfile_elf.c 11 Sep 2018 07:29:10 -
@@ -716,6 +716,8 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
sz = elf->e_phnum * sizeof(Elf64_Phdr);
phdr = malloc(sz);
+ if (phdr == NULL)
+ return 1;
if (fseeko(fp, (off_t)elf->e_phoff, SEEK_SET) == -1) {
free(phdr);
@@ -813,6 +815,8 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
}
sz = elf->e_shnum * sizeof(Elf64_Shdr);
shp = malloc(sz);
+ if (shp == NULL)
+ return 1;
if (fread(shp, 1, sz, fp) != sz) {
free(shp);
@@ -824,6 +828,8 @@ elf64_exec(FILE *fp, Elf64_Ehdr *elf, u_
size_t shstrsz = shp[elf->e_shstrndx].sh_size;
char *shstr = malloc(shstrsz);
+ if (shstr == NULL)
+ return 1;
if (fseeko(fp, (off_t)shp[elf->e_shstrndx].sh_offset,
SEEK_SET) == -1) {
free(shstr);
@@ -938,6 +944,8 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
sz = elf->e_phnum * sizeof(Elf32_Phdr);
phdr = malloc(sz);
+ if (phdr == NULL)
+ return 1;
if (fseeko(fp, (off_t)elf->e_phoff, SEEK_SET) == -1) {
free(phdr);
@@ -1035,6 +1043,8 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
}
sz = elf->e_shnum * sizeof(Elf32_Shdr);
shp = malloc(sz);
+ if (shp == NULL)
+ return 1;
if (fread(shp, 1, sz, fp) != sz) {
free(shp);
@@ -1046,6 +1056,8 @@ elf32_exec(FILE *fp, Elf32_Ehdr *elf, u_
size_t shstrsz = shp[elf->e_shstrndx].sh_size;
char *shstr = malloc(shstrsz);
+ if (shstr == NULL)
+ return 1;
if (fseeko(fp, (off_t)shp[elf->e_shstrndx].sh_offset,
SEEK_SET) == -1) {
free(shstr);
Index: vioqcow2.c
===
RCS file: /cvs/src/usr.sbin/vmd/vioqcow2.c,v
retrieving revision 1.2
diff -u -p -u -r1.2 vioqcow2.c
--- vioqcow2.c 11 Sep 2018 04:06:32 - 1.2
+++ vioqcow2.c 11 Sep 2018 07:29:10 -
@@ -202,6 +202,9 @@ qc2_open(struct qcdisk *disk, int fd)
}
disk->l1 = calloc(disk->l1sz, sizeof *disk->l1);
+ if (disk->l1 == NULL)
+ return -1;
+
if (pread(disk->fd, (char*)disk->l1, 8*disk->l1sz, disk->l1off)
!= 8*disk->l1sz) {
free(disk->l1);
@@ -237,6 +240,8 @@ qc2_open(struct qcdisk *disk, int fd)
basepath[backingsz] = 0;
disk->base = calloc(1, sizeof(struct qcdisk));
+ if (disk->base == NULL)
+ return -1;
if (qc2_openpath(disk->base, basepath, O_RDONLY) == -1) {
free(disk->base);
return -1;
Index: vioraw.c
===
RCS file: /cvs/src/usr.sbin/vmd/vioraw.c,v
retrieving revision 1.1
diff -u -p -u -r1.1 vioraw.c
--- vioraw.c25 Aug 2018 04:16:09 - 1.1
+++ vioraw.c11 Sep 2018 07:29:10 -
@@ -62,6 +62,8 @@ virtio_init_raw(struct virtio_backing *f
return -1;
fdp = malloc(sizeof(int));
+ if (fdp == NULL)
+ return -1;
*fdp = fd;
file->p = fdp;
file->pread = raw_pread;
Re: pfctl: introduce copy_sa()
On Mon, Sep 10, 2018 at 12:16:57PM +0200, Klemens Nanni wrote: > On Mon, Sep 10, 2018 at 11:59:55AM +0200, Klemens Nanni wrote: > > Small helper to put the dance around `af' into one single location. > Wrong/bad diff, please disregard. > I was just about to reply that the size in the memcpy() might always be the same.
csh: simplify strsave()
Hello,
The function strsave() in csh(1) is practically strdup(3).
The only difference is memory allocation failure results in
calling the stderror() error handler, which will later exit.
This patch makes the code (IMO) clearer by removing two loops.
xmalloc() behaves the same as xreallocarray() in terms of
calling stderror(). Does this look OK?
- Michael
Index: misc.c
===
RCS file: /cvs/src/bin/csh/misc.c,v
retrieving revision 1.20
diff -u -p -u -r1.20 misc.c
--- misc.c 20 Jun 2017 16:44:06 - 1.20
+++ misc.c 8 Sep 2018 02:23:34 -
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include "csh.h"
#include "extern.h"
@@ -56,15 +57,13 @@ char *
strsave(char *s)
{
char *n;
-char *p;
+size_t len;
if (s == NULL)
s = "";
-for (p = s; *p++;)
- continue;
-n = p = xreallocarray(NULL, (p - s), sizeof(char));
-while ((*p++ = *s++) != '\0')
- continue;
+len = strlen(s) + 1;
+n = xmalloc(len);
+(void)memcpy(n, s, len);
return (n);
}
Re: csh: blkfree() usage
ping.
On Thu, Aug 30, 2018 at 12:20:37AM +0800, Michael Mikonos wrote:
> Hello,
>
> In csh(1) the function blkfree() behaves like free(3) and
> performs no action if its argument is NULL, so the caller
> can avoid checking.
>
> I lightly tested the following patch on i386 and amd64.
> In two places the pointer was copied to a temporary
> variable (v) before being passed to blkfree() which seemed
> a bit awkward. Would anyone be willing to OK this?
>
> - Michael
>
>
> Index: csh.c
> ===
> RCS file: /cvs/src/bin/csh/csh.c,v
> retrieving revision 1.43
> diff -u -p -u -r1.43 csh.c
> --- csh.c 16 Dec 2017 10:27:21 - 1.43
> +++ csh.c 29 Aug 2018 14:45:40 -
> @@ -885,7 +885,6 @@ pintr(int notused)
> void
> pintr1(bool wantnl)
> {
> -Char **v;
> sigset_t sigset, osigset;
>
> sigemptyset();
> @@ -914,10 +913,10 @@ pintr1(bool wantnl)
> if (gointr) {
> gotolab(gointr);
> timflg = 0;
> - if ((v = pargv) != NULL)
> - pargv = 0, blkfree(v);
> - if ((v = gargv) != NULL)
> - gargv = 0, blkfree(v);
> + blkfree(pargv);
> + pargv = NULL;
> + blkfree(gargv);
> + gargv = NULL;
> reset();
> }
> else if (intty && wantnl) {
> Index: dol.c
> ===
> RCS file: /cvs/src/bin/csh/dol.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 dol.c
> --- dol.c 16 Dec 2017 10:27:21 - 1.21
> +++ dol.c 29 Aug 2018 14:45:40 -
> @@ -952,7 +952,7 @@ heredoc(Char *term)
> ocnt = BUFSIZ;
> }
> }
> - if (pargv)
> - blkfree(pargv), pargv = 0;
> + blkfree(pargv);
> + pargv = NULL;
> }
> }
> Index: error.c
> ===
> RCS file: /cvs/src/bin/csh/error.c,v
> retrieving revision 1.13
> diff -u -p -u -r1.13 error.c
> --- error.c 16 Dec 2017 10:27:21 - 1.13
> +++ error.c 29 Aug 2018 14:45:40 -
> @@ -315,7 +315,6 @@ void
> stderror(int id, ...)
> {
> va_list va;
> -Char **v;
> int flags = id & ERR_FLAGS;
>
> id &= ~ERR_FLAGS;
> @@ -349,10 +348,10 @@ stderror(int id, ...)
> free(seterr);
> seterr = NULL;
>
> -if ((v = pargv) != NULL)
> - pargv = 0, blkfree(v);
> -if ((v = gargv) != NULL)
> - gargv = 0, blkfree(v);
> +blkfree(pargv);
> +pargv = NULL;
> +blkfree(gargv);
> +gargv = NULL;
>
> (void) fflush(cshout);
> (void) fflush(csherr);
> Index: func.c
> ===
> RCS file: /cvs/src/bin/csh/func.c,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 func.c
> --- func.c18 Dec 2017 19:12:24 - 1.37
> +++ func.c29 Aug 2018 14:45:40 -
> @@ -822,8 +822,7 @@ wfree(void)
> }
> }
>
> - if (wp->w_fe0)
> - blkfree(wp->w_fe0);
> + blkfree(wp->w_fe0);
> free(wp->w_fename);
> free(wp);
> }
> @@ -886,8 +885,8 @@ xecho(int sep, Char **v)
> (void) fflush(cshout);
> if (setintr)
> sigprocmask(SIG_BLOCK, , NULL);
> -if (gargv)
> - blkfree(gargv), gargv = 0;
> + blkfree(gargv);
> + gargv = NULL;
> }
>
> void
> @@ -1373,8 +1372,8 @@ doeval(Char **v, struct command *t)
> SHIN = dmove(saveIN, oSHIN);
> SHOUT = dmove(saveOUT, oSHOUT);
> SHERR = dmove(saveERR, oSHERR);
> -if (gv)
> - blkfree(gv), gv = NULL;
> +blkfree(gv);
> +gv = NULL;
> resexit(osetexit);
> gv = savegv;
> if (my_reenter)
> Index: glob.c
> ===
> RCS file: /cvs/src/bin/csh/glob.c,v
> retrieving revision 1.22
> diff -u -p -u -r1.22 glob.c
> --- glob.c26 Dec 2015 13:48:38 - 1.22
> +++ glob.c29 Aug 2018 14:45:40 -
> @@ -578,9 +578,7 @@ dobackp(Char *cp, bool literal)
> Char *lp, *rp;
> Char *ep, word[PATH_MAX];
>
> -if (pargv) {
> - blkfree(pargv);
> -}
> +blkfree(pargv);
> pargsiz = GLOBSPACE;
> pargv = xreallocarray(NULL, pargsiz, sizeof(Char *));
> pargv[0] = NULL;
> Index: proc.c
> ===
> RCS file: /cvs/src/bin/csh/proc.c,v
> retrieving revision 1.31
> diff -u -p -u -r1.31 proc.c
> --- proc.c22 Jul 2017 09:37:21 - 1.31
> +++ proc.c29 Aug 2018 14:45:41 -
> @@ -1073,8 +1073,8 @@ pkill(Char **v, int signum)
> cont:
> v++;
> }
> -if (gargv)
> - blkfree(gargv), gargv = 0;
> +blkfree(gargv);
> +gargv = NULL;
> sigprocmask(SIG_UNBLOCK, , NULL);
> if (err1)
> stderror(ERR_SILENT);
umidi(4) jack count
Hello,
The umidi(4) driver has three different endpoint allocation
functions, which are called by alloc_all_endpoints(). The
initial jack count can be moved here because it is zero
no matter which allocation function is used.
Also when we eventually free the jacks the count can
be reset. Does this look OK?
- Michael
Index: umidi.c
===
RCS file: /cvs/src/sys/dev/usb/umidi.c,v
retrieving revision 1.47
diff -u -p -u -r1.47 umidi.c
--- umidi.c 6 Sep 2018 09:48:23 - 1.47
+++ umidi.c 6 Sep 2018 16:24:22 -
@@ -387,6 +387,8 @@ alloc_all_endpoints(struct umidi_softc *
struct umidi_endpoint *ep;
int i;
+ sc->sc_out_num_jacks = sc->sc_in_num_jacks = 0;
+
if (UMQ_ISTYPE(sc, UMQ_TYPE_FIXED_EP))
err = alloc_all_endpoints_fixed_ep(sc);
else if (UMQ_ISTYPE(sc, UMQ_TYPE_YAMAHA))
@@ -436,8 +438,6 @@ alloc_all_endpoints_fixed_ep(struct umid
fp = umidi_get_quirk_data_from_type(sc->sc_quirk,
UMQ_TYPE_FIXED_EP);
- sc->sc_out_num_jacks = 0;
- sc->sc_in_num_jacks = 0;
sc->sc_out_num_endpoints = fp->num_out_ep;
sc->sc_in_num_endpoints = fp->num_in_ep;
sc->sc_endpoints = mallocarray(sc->sc_out_num_endpoints +
@@ -521,7 +521,6 @@ alloc_all_endpoints_yamaha(struct umidi_
int out_addr, in_addr, in_packetsize, i, dir;
size_t remain, descsize;
- sc->sc_out_num_jacks = sc->sc_in_num_jacks = 0;
out_addr = in_addr = 0;
/* detect endpoints */
@@ -627,7 +626,6 @@ alloc_all_endpoints_genuine(struct umidi
if (!p)
return USBD_NOMEM;
- sc->sc_out_num_jacks = sc->sc_in_num_jacks = 0;
sc->sc_out_num_endpoints = sc->sc_in_num_endpoints = 0;
epaddr = -1;
@@ -780,6 +778,7 @@ free_all_jacks(struct umidi_softc *sc)
if (sc->sc_out_jacks) {
free(sc->sc_jacks, M_USBDEV, jacks * sizeof(*sc->sc_out_jacks));
sc->sc_jacks = sc->sc_in_jacks = sc->sc_out_jacks = NULL;
+ sc->sc_out_num_jacks = sc->sc_in_num_jacks = 0;
}
splx(s);
}
umidi(4) alloc_all_endpoints_fixed_ep()
Hello,
In the umidi(4) driver the function alloc_all_endpoints_fixed_ep()
returns USBD_NOMEM if mallocarray() fails, but the return value is
always USBD_INVAL when 'goto error' happens. OK?
- Michael
Index: umidi.c
===
RCS file: /cvs/src/sys/dev/usb/umidi.c,v
retrieving revision 1.47
diff -u -p -u -r1.47 umidi.c
--- umidi.c 6 Sep 2018 09:48:23 - 1.47
+++ umidi.c 6 Sep 2018 14:28:21 -
@@ -428,7 +428,6 @@ free_all_endpoints(struct umidi_softc *s
static usbd_status
alloc_all_endpoints_fixed_ep(struct umidi_softc *sc)
{
- usbd_status err;
struct umq_fixed_ep_desc *fp;
struct umidi_endpoint *ep;
usb_endpoint_descriptor_t *epd;
@@ -458,14 +457,12 @@ alloc_all_endpoints_fixed_ep(struct umid
if (!epd) {
DPRINTF(("%s: cannot get endpoint descriptor(out:%d)\n",
sc->sc_dev.dv_xname, fp->out_ep[i].ep));
- err = USBD_INVAL;
goto error;
}
if (UE_GET_XFERTYPE(epd->bmAttributes)!=UE_BULK ||
UE_GET_DIR(epd->bEndpointAddress)!=UE_DIR_OUT) {
printf("%s: illegal endpoint(out:%d)\n",
sc->sc_dev.dv_xname, fp->out_ep[i].ep);
- err = USBD_INVAL;
goto error;
}
ep->sc = sc;
@@ -485,14 +482,12 @@ alloc_all_endpoints_fixed_ep(struct umid
if (!epd) {
DPRINTF(("%s: cannot get endpoint descriptor(in:%d)\n",
sc->sc_dev.dv_xname, fp->in_ep[i].ep));
- err = USBD_INVAL;
goto error;
}
if (UE_GET_XFERTYPE(epd->bmAttributes)!=UE_BULK ||
UE_GET_DIR(epd->bEndpointAddress)!=UE_DIR_IN) {
printf("%s: illegal endpoint(in:%d)\n",
sc->sc_dev.dv_xname, fp->in_ep[i].ep);
- err = USBD_INVAL;
goto error;
}
ep->sc = sc;
@@ -509,7 +504,7 @@ alloc_all_endpoints_fixed_ep(struct umid
error:
free(sc->sc_endpoints, M_USBDEV, 0);
sc->sc_endpoints = NULL;
- return err;
+ return USBD_INVAL;
}
static usbd_status
icmp6_mtudisc_clone() tweak
Hello,
The code executed on failure in icmp6_mtudisc_clone() is always
the same so it can be declared in one place. Is anyone happy to
OK this?
- Michael
Index: icmp6.c
===
RCS file: /cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.225
diff -u -p -u -r1.225 icmp6.c
--- icmp6.c 11 Jul 2018 13:06:16 - 1.225
+++ icmp6.c 4 Sep 2018 15:50:29 -
@@ -1768,20 +1768,16 @@ icmp6_mtudisc_clone(struct sockaddr *dst
rt = rtalloc(dst, RT_RESOLVE, rtableid);
/* Check if the route is actually usable */
- if (!rtisvalid(rt) || (rt->rt_flags & (RTF_REJECT|RTF_BLACKHOLE))) {
- rtfree(rt);
- return (NULL);
- }
+ if (!rtisvalid(rt) || (rt->rt_flags & (RTF_REJECT|RTF_BLACKHOLE)))
+ goto bad;
/*
* No PMTU for local routes and permanent neighbors,
* ARP and NDP use the same expire timer as the route.
*/
if (ISSET(rt->rt_flags, RTF_LOCAL) ||
- (ISSET(rt->rt_flags, RTF_LLINFO) && rt->rt_expire == 0)) {
- rtfree(rt);
- return (NULL);
- }
+ (ISSET(rt->rt_flags, RTF_LLINFO) && rt->rt_expire == 0))
+ goto bad;
/* If we didn't get a host route, allocate one */
if ((rt->rt_flags & RTF_HOST) == 0) {
@@ -1799,10 +1795,8 @@ icmp6_mtudisc_clone(struct sockaddr *dst
error = rtrequest(RTM_ADD, , rt->rt_priority, ,
rtableid);
- if (error) {
- rtfree(rt);
- return (NULL);
- }
+ if (error)
+ goto bad;
nrt->rt_rmx = rt->rt_rmx;
rtfree(rt);
rt = nrt;
@@ -1810,12 +1804,13 @@ icmp6_mtudisc_clone(struct sockaddr *dst
}
error = rt_timer_add(rt, icmp6_mtudisc_timeout, icmp6_mtudisc_timeout_q,
rtableid);
- if (error) {
- rtfree(rt);
- return (NULL);
- }
+ if (error)
+ goto bad;
return (rt);
+bad:
+ rtfree(rt);
+ return (NULL);
}
void
Re: smtpd: malloc+strlcpy -> strndup
On Mon, Sep 03, 2018 at 02:24:49PM +0800, Michael Mikonos wrote:
> On Sat, Sep 01, 2018 at 11:31:49PM +0200, Gilles Chehade wrote:
> > On Sat, Sep 01, 2018 at 09:20:59PM +0800, Michael Mikonos wrote:
> > > Hello,
> > >
> > > Replace a malloc+strlcpy with strndup in cmdline_symset().
> > > Parameter s is a "keyname=value" string and sym is the
> > > "keyname" part.
> > >
> > > If s is "=value", sym will be an empty string.
> > > The patch doesn't change this behaviour although
> > > it might be undesirable to call symset() with
> > > an empty string. Possibly it could also return -1
> > > if len is zero. Thoughts?
> > >
> >
> > Not opposed to the diff but at this late hour I find it easier to read
> > the malloc+strlcpy and be sure there's not an off-by-one than with the
> > strndup version, I'll read again tomorrow.
>
> In my understanding the length argument of strndup(3) doesn't include
> the terminating NUL character. I think the linux manual for strndup(3)
> is slightly clearer on this because it has the text:
>
> ... only n bytes are copied, and a terminating null byte ('\0') is
> added.
>
> > Just wanted to remind you that this function is shared between daemons
> > so this can't be an smtpd-only change :-)
Thanks for the reminder. Here is a new version of the patch to include
other daemons. I also followed a suggestion from halex@ to remove the
strlen() calls and determine length using val-s. Did I miss anything?
Index: acme-client/parse.y
===
RCS file: /cvs/src/usr.sbin/acme-client/parse.y,v
retrieving revision 1.29
diff -u -p -u -r1.29 parse.y
--- acme-client/parse.y 3 Aug 2018 17:57:21 - 1.29
+++ acme-client/parse.y 3 Sep 2018 15:18:23 -
@@ -839,17 +839,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return -1;
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- errx(EXIT_FAILURE, "cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
+ errx(EXIT_FAILURE, "%s: strndup", __func__);
ret = symset(sym, val + 1, 1);
free(sym);
Index: bgpd/parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.331
diff -u -p -u -r1.331 parse.y
--- bgpd/parse.y27 Aug 2018 19:32:37 - 1.331
+++ bgpd/parse.y3 Sep 2018 15:18:24 -
@@ -3145,17 +3145,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- fatal("cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
+ fatal("%s: strndup", __func__);
ret = symset(sym, val + 1, 1);
free(sym);
Index: dvmrpd/parse.y
===
RCS file: /cvs/src/usr.sbin/dvmrpd/parse.y,v
retrieving revision 1.36
diff -u -p -u -r1.36 parse.y
--- dvmrpd/parse.y 11 Jul 2018 07:39:22 - 1.36
+++ dvmrpd/parse.y 3 Sep 2018 15:18:24 -
@@ -834,17 +834,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- errx(1, "cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
+ errx(1, "%s: strndup", __func__);
ret = symset(sym, val + 1, 1);
free(sym);
Index: eigrpd/parse.y
===
RCS file: /cvs/src/usr.sbin/eigrpd/parse.y,v
retrieving revision 1.27
diff -u -p -u -r1.27 parse.y
--- eigrpd/parse.y 11 Jul 2018 07:39:22 - 1.27
+++ eigrpd/parse.y 3 Sep 2018 15:18:24 -
@@ -1094,17 +1094,12 @@ cmdline_symset(char *s)
{
char*sym, *val;
int ret;
- size_t len;
if ((val = strrchr(s, '=')) == NULL)
return (-1);
-
- len = strlen(s) - strlen(val) + 1;
- if ((sym = malloc(len)) == NULL)
- errx(1, "cmdline_symset: malloc");
-
- strlcpy(sym, s, len);
-
+ sym = strndup(s, val - s);
+ if (sym == NULL)
Re: smtpd: malloc+strlcpy -> strndup
On Sat, Sep 01, 2018 at 11:31:49PM +0200, Gilles Chehade wrote:
> On Sat, Sep 01, 2018 at 09:20:59PM +0800, Michael Mikonos wrote:
> > Hello,
> >
> > Replace a malloc+strlcpy with strndup in cmdline_symset().
> > Parameter s is a "keyname=value" string and sym is the
> > "keyname" part.
> >
> > If s is "=value", sym will be an empty string.
> > The patch doesn't change this behaviour although
> > it might be undesirable to call symset() with
> > an empty string. Possibly it could also return -1
> > if len is zero. Thoughts?
> >
>
> Not opposed to the diff but at this late hour I find it easier to read
> the malloc+strlcpy and be sure there's not an off-by-one than with the
> strndup version, I'll read again tomorrow.
In my understanding the length argument of strndup(3) doesn't include
the terminating NUL character. I think the linux manual for strndup(3)
is slightly clearer on this because it has the text:
... only n bytes are copied, and a terminating null byte ('\0') is
added.
> Just wanted to remind you that this function is shared between daemons
> so this can't be an smtpd-only change :-)
>
>
> > Index: parse.y
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
> > retrieving revision 1.218
> > diff -u -p -u -r1.218 parse.y
> > --- parse.y 25 Aug 2018 19:05:23 - 1.218
> > +++ parse.y 1 Sep 2018 12:42:45 -
> > @@ -2129,11 +2129,10 @@ cmdline_symset(char *s)
> > if ((val = strrchr(s, '=')) == NULL)
> > return (-1);
> >
> > - len = strlen(s) - strlen(val) + 1;
> > - if ((sym = malloc(len)) == NULL)
> > - errx(1, "cmdline_symset: malloc");
> > -
> > - (void)strlcpy(sym, s, len);
> > + len = strlen(s) - strlen(val);
> > + sym = strndup(s, len);
> > + if (sym == NULL)
> > + errx(1, "%s: strndup", __func__);
> >
> > ret = symset(sym, val + 1, 1);
> > free(sym);
> >
>
> --
> Gilles Chehade
>
> https://www.poolp.org @poolpOrg
smtpd: malloc+strlcpy -> strndup
Hello, Replace a malloc+strlcpy with strndup in cmdline_symset(). Parameter s is a "keyname=value" string and sym is the "keyname" part. If s is "=value", sym will be an empty string. The patch doesn't change this behaviour although it might be undesirable to call symset() with an empty string. Possibly it could also return -1 if len is zero. Thoughts? - Michael Index: parse.y === RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v retrieving revision 1.218 diff -u -p -u -r1.218 parse.y --- parse.y 25 Aug 2018 19:05:23 - 1.218 +++ parse.y 1 Sep 2018 12:42:45 - @@ -2129,11 +2129,10 @@ cmdline_symset(char *s) if ((val = strrchr(s, '=')) == NULL) return (-1); - len = strlen(s) - strlen(val) + 1; - if ((sym = malloc(len)) == NULL) - errx(1, "cmdline_symset: malloc"); - - (void)strlcpy(sym, s, len); + len = strlen(s) - strlen(val); + sym = strndup(s, len); + if (sym == NULL) + errx(1, "%s: strndup", __func__); ret = symset(sym, val + 1, 1); free(sym);
smtpd: smtp_client_state() error message
Hello,
smtp_client_state() and smtp_client_response() both do
switch (proto->state) ...
but smtp_client_state() doesn't print the id of the bad state
in the default case. This patch makes the fatalx() message the
same for both. Does this look OK?
- Michael
Index: smtp_client.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_client.c,v
retrieving revision 1.6
diff -u -p -u -r1.6 smtp_client.c
--- smtp_client.c 30 Aug 2018 11:58:01 - 1.6
+++ smtp_client.c 1 Sep 2018 10:32:43 -
@@ -420,7 +420,7 @@ smtp_client_state(struct smtp_client *pr
break;
default:
- fatalx("smtp_client_state: unknown state");
+ fatalx("%s: bad state %d", __func__, proto->state);
}
#undef smtp_client_state
}
Re: csh: blkfree() usage
I also tested this patch on landisk, but this was mainly
as an exercise to try a recent snapshot under the gxemul
emulator (v0.6.0).
On Thu, Aug 30, 2018 at 12:20:37AM +0800, Michael Mikonos wrote:
> Hello,
>
> In csh(1) the function blkfree() behaves like free(3) and
> performs no action if its argument is NULL, so the caller
> can avoid checking.
>
> I lightly tested the following patch on i386 and amd64.
> In two places the pointer was copied to a temporary
> variable (v) before being passed to blkfree() which seemed
> a bit awkward. Would anyone be willing to OK this?
>
> - Michael
>
>
> Index: csh.c
> ===
> RCS file: /cvs/src/bin/csh/csh.c,v
> retrieving revision 1.43
> diff -u -p -u -r1.43 csh.c
> --- csh.c 16 Dec 2017 10:27:21 - 1.43
> +++ csh.c 29 Aug 2018 14:45:40 -
> @@ -885,7 +885,6 @@ pintr(int notused)
> void
> pintr1(bool wantnl)
> {
> -Char **v;
> sigset_t sigset, osigset;
>
> sigemptyset();
> @@ -914,10 +913,10 @@ pintr1(bool wantnl)
> if (gointr) {
> gotolab(gointr);
> timflg = 0;
> - if ((v = pargv) != NULL)
> - pargv = 0, blkfree(v);
> - if ((v = gargv) != NULL)
> - gargv = 0, blkfree(v);
> + blkfree(pargv);
> + pargv = NULL;
> + blkfree(gargv);
> + gargv = NULL;
> reset();
> }
> else if (intty && wantnl) {
> Index: dol.c
> ===
> RCS file: /cvs/src/bin/csh/dol.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 dol.c
> --- dol.c 16 Dec 2017 10:27:21 - 1.21
> +++ dol.c 29 Aug 2018 14:45:40 -
> @@ -952,7 +952,7 @@ heredoc(Char *term)
> ocnt = BUFSIZ;
> }
> }
> - if (pargv)
> - blkfree(pargv), pargv = 0;
> + blkfree(pargv);
> + pargv = NULL;
> }
> }
> Index: error.c
> ===
> RCS file: /cvs/src/bin/csh/error.c,v
> retrieving revision 1.13
> diff -u -p -u -r1.13 error.c
> --- error.c 16 Dec 2017 10:27:21 - 1.13
> +++ error.c 29 Aug 2018 14:45:40 -
> @@ -315,7 +315,6 @@ void
> stderror(int id, ...)
> {
> va_list va;
> -Char **v;
> int flags = id & ERR_FLAGS;
>
> id &= ~ERR_FLAGS;
> @@ -349,10 +348,10 @@ stderror(int id, ...)
> free(seterr);
> seterr = NULL;
>
> -if ((v = pargv) != NULL)
> - pargv = 0, blkfree(v);
> -if ((v = gargv) != NULL)
> - gargv = 0, blkfree(v);
> +blkfree(pargv);
> +pargv = NULL;
> +blkfree(gargv);
> +gargv = NULL;
>
> (void) fflush(cshout);
> (void) fflush(csherr);
> Index: func.c
> ===
> RCS file: /cvs/src/bin/csh/func.c,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 func.c
> --- func.c18 Dec 2017 19:12:24 - 1.37
> +++ func.c29 Aug 2018 14:45:40 -
> @@ -822,8 +822,7 @@ wfree(void)
> }
> }
>
> - if (wp->w_fe0)
> - blkfree(wp->w_fe0);
> + blkfree(wp->w_fe0);
> free(wp->w_fename);
> free(wp);
> }
> @@ -886,8 +885,8 @@ xecho(int sep, Char **v)
> (void) fflush(cshout);
> if (setintr)
> sigprocmask(SIG_BLOCK, , NULL);
> -if (gargv)
> - blkfree(gargv), gargv = 0;
> + blkfree(gargv);
> + gargv = NULL;
> }
>
> void
> @@ -1373,8 +1372,8 @@ doeval(Char **v, struct command *t)
> SHIN = dmove(saveIN, oSHIN);
> SHOUT = dmove(saveOUT, oSHOUT);
> SHERR = dmove(saveERR, oSHERR);
> -if (gv)
> - blkfree(gv), gv = NULL;
> +blkfree(gv);
> +gv = NULL;
> resexit(osetexit);
> gv = savegv;
> if (my_reenter)
> Index: glob.c
> ===
> RCS file: /cvs/src/bin/csh/glob.c,v
> retrieving revision 1.22
> diff -u -p -u -r1.22 glob.c
> --- glob.c26 Dec 2015 13:48:38 - 1.22
> +++ glob.c29 Aug 2018 14:45:40 -
> @@ -578,9 +578,7 @@ dobackp(Char *cp, bool literal)
> Char *lp, *rp;
> Char *ep, word[PATH_MAX];
>
> -if (pargv) {
> - blkfree(pargv);
> -}
> +blkfree(pargv);
> pargsiz = GLOBSPACE;
> pargv = xreallocarray(NULL, pargsiz, sizeof(Char *));
> pargv[0] = NULL;
> Index: proc.c
> ===
> RCS file: /cvs/src/bin/csh/proc.c,v
> retrieving revision 1.31
> diff -u -p -u -r1.31 proc.c
> --- proc.c22 Jul 2017 09:37:21 - 1.31
> +++ proc.c29 Aug 2018 14:45:41 -
> @@ -1073,8 +1073,8 @@ pkill(Char **v, int signum)
> cont:
> v++;
> }
> -if (gargv)
> - blkfree(gargv), gargv = 0;
> +blkfree(gargv);
> +gargv = NULL;
> sigprocmask(SIG_UNBLOCK, , NULL);
> if (err1)
> stderror(ERR_SILENT);
>
Re: [PATCH] qcow2 disk format
On Thu, Aug 30, 2018 at 11:19:08PM -0700, Ori Bernstein wrote: > On Wed, 15 Aug 2018 23:02:16 -0700, Ori Bernstein wrote: > > > Updated style from feedback from off-list. Also added checks and > > erroring for incompatible extensions. > > One more update. This revision adds explicit definitions of the > disk format, and adds regress tests for parsing it. The syntax > for it is: > > disk "foo" format "qcow2" > disk "bar" format "raw" > > If the format is left unspecified, you get a raw image. This > change is fully backwards compatible, so your configs don't > need to change. > > Alternatively, you can specify the disk format on the command > line: > > vmctl start ... -d raw:mydisk -d qcow2:moredisk \ > -d harddiskraw.img > > Again, the default format is raw, and qcow2 needs to be specified > explicitly. > > Updated patch below: Hi Ori, I had one question about mkcluster() in src/usr.sbin/vmd/vioqcow2.c... + if (src_phys > 0 && copy_cluster(disk, base, disk->end, src_phys) == -1) + return -1; The other error cases around it do "goto fail" which calls pthread_rwlock_unlock(). Should this do the same? - Michael
Re: update tradcpp to 0.5.2
On Fri, Aug 31, 2018 at 12:57:14PM +1000, Jonathan Gray wrote: > update tradcpp to 0.5.2 > > release 0.5.2 (20160904) >- Fix typo in -U usage message, noticed by Joerg. >- Add a -debuglog option to send an execution trace to a file. > Intended to be used when debugging imake templates and other > complex input, not for debugging tradcpp itself. > > release 0.5.1 (20150612) >- Fix a stupid regression in 0.5 that causes it to not recognize a > pile of options. >- Fix output corruption caused by mishandling which macros are > currently in use. In particular, "curmacro" is only valid while > we're parsing a macro name and arguments, and can change once we > start expanding, so don't use it to clear the in-use flag. This > problem has been around all along but was only just exposed. >- Also don't set curmacro to null after calling expand_domacro as > that can cause us to think a macro name we just read is defined(). > This one was introduced in 0.5. >- Don't use "remove" as a local variable as gcc 4.1 gets upset > about it vs. remove(3) in stdio.h. > > release 0.5 (20150612) >- Don't report unclosed comments as "No newline at end of file". >- Don't rely on existing, as (predictably) it doesn't > work on Solaris. >- Similarly, don't rely on C11 anonymous unions as the Solaris > compiler vomits on them. >- Typo fix in man page from Jason McIntyre; and change "Usage" to > "usage" in usage for pedantic reasons, from Igor Sobrado. >- Accept "-" as either input or output file name to mean stdin or > stdout respectively. Suggested by Jonathan Gray. >- Fix output spacing behavior to match gcc when newlines appear in or > while looking for macro arguments. Partly from Joerg Sonnenberger. >- Implement __FILE__ and __LINE__ macros. Mostly from Joerg Sonnenberger. >- Implement #line. Partly from Joerg Sonnenberger. >- Declare usage() with PF(). From wiz. When reading over the patch I noticed a few "unsigned" declarations which could be expanded to "unsigned int", but some existing decls already use unsigned so it could be done as a separate patch later. Overall I think this is worth having even just for __LINE__ & __FILE__, so it's OK miko@ as is.
csh: blkfree() usage
Hello,
In csh(1) the function blkfree() behaves like free(3) and
performs no action if its argument is NULL, so the caller
can avoid checking.
I lightly tested the following patch on i386 and amd64.
In two places the pointer was copied to a temporary
variable (v) before being passed to blkfree() which seemed
a bit awkward. Would anyone be willing to OK this?
- Michael
Index: csh.c
===
RCS file: /cvs/src/bin/csh/csh.c,v
retrieving revision 1.43
diff -u -p -u -r1.43 csh.c
--- csh.c 16 Dec 2017 10:27:21 - 1.43
+++ csh.c 29 Aug 2018 14:45:40 -
@@ -885,7 +885,6 @@ pintr(int notused)
void
pintr1(bool wantnl)
{
-Char **v;
sigset_t sigset, osigset;
sigemptyset();
@@ -914,10 +913,10 @@ pintr1(bool wantnl)
if (gointr) {
gotolab(gointr);
timflg = 0;
- if ((v = pargv) != NULL)
- pargv = 0, blkfree(v);
- if ((v = gargv) != NULL)
- gargv = 0, blkfree(v);
+ blkfree(pargv);
+ pargv = NULL;
+ blkfree(gargv);
+ gargv = NULL;
reset();
}
else if (intty && wantnl) {
Index: dol.c
===
RCS file: /cvs/src/bin/csh/dol.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 dol.c
--- dol.c 16 Dec 2017 10:27:21 - 1.21
+++ dol.c 29 Aug 2018 14:45:40 -
@@ -952,7 +952,7 @@ heredoc(Char *term)
ocnt = BUFSIZ;
}
}
- if (pargv)
- blkfree(pargv), pargv = 0;
+ blkfree(pargv);
+ pargv = NULL;
}
}
Index: error.c
===
RCS file: /cvs/src/bin/csh/error.c,v
retrieving revision 1.13
diff -u -p -u -r1.13 error.c
--- error.c 16 Dec 2017 10:27:21 - 1.13
+++ error.c 29 Aug 2018 14:45:40 -
@@ -315,7 +315,6 @@ void
stderror(int id, ...)
{
va_list va;
-Char **v;
int flags = id & ERR_FLAGS;
id &= ~ERR_FLAGS;
@@ -349,10 +348,10 @@ stderror(int id, ...)
free(seterr);
seterr = NULL;
-if ((v = pargv) != NULL)
- pargv = 0, blkfree(v);
-if ((v = gargv) != NULL)
- gargv = 0, blkfree(v);
+blkfree(pargv);
+pargv = NULL;
+blkfree(gargv);
+gargv = NULL;
(void) fflush(cshout);
(void) fflush(csherr);
Index: func.c
===
RCS file: /cvs/src/bin/csh/func.c,v
retrieving revision 1.37
diff -u -p -u -r1.37 func.c
--- func.c 18 Dec 2017 19:12:24 - 1.37
+++ func.c 29 Aug 2018 14:45:40 -
@@ -822,8 +822,7 @@ wfree(void)
}
}
- if (wp->w_fe0)
- blkfree(wp->w_fe0);
+ blkfree(wp->w_fe0);
free(wp->w_fename);
free(wp);
}
@@ -886,8 +885,8 @@ xecho(int sep, Char **v)
(void) fflush(cshout);
if (setintr)
sigprocmask(SIG_BLOCK, , NULL);
-if (gargv)
- blkfree(gargv), gargv = 0;
+ blkfree(gargv);
+ gargv = NULL;
}
void
@@ -1373,8 +1372,8 @@ doeval(Char **v, struct command *t)
SHIN = dmove(saveIN, oSHIN);
SHOUT = dmove(saveOUT, oSHOUT);
SHERR = dmove(saveERR, oSHERR);
-if (gv)
- blkfree(gv), gv = NULL;
+blkfree(gv);
+gv = NULL;
resexit(osetexit);
gv = savegv;
if (my_reenter)
Index: glob.c
===
RCS file: /cvs/src/bin/csh/glob.c,v
retrieving revision 1.22
diff -u -p -u -r1.22 glob.c
--- glob.c 26 Dec 2015 13:48:38 - 1.22
+++ glob.c 29 Aug 2018 14:45:40 -
@@ -578,9 +578,7 @@ dobackp(Char *cp, bool literal)
Char *lp, *rp;
Char *ep, word[PATH_MAX];
-if (pargv) {
- blkfree(pargv);
-}
+blkfree(pargv);
pargsiz = GLOBSPACE;
pargv = xreallocarray(NULL, pargsiz, sizeof(Char *));
pargv[0] = NULL;
Index: proc.c
===
RCS file: /cvs/src/bin/csh/proc.c,v
retrieving revision 1.31
diff -u -p -u -r1.31 proc.c
--- proc.c 22 Jul 2017 09:37:21 - 1.31
+++ proc.c 29 Aug 2018 14:45:41 -
@@ -1073,8 +1073,8 @@ pkill(Char **v, int signum)
cont:
v++;
}
-if (gargv)
- blkfree(gargv), gargv = 0;
+blkfree(gargv);
+gargv = NULL;
sigprocmask(SIG_UNBLOCK, , NULL);
if (err1)
stderror(ERR_SILENT);
