Re: snmpd(8): clean up variable printing
On Wed, 2022-01-19 at 16:23 +0100, Martijn van Duren wrote:
> The new code uses smi_print_element when debugging is enabled to trace
> calls. Unfortunately the current smi_print_element lacks in quite a few
> departments. This diff rewrites smi_print_element to be more concise
> than what we currently have, without moving into the more complex
> territory that snmp(1) has.
>
> Unknown types are printed in a similar fashion to what tcpdump(8)'s
> snmp output does.
>
> This change helps mostly with exceptions (NOSUCH{OBJECT,INSTANCE} and
> ENDOFMIBVIEW) and distinguishing between different integer types.
>
> I kept the current implementation under smi_print_element_legacy, so
> that we don't change the output of trap handlers. We should probably
> revisit that one at some point, but I don't think to go into that
> territory right now.
>
> OK?
>
> martijn@
>
> p.s. I'm not particularly thrilled about the type hinting, but it's
> the cleanest that I could come up with without being too much of an
> eyesore or filling the screen up even further.
Found a missing break after the BER_TYPE_NULL case.
OK?
martijn@
Index: smi.c
===
RCS file: /cvs/src/usr.sbin/snmpd/smi.c,v
retrieving revision 1.30
diff -u -p -r1.30 smi.c
--- smi.c 21 Oct 2021 15:08:15 - 1.30
+++ smi.c 29 Jan 2022 07:28:15 -
@@ -46,6 +46,7 @@
#include "snmpd.h"
#include "mib.h"
+#include "application.h"
#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
@@ -461,8 +462,9 @@ smi_debug_elements(struct ber_element *r
}
#endif
+/* Keep around so trap handle scripts don't break */
char *
-smi_print_element(struct ber_element *root)
+smi_print_element_legacy(struct ber_element *root)
{
char*str = NULL, *buf, *p;
size_t len, i;
@@ -520,6 +522,140 @@ smi_print_element(struct ber_element *ro
case BER_TYPE_SET:
default:
str = strdup("");
+ break;
+ }
+
+ return (str);
+
+ fail:
+ free(str);
+ return (NULL);
+}
+
+char *
+smi_print_element(struct ber_element *root)
+{
+ char*str = NULL, *buf, *p;
+ long longv;
+ struct ber_oid o;
+ char strbuf[BUFSIZ];
+
+ switch (root->be_class) {
+ case BER_CLASS_UNIVERSAL:
+ switch (root->be_type) {
+ case BER_TYPE_INTEGER:
+ if (ober_get_integer(root, ) == -1)
+ goto fail;
+ if (asprintf(, "%lld", v) == -1)
+ goto fail;
+ break;
+ case BER_TYPE_OBJECT:
+ if (ober_get_oid(root, ) == -1)
+ goto fail;
+ if (asprintf(, "%s", smi_oid2string(, strbuf,
+ sizeof(strbuf), 0)) == -1)
+ goto fail;
+ break;
+ case BER_TYPE_OCTETSTRING:
+ if (ober_get_string(root, ) == -1)
+ goto fail;
+ p = reallocarray(NULL, 4, root->be_len + 1);
+ if (p == NULL)
+ goto fail;
+ strvisx(p, buf, root->be_len, VIS_NL);
+ if (asprintf(, "\"%s\"", p) == -1) {
+ free(p);
+ goto fail;
+ }
+ free(p);
+ break;
+ case BER_TYPE_NULL:
+ if (asprintf(, "null") == -1)
+ goto fail;
+ break;
+ default:
+ /* Should not happen in a valid SNMP packet */
+ if (asprintf(, "[U/%u]", root->be_type) == -1)
+ goto fail;
+ break;
+ }
+ break;
+ case BER_CLASS_APPLICATION:
+ switch (root->be_type) {
+ case SNMP_T_IPADDR:
+ if (ober_get_string(root, ) == -1)
+ goto fail;
+ if (asprintf(, "%s",
+ inet_ntoa(*(struct in_addr *)buf)) == -1)
+ goto fail;
+ break;
+ case SNMP_T_COUNTER32:
+ if (ober_get_integer(root, ) == -1)
+ goto fail;
+ if (asprintf(, "%lld(c32)", v) == -1)
+ goto fail;
+ break;
+ case SNMP_T_GAUGE32:
+ if (ober_get_integer(root, ) == -1)
+ goto fail;
+ if (asprintf(, "%lld(g32)", v) == -1)
+ goto
Re: [PATCH v2 3/3] script(1): fix exit status wording, use 125 for general failure
On Fri, Jan 28, 2022 at 5:28 AM наб wrote: > This is a base-line attempt at separating errors from the child from the > ones from script itself ‒ 125 is the general-purpose code in POSIX > utilities that exec() (with 126 being ENOEXEC and 127 ‒ ENOENT) > I just checked the draft of the next revision of the POSIX spec and can find no reference to 125 being either recommended or required as the status for general exec failures. For example, the spec for xargs includes this: EXIT STATUS The following exit values shall be returned: 0All invocations of utility returned exit status zero. 1-125 A command line meeting the specified requirements could not be assembled, one or more of the invocations of utility returned a non-zero exit status, or some other error occurred. 126The utility specified by utility was found but could not be invoked. 127The utility specified by utility could not be found. I'm confident that this isn't a change from previous versions. Where is this proposed use of 125 documented? Philip Guenther
Re: touch(1): don't leak descriptor if futimens(2) fails
On Fri, Jan 28, 2022 at 7:37 AM Scott Cheloha wrote: > On Fri, Jan 28, 2022 at 07:28:40AM -0700, Todd C. Miller wrote: > > On Thu, 27 Jan 2022 20:02:18 -0800, Philip Guenther wrote: > > > > > > I think futimens(2) and close(2) failures are exotic enough to > warrant > > > > printing the system call name. > > > > > > I don't understand this. Can you give an example of an error message > that > > > touch currently might emit where knowing that the failed call was > > > futimens() or close() would affect the analysis of how to deal with > it? I > > > mean, it looks like the only errors that futimens() could really > return are > > > EROFS, EIO, and EPERM (implies a race by different users to create the > > > file), and close() could only return EIO. For any of those errors, > you're > > > going to handle them the same whether they're from open, futimens, or > > > close, no? > > > > I agree. The actual syscall in this case is pretty much irrelevant. > > The mostly likely failure is due to an I/O error of some kind. > > Alright, you have both convinced me. > > We'll go with this patch? > Sure (but I think the rval assignment and warn() call should be in a consistent order). Philip Guenther
Re: hardware checksum ix and ixl
On Wed, Jan 26, 2022 at 11:05:51AM +0100, Claudio Jeker wrote: > On Wed, Jan 26, 2022 at 01:29:42AM +0100, Alexander Bluhm wrote: > > Hi, > > > > There were some problems with ix(4) and ixl(4) hardware checksumming > > for the output path on strict alignment architectures. > > > > I have merged jan@'s diffs and added some sanity checks and > > workarounds. > > > > - If the first mbuf is not aligned or not contigous, use m_copydata() > > to extract the IP, IPv6, TCP header. > > - If the header is in the first mbuf, use m_data for the fast path. > > - Add netstat counter for invalid header chains. This makes > > us aware when hardware checksumming fails. > > - Add netstat counter for header copies. This indicates that > > better storage allocation in the network stack is possible. > > It also allows to recognize alignment problems on non-strict > > architectures. > > - There is not risk of crashes on sparc64. > > > > Does this aproach make sense? > > I think it is overly complicated and too much data is copied around. > First of all there is no need to extract ipproto. > The code can just use the M_TCP_CSUM_OUT and M_UDP_CSUM_OUT flags (they > are not set for other protos). > Because of this only they ip_hlen needs to be accessed and this can be > done with m_getptr(). A solution with m_getptr() is where we started. It has already been rejected. The problem is that there are 6 ways to implement this feature. Every option has its drawbacks and was rejected. Options are: 1. m_getptr() and access the struct. Alignment cannot be guaranteed. 2. m_getptr() and access the byte or word. Header fields should be accessed by structs. 3. Always m_copydata. Too much overhead. 4. Always use m_data. Kernel may crash or use invalid data. 5. Combination of m_data and m_copydata. Too complex. 6. Track the fields in mbuf header. Too fragile and memory overhead. In my measurements checksum offloading gave us 10% performance boost so I want this feature. Other drivers also have it. Could we get another idea or find a consensus which option to use? > In the IP6 case even more can be skipped since ip_hlen is static for IPv6. > > In ixl(4) also the tcp header lenght needs to be extracted. Again the code > can be simplified because HW checksumming is only enabled if ip_hlen == 5 > and so the offset of the th_off field is static (for both IPv4 and IPv6). > Again m_getptr can be used to just access the byte with th_off. > > Longterm in_proto_cksum_out() should probably help provide the th_off > field. I think enforcing ip_hlen == 5 for UDP and TCP is fine, who needs > IP options on UDP and TCP? Other diffs have been rejected as they make too many assumtions how the stack works. bluhm
Re: request for testing: malloc and large allocations
On Fri, Jan 28, 2022 at 04:33:28PM +0100, Alexander Bluhm wrote:
> On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote:
> > currently malloc does cache a number of free'ed regions up to 128k in
> > size. This cache is indexed by size (in # of pages), so it is very
> > quick to check.
> >
> > Some programs allocate and deallocate larger allocations in a frantic
> > way. Accodomate those programs by also keeping a cache of regions
> > betwen 128k and 2M, in a cache of variable sized regions.
> >
> > My test case speeds up about twice. A make build gets a small speedup.
> >
> > This has been tested by myself on amd64 quite intensively. I am asking
> > for more tests, especialy on more "exotic" platforms. I wil do arm64
> > myself soon. Test can be running your favorite programs, doing make
> > builds or running tests in regress/lib/libc/malloc.
>
> I see openssl and tmux crash with this diff.
> /usr/src/regress/usr.sbin/openssl reproduces it on arm64, amd64,
> i386.
Are you running with any malloc flags?
-Otto
>
> #0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:52
> 52 L1: rep
> (gdb) bt
> #0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:52
> #1 0x0fde4ed058d8 in _libc_explicit_bzero (buf=0xfded1f8e000,
> len=18446744073709549600) at /usr/src/lib/libc/string/explicit_bzero.c:17
> #2 0x0fde4ed6f84f in unmap (d=0xfdead893830, p=Variable "p" is not
> available.
> ) at /usr/src/lib/libc/stdlib/malloc.c:805
> #3 0x0fde4ed6ceca in ofree (argpool=0x7f7c2268, p=Variable "p" is
> not available.
> ) at /usr/src/lib/libc/stdlib/malloc.c:1511
> #4 0x0fde4ed6e4cb in _libc_recallocarray (ptr=0xfded1f8e7e0,
> oldnmemb=Variable "oldnmemb" is not available.
> ) at /usr/src/lib/libc/stdlib/malloc.c:1908
> #5 0x0fdbd4787afe in xrecallocarray (ptr=Unhandled dwarf expression
> opcode 0xa3
> ) at /usr/src/usr.bin/tmux/xmalloc.c:81
> #6 0x0fdbd4703ce6 in cmd_parse_build_commands (cmds=Unhandled dwarf
> expression opcode 0xa3
> ) at cmd-parse.y:815
> #7 0x0fdbd4703d47 in cmd_parse_build_commands (cmds=Unhandled dwarf
> expression opcode 0xa3
> ) at cmd-parse.y:823
> #8 0x0fdbd47040f7 in cmd_parse_from_buffer (buf=Unhandled dwarf
> expression opcode 0xa3
> ) at cmd-parse.y:1036
> #9 0x0fdbd4703ffd in cmd_parse_from_string (
> s=0xfdbd46d6522 "bind > { display-menu -xP -yP -T
> '#[align=centre]#{pane_index} (#{pane_id})'
> '#{?#{m/r:(copy|view)-mode,#{pane_mode}},Go To Top,}' '<' {send -X
> history-top} '#{?#{m/r:(copy|view)-mode,#{pane_mode}},G"...,
> pi=0x7f7c24a0) at cmd-parse.y:959
> #10 0x0fdbd473506e in key_bindings_init () at
> /usr/src/usr.bin/tmux/key-bindings.c:636
> #11 0x0fdbd47564c8 in server_start (client=0xfdec1056000,
> flags=402653184, base=0xfdec103f400, lockfd=5, lockfile=0xfdec1041b80
> "/tmp/tmux-0/default.lock")
> at /usr/src/usr.bin/tmux/server.c:210
> #12 0x0fdbd46f8788 in client_main (base=0xfdec103f400, argc=Unhandled
> dwarf expression opcode 0xa3
> ) at /usr/src/usr.bin/tmux/client.c:165
> #13 0x0fdbd4761b62 in main (argc=0, argv=0x7f7c2880) at
> /usr/src/usr.bin/tmux/tmux.c:529
>
> bluhm
Re: touch(1): don't leak descriptor if futimens(2) fails
On Fri, 28 Jan 2022 09:37:33 -0600, Scott Cheloha wrote: > Alright, you have both convinced me. > > We'll go with this patch? Sure. OK millert@ - todd
Re: touch(1): don't leak descriptor if futimens(2) fails
On Fri, Jan 28, 2022 at 07:28:40AM -0700, Todd C. Miller wrote:
> On Thu, 27 Jan 2022 20:02:18 -0800, Philip Guenther wrote:
>
> > > I think futimens(2) and close(2) failures are exotic enough to warrant
> > > printing the system call name.
> >
> > I don't understand this. Can you give an example of an error message that
> > touch currently might emit where knowing that the failed call was
> > futimens() or close() would affect the analysis of how to deal with it? I
> > mean, it looks like the only errors that futimens() could really return are
> > EROFS, EIO, and EPERM (implies a race by different users to create the
> > file), and close() could only return EIO. For any of those errors, you're
> > going to handle them the same whether they're from open, futimens, or
> > close, no?
>
> I agree. The actual syscall in this case is pretty much irrelevant.
> The mostly likely failure is due to an I/O error of some kind.
Alright, you have both convinced me.
We'll go with this patch?
Index: touch.c
===
RCS file: /cvs/src/usr.bin/touch/touch.c,v
retrieving revision 1.26
diff -u -p -r1.26 touch.c
--- touch.c 10 Mar 2019 15:11:52 - 1.26
+++ touch.c 28 Jan 2022 15:35:07 -
@@ -137,9 +137,18 @@ main(int argc, char *argv[])
/* Create the file. */
fd = open(*argv, O_WRONLY | O_CREAT, DEFFILEMODE);
- if (fd == -1 || futimens(fd, ts) || close(fd)) {
+ if (fd == -1) {
rval = 1;
warn("%s", *argv);
+ continue;
+ }
+ if (futimens(fd, ts) == -1) {
+ warn("%s", *argv);
+ rval = 1;
+ }
+ if (close(fd) == -1) {
+ warn("%s", *argv);
+ rval = 1;
}
}
return rval;
Re: request for testing: malloc and large allocations
On Sun, Jan 09, 2022 at 02:54:43PM +0100, Otto Moerbeek wrote:
> currently malloc does cache a number of free'ed regions up to 128k in
> size. This cache is indexed by size (in # of pages), so it is very
> quick to check.
>
> Some programs allocate and deallocate larger allocations in a frantic
> way. Accodomate those programs by also keeping a cache of regions
> betwen 128k and 2M, in a cache of variable sized regions.
>
> My test case speeds up about twice. A make build gets a small speedup.
>
> This has been tested by myself on amd64 quite intensively. I am asking
> for more tests, especialy on more "exotic" platforms. I wil do arm64
> myself soon. Test can be running your favorite programs, doing make
> builds or running tests in regress/lib/libc/malloc.
I see openssl and tmux crash with this diff.
/usr/src/regress/usr.sbin/openssl reproduces it on arm64, amd64,
i386.
#0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:52
52 L1: rep
(gdb) bt
#0 L1 () at /usr/src/lib/libc/arch/amd64/string/memset.S:52
#1 0x0fde4ed058d8 in _libc_explicit_bzero (buf=0xfded1f8e000,
len=18446744073709549600) at /usr/src/lib/libc/string/explicit_bzero.c:17
#2 0x0fde4ed6f84f in unmap (d=0xfdead893830, p=Variable "p" is not
available.
) at /usr/src/lib/libc/stdlib/malloc.c:805
#3 0x0fde4ed6ceca in ofree (argpool=0x7f7c2268, p=Variable "p" is not
available.
) at /usr/src/lib/libc/stdlib/malloc.c:1511
#4 0x0fde4ed6e4cb in _libc_recallocarray (ptr=0xfded1f8e7e0,
oldnmemb=Variable "oldnmemb" is not available.
) at /usr/src/lib/libc/stdlib/malloc.c:1908
#5 0x0fdbd4787afe in xrecallocarray (ptr=Unhandled dwarf expression opcode
0xa3
) at /usr/src/usr.bin/tmux/xmalloc.c:81
#6 0x0fdbd4703ce6 in cmd_parse_build_commands (cmds=Unhandled dwarf
expression opcode 0xa3
) at cmd-parse.y:815
#7 0x0fdbd4703d47 in cmd_parse_build_commands (cmds=Unhandled dwarf
expression opcode 0xa3
) at cmd-parse.y:823
#8 0x0fdbd47040f7 in cmd_parse_from_buffer (buf=Unhandled dwarf expression
opcode 0xa3
) at cmd-parse.y:1036
#9 0x0fdbd4703ffd in cmd_parse_from_string (
s=0xfdbd46d6522 "bind > { display-menu -xP -yP -T
'#[align=centre]#{pane_index} (#{pane_id})'
'#{?#{m/r:(copy|view)-mode,#{pane_mode}},Go To Top,}' '<' {send -X history-top}
'#{?#{m/r:(copy|view)-mode,#{pane_mode}},G"..., pi=0x7f7c24a0) at
cmd-parse.y:959
#10 0x0fdbd473506e in key_bindings_init () at
/usr/src/usr.bin/tmux/key-bindings.c:636
#11 0x0fdbd47564c8 in server_start (client=0xfdec1056000, flags=402653184,
base=0xfdec103f400, lockfd=5, lockfile=0xfdec1041b80 "/tmp/tmux-0/default.lock")
at /usr/src/usr.bin/tmux/server.c:210
#12 0x0fdbd46f8788 in client_main (base=0xfdec103f400, argc=Unhandled dwarf
expression opcode 0xa3
) at /usr/src/usr.bin/tmux/client.c:165
#13 0x0fdbd4761b62 in main (argc=0, argv=0x7f7c2880) at
/usr/src/usr.bin/tmux/tmux.c:529
bluhm
Re: rpki-client RFC "compliant" MFT parsing
On Fri, Jan 28, 2022 at 03:54:14PM +0100, Claudio Jeker wrote: > On Fri, Jan 28, 2022 at 09:31:26AM +0100, Theo Buehler wrote: > > On Thu, Jan 27, 2022 at 09:38:54AM +0100, Claudio Jeker wrote: > > > On Thu, Jan 27, 2022 at 07:46:32AM +0100, Theo Buehler wrote: > > > > On Wed, Jan 26, 2022 at 04:42:04PM +0100, Claudio Jeker wrote: > > > > > So the RFC is not very clear but in general the idea is that if > > > > > multiple > > > > > MFTs are available the newest one (highest manifest number) should be > > > > > used. > > > > > > > > > > In our case there are two possible MFTs available the previously > > > > > valid on > > > > > and the now downloaded one. So adjust the parser code so that both > > > > > files > > > > > are opened and parsed and the x509 is verified. Checks like the > > > > > thisUpdate/nextUpdate validity and FileAndHash sequence are postponed. > > > > > Compare these two mfts and decide which one should be used. > > > > > Now check everything that was postponed. > > > > > > > > > > When checking the hash of files in the MFT check both locations and > > > > > remember which file was the actual match. It is important that later > > > > > on > > > > > the same file is opened. > > > > > > > > > > The error checking around MFTs had to be adjusted in some places > > > > > since it > > > > > turned out to be too noisy on stale caches. > > > > > > > > > > Please test and report unexpected behaviour. > > > > > > > > This seems to work fine here. I have read the diff and it looks good, > > > > but have not reviewed it thoroughly. Do you consider it ready for that? > > > > > > I have parts I'm not super happy with but have no better idea yet. > > > Mainly the changes to parse_entity() make that function even more complex > > > and error prone. proc_parser_mft_check() is the other function where I'm > > > not sure. So happy for any feedback to improve those bits. > > > > The only suggestion I have is the usual one: factor the complicated bits > > into a function. The diff below changes two things: > > > > 1. Add error checking to mft_compare() > > 2. Factor the RTYPE_MFT handling into parse_load_mft() > > > > I think I like the more symmetric ownership handling of the two files > > better this way. This could probably be improved quite a bit by tweaking > > mft_compare() and by choosing better variable names than foo1 and foo2. > > In a next pass we could polish the other RTYPE_* cases in entity_parse() > > to resemble each other more. > > > > I'm also happy to land your initial diff (ok tb for that) and improve > > things in tree. > > Here is my latest version. I changed mft_compare() to compare the > hexnumbers with a length check and strcmp(). Looking at BN_bn2hex() this > should always work since leading zeros are correctly suppressed. Agreed. > I also refactored the code out into parse_load_mft() but did the split a > bit different. By doing the proc_parser_mft_post() check in parse_entity() > it simplifies the return code a bit. Yes, that's a lot nicer. > So this is the version I would like to commit and we can then iterate > further in tree. Let's do this. ok
Re: rpki-client RFC "compliant" MFT parsing
On Fri, Jan 28, 2022 at 09:31:26AM +0100, Theo Buehler wrote:
> On Thu, Jan 27, 2022 at 09:38:54AM +0100, Claudio Jeker wrote:
> > On Thu, Jan 27, 2022 at 07:46:32AM +0100, Theo Buehler wrote:
> > > On Wed, Jan 26, 2022 at 04:42:04PM +0100, Claudio Jeker wrote:
> > > > So the RFC is not very clear but in general the idea is that if multiple
> > > > MFTs are available the newest one (highest manifest number) should be
> > > > used.
> > > >
> > > > In our case there are two possible MFTs available the previously valid
> > > > on
> > > > and the now downloaded one. So adjust the parser code so that both files
> > > > are opened and parsed and the x509 is verified. Checks like the
> > > > thisUpdate/nextUpdate validity and FileAndHash sequence are postponed.
> > > > Compare these two mfts and decide which one should be used.
> > > > Now check everything that was postponed.
> > > >
> > > > When checking the hash of files in the MFT check both locations and
> > > > remember which file was the actual match. It is important that later on
> > > > the same file is opened.
> > > >
> > > > The error checking around MFTs had to be adjusted in some places since
> > > > it
> > > > turned out to be too noisy on stale caches.
> > > >
> > > > Please test and report unexpected behaviour.
> > >
> > > This seems to work fine here. I have read the diff and it looks good,
> > > but have not reviewed it thoroughly. Do you consider it ready for that?
> >
> > I have parts I'm not super happy with but have no better idea yet.
> > Mainly the changes to parse_entity() make that function even more complex
> > and error prone. proc_parser_mft_check() is the other function where I'm
> > not sure. So happy for any feedback to improve those bits.
>
> The only suggestion I have is the usual one: factor the complicated bits
> into a function. The diff below changes two things:
>
> 1. Add error checking to mft_compare()
> 2. Factor the RTYPE_MFT handling into parse_load_mft()
>
> I think I like the more symmetric ownership handling of the two files
> better this way. This could probably be improved quite a bit by tweaking
> mft_compare() and by choosing better variable names than foo1 and foo2.
> In a next pass we could polish the other RTYPE_* cases in entity_parse()
> to resemble each other more.
>
> I'm also happy to land your initial diff (ok tb for that) and improve
> things in tree.
Here is my latest version. I changed mft_compare() to compare the
hexnumbers with a length check and strcmp(). Looking at BN_bn2hex() this
should always work since leading zeros are correctly suppressed.
I also refactored the code out into parse_load_mft() but did the split a
bit different. By doing the proc_parser_mft_post() check in parse_entity()
it simplifies the return code a bit.
So this is the version I would like to commit and we can then iterate
further in tree.
--
:wq Claudio
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.115
diff -u -p -r1.115 extern.h
--- extern.h24 Jan 2022 17:29:37 - 1.115
+++ extern.h25 Jan 2022 13:51:03 -
@@ -164,12 +164,19 @@ enum rtype {
RTYPE_FILE,
};
+enum location {
+ DIR_UNKNOWN,
+ DIR_TEMP,
+ DIR_VALID,
+};
+
/*
* Files specified in an MFT have their bodies hashed with SHA256.
*/
struct mftfile {
char*file; /* filename (CER/ROA/CRL, no path) */
enum rtype type; /* file type as determined by extension */
+ enum locationlocation; /* temporary or valid directory */
unsigned charhash[SHA256_DIGEST_LENGTH]; /* sha256 of body */
};
@@ -181,11 +188,13 @@ struct mftfile {
struct mft {
char*path; /* relative path to directory of the MFT */
struct mftfile *files; /* file and hash */
- size_t filesz; /* number of filenames */
char*seqnum; /* manifestNumber */
char*aia; /* AIA */
char*aki; /* AKI */
char*ski; /* SKI */
+ time_t valid_from;
+ time_t valid_until;
+ size_t filesz; /* number of filenames */
unsigned int repoid;
int stale; /* if a stale manifest */
};
@@ -349,6 +358,7 @@ struct entity {
unsigned int repoid;/* repository identifier */
int talid; /* tal identifier */
enum rtype type; /* type of entity (not RTYPE_EOF) */
+ enum locationlocation; /* which directroy the file lives in */
};
TAILQ_HEAD(entityq, entity);
@@ -416,12 +426,13 @@ struct cert *ta_parse(const char *, cons
struct cert*cert_read(struct ibuf *);
voidcert_insert_brks(struct brk_tree *, struct cert *);
+enum rtype rtype_from_file_extension(const char *);
void
Re: in4_cksum changes, step 1
On Tue, Jan 25, 2022 at 08:23:18PM +, Miod Vallat wrote:
> in4_cksum(), used to compute packet checksums for the legacy internet
> protocol, has been hand-optimized for speed on most elderly platforms,
> with the most recent pieces of silicon using a portable C
> implementation.
>
> Most of these implementations, in a not-so-uncommon case, need to
> checksum an extra (``overlay'') header, and invoke memset or bzero on
> such an overlay, prior to initializing it and checksumming it.
>
> However, except for one byte, the zeroed parts are useless since they
> will not change the checksum, so that memset/bzero call can be removed,
> and the sum can omit the first 8 bytes which will always be zero.
>
> The following diff implements that idea. Plus on sparc64 you get one
> useless assembly instruction removed, for free, isn't that awesome?
>
> Affected platforms: alpha, amd64, arm64, hppa, landisk, luna88k, macppc,
> octeon, powerpc64, riscv64, sparc64. No need to test on armv7 and i386.
>
> Index: sys/arch/alpha/alpha/in_cksum.c
> ===
> RCS file: /OpenBSD/src/sys/arch/alpha/alpha/in_cksum.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 in_cksum.c
> --- sys/arch/alpha/alpha/in_cksum.c 21 Aug 2014 14:24:08 - 1.9
> +++ sys/arch/alpha/alpha/in_cksum.c 25 Jan 2022 20:21:06 -
> @@ -212,14 +212,13 @@ in4_cksum(struct mbuf *m, u_int8_t nxt,
> panic("in4_cksum: bad mbuf chain");
> #endif
>
> - memset(, 0, sizeof(ipov));
> -
> - ipov.ih_len = htons(len);
> + ipov.ih_x1[8] = 0;
> ipov.ih_pr = nxt;
> + ipov.ih_len = htons(len);
> ipov.ih_src = mtod(m, struct ip *)->ip_src;
> ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
>
> - sum += in_cksumdata((caddr_t) , sizeof(ipov));
> + sum += in_cksumdata((caddr_t) + 8, sizeof(ipov) - 8);
I think this would be clearer with a comment.
/* assumes first 8 bytes are zeroes */
sum += in_cksumdata((caddr_t) + 8, sizeof(ipov) - 8);
> }
>
> /* skip over unnecessary part */
> Index: sys/arch/m88k/m88k/in_cksum.c
> ===
> RCS file: /OpenBSD/src/sys/arch/m88k/m88k/in_cksum.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 in_cksum.c
> --- sys/arch/m88k/m88k/in_cksum.c 21 Aug 2014 14:24:08 - 1.4
> +++ sys/arch/m88k/m88k/in_cksum.c 25 Jan 2022 20:21:07 -
> @@ -95,19 +95,22 @@ in4_cksum(struct mbuf *m, uint8_t nxt, i
> {
> u_int16_t *w;
> u_int sum = 0;
> - struct ipovly ipov;
> + union {
> + struct ipovly ipov;
> + u_int16_t w[10];
> + } u;
>
> if (nxt != 0) {
> /* pseudo header */
> - bzero(, sizeof(ipov));
> - ipov.ih_len = htons(len);
> - ipov.ih_pr = nxt;
> - ipov.ih_src = mtod(m, struct ip *)->ip_src;
> - ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
> - w = (u_int16_t *)
> - /* assumes sizeof(ipov) == 20 */
> - sum += w[0]; sum += w[1]; sum += w[2]; sum += w[3]; sum += w[4];
> - sum += w[5]; sum += w[6]; sum += w[7]; sum += w[8]; sum += w[9];
> + u.ipov.ih_x1[8] = 0;
> + u.ipov.ih_pr = nxt;
> + u.ipov.ih_len = htons(len);
> + u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
> + u.ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
> + w = u.w;
> + /* assumes sizeof(ipov) == 20 and first 8 bytes are zeroes */
> + sum += w[4]; sum += w[5]; sum += w[6];
> + sum += w[7]; sum += w[8]; sum += w[9];
Please remove the trailing space that some of the changed lines have.
> }
>
> /* skip unnecessary part */
> Index: sys/arch/powerpc/powerpc/in_cksum.c
> ===
> RCS file: /OpenBSD/src/sys/arch/powerpc/powerpc/in_cksum.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 in_cksum.c
> --- sys/arch/powerpc/powerpc/in_cksum.c 22 Jul 2014 10:35:35 -
> 1.10
> +++ sys/arch/powerpc/powerpc/in_cksum.c 25 Jan 2022 20:21:07 -
> @@ -254,15 +254,15 @@ in4_cksum(struct mbuf *m, uint8_t nxt, i
>
> if (nxt != 0) {
> /* pseudo header */
> - memset(, 0, sizeof(u.ipov));
> - u.ipov.ih_len = htons(len);
> + u.ipov.ih_x1[8] = 0;
> u.ipov.ih_pr = nxt;
> + u.ipov.ih_len = htons(len);
> u.ipov.ih_src = mtod(m, struct ip *)->ip_src;
> u.ipov.ih_dst = mtod(m, struct ip *)->ip_dst;
> w = u.w;
> - /* assumes sizeof(ipov) == 20 */
> - sum += w[0]; sum += w[1]; sum += w[2]; sum += w[3]; sum += w[4];
> - sum += w[5]; sum += w[6]; sum +=
Re: touch(1): don't leak descriptor if futimens(2) fails
On Thu, 27 Jan 2022 20:02:18 -0800, Philip Guenther wrote: > > I think futimens(2) and close(2) failures are exotic enough to warrant > > printing the system call name. > > > > I don't understand this. Can you give an example of an error message that > touch currently might emit where knowing that the failed call was > futimens() or close() would affect the analysis of how to deal with it? I > mean, it looks like the only errors that futimens() could really return are > EROFS, EIO, and EPERM (implies a race by different users to create the > file), and close() could only return EIO. For any of those errors, you're > going to handle them the same whether they're from open, futimens, or > close, no? I agree. The actual syscall in this case is pretty much irrelevant. The mostly likely failure is due to an I/O error of some kind. - todd
[PATCH v2 3/3] script(1): fix exit status wording, use 125 for general failure
This is a base-line attempt at separating errors from the child from the
ones from script itself ‒ 125 is the general-purpose code in POSIX
utilities that exec() (with 126 being ENOEXEC and 127 ‒ ENOENT)
---
Please keep me in CC, as I'm not subscribed.
usr.bin/script/script.1 | 6 ++
usr.bin/script/script.c | 6 +++---
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/usr.bin/script/script.1 b/usr.bin/script/script.1
index aa8bb2790..2ce483b97 100644
--- a/usr.bin/script/script.1
+++ b/usr.bin/script/script.1
@@ -80,10 +80,8 @@ or control-D
.Pq Ql ^D
will exit most interactive shells).
.Nm
-will exit with the status of 0 unless any of its child
-processes fail, in which case,
-.Nm
-will return 1.
+will exit with the status of the shell,
+or 125 if it couldn't execute it.
.Pp
Certain interactive commands, such as
.Xr vi 1 ,
diff --git a/usr.bin/script/script.c b/usr.bin/script/script.c
index fd2829033..1c2db608d 100644
--- a/usr.bin/script/script.c
+++ b/usr.bin/script/script.c
@@ -119,7 +119,7 @@ main(int argc, char *argv[])
default:
fprintf(stderr, "usage: %s [-a] [-c command] [file]\n",
__progname);
- exit(1);
+ exit(125);
}
argc -= optind;
argv += optind;
@@ -206,7 +206,7 @@ void
finish(int signo)
{
int save_errno = errno;
- int status, e = 1;
+ int status, e = 125;
pid_t pid;
while ((pid = wait3(, WNOHANG, 0)) > 0) {
@@ -335,7 +335,7 @@ fail(void)
{
(void)kill(0, SIGTERM);
- done(1);
+ done(125);
}
void
--
2.30.2
signature.asc
Description: PGP signature
[PATCH v2 2/3] script(1): simplify shell execution
Use execl in both paths and the same warn() call
---
Please keep me in CC, as I'm not subscribed.
usr.bin/script/script.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/usr.bin/script/script.c b/usr.bin/script/script.c
index 763975d6a..fd2829033 100644
--- a/usr.bin/script/script.c
+++ b/usr.bin/script/script.c
@@ -313,10 +313,7 @@ scriptflush(int signo)
void
doshell(char *cmd)
{
- char *shell;
- char *argp[] = {"sh", "-c", NULL, NULL};
-
- shell = getenv("SHELL");
+ const char *shell = cmd ? NULL : getenv("SHELL");
if (shell == NULL)
shell = _PATH_BSHELL;
@@ -324,14 +321,12 @@ doshell(char *cmd)
(void)fclose(fscript);
login_tty(slave);
- if (cmd != NULL) {
- argp[2] = cmd;
- execv(_PATH_BSHELL, argp);
- warn("unable to execute %s", _PATH_BSHELL);
- } else {
+ if (cmd != NULL)
+ execl(shell, "sh", "-c", cmd, (char *)NULL);
+ else
execl(shell, shell, "-i", (char *)NULL);
- warn("%s", shell);
- }
+
+ warn("unable to execute %s", shell);
fail();
}
--
2.30.2
signature.asc
Description: PGP signature
[PATCH v2 1/3] script(1): actually bubble child errors
script.1 says
> script will exit with the status of 0 unless any of its child
> processes fail, in which case, script will return 1.
This is a patent lie: it only exits with 1 if the host or writer
processes fail, not the actual child
Instead, wait for the child in the writer process and bubble its status
up verbatim (for signals ‒ as shell-style 128+sig)
---
Resending after a month. I previously posted this to user@,
per official instructions, but this may be a better place.
1/3 is def. what I'm most interested in pushing through,
since having functional pass-through fixes a bunch of dirty work-arounds
in my CI jobs, and 2/3 is simple clean-up and fixes mismatched warnings;
3/3 would be nice to have, but ultimately not that impactful.
Please keep me in CC, as I'm not subscribed.
usr.bin/script/script.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/usr.bin/script/script.c b/usr.bin/script/script.c
index da22623ff..763975d6a 100644
--- a/usr.bin/script/script.c
+++ b/usr.bin/script/script.c
@@ -251,16 +251,12 @@ dooutput(void)
sigemptyset();
sigaddset(, SIGALRM);
+ sigaddset(, SIGCHLD);
bzero(, sizeof sa);
sigemptyset(_mask);
sa.sa_handler = scriptflush;
(void)sigaction(SIGALRM, , NULL);
- bzero(, sizeof sa);
- sigemptyset(_mask);
- sa.sa_handler = SIG_IGN;
- (void)sigaction(SIGCHLD, , NULL);
-
if (pledge("stdio proc", NULL) == -1)
err(1, "pledge");
@@ -295,7 +291,17 @@ dooutput(void)
outcc += cc;
sigprocmask(SIG_UNBLOCK, , NULL);
}
- done(0);
+
+ int e = 0, err;
+ while ((err = wait()) == -1 && errno == EINTR)
+ ;
+ if (err != -1) {
+ if (WIFEXITED(e))
+ e = WEXITSTATUS(e);
+ else
+ e = 128 + WTERMSIG(e);
+ }
+ done(e);
}
void
--
2.30.2
signature.asc
Description: PGP signature
Re: rpki-client RRDP dir cleanup
On Fri, Jan 28, 2022 at 01:07:10PM +0100, Claudio Jeker wrote:
> I think I introduced a bit of an error when skipping cleanup of RRDP
> directories when RRDP is off. When RRDP is off the cache is updated via
> rsync but when RRDP is turned back on later on the cache does not match
> with the RRDP state file and so deltas will often fail to apply.
>
> It is better to clean out .rrdp if rrdp is disabled so the repo is
> properly synced.
>
> The noop test is probably fine since we don't want to remove the rrdp
> cache and state in a quick -n run. Still it would probably be better to
> skip all repo cleanup in the noop case.
>
ok
> --
> :wq Claudio
>
> Index: repo.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/repo.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 repo.c
> --- repo.c26 Jan 2022 13:57:56 - 1.28
> +++ repo.c27 Jan 2022 16:30:41 -
> @@ -1389,7 +1389,7 @@ repo_cleanup(struct filepath_tree *tree)
>* clear them if they are not used anymore but
>* only if rrdp is active.
>*/
> - if (e->fts_pointer == RRDP_DIR && !noop && rrdpon &&
> + if (e->fts_pointer == RRDP_DIR && !noop &&
> e->fts_level == 2) {
> if (!rrdp_is_active(path))
> e->fts_pointer = NULL;
>
rpki-client RRDP dir cleanup
I think I introduced a bit of an error when skipping cleanup of RRDP
directories when RRDP is off. When RRDP is off the cache is updated via
rsync but when RRDP is turned back on later on the cache does not match
with the RRDP state file and so deltas will often fail to apply.
It is better to clean out .rrdp if rrdp is disabled so the repo is
properly synced.
The noop test is probably fine since we don't want to remove the rrdp
cache and state in a quick -n run. Still it would probably be better to
skip all repo cleanup in the noop case.
--
:wq Claudio
Index: repo.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/repo.c,v
retrieving revision 1.28
diff -u -p -r1.28 repo.c
--- repo.c 26 Jan 2022 13:57:56 - 1.28
+++ repo.c 27 Jan 2022 16:30:41 -
@@ -1389,7 +1389,7 @@ repo_cleanup(struct filepath_tree *tree)
* clear them if they are not used anymore but
* only if rrdp is active.
*/
- if (e->fts_pointer == RRDP_DIR && !noop && rrdpon &&
+ if (e->fts_pointer == RRDP_DIR && !noop &&
e->fts_level == 2) {
if (!rrdp_is_active(path))
e->fts_pointer = NULL;
Re: rpki-client RFC "compliant" MFT parsing
On Thu, Jan 27, 2022 at 09:38:54AM +0100, Claudio Jeker wrote:
> On Thu, Jan 27, 2022 at 07:46:32AM +0100, Theo Buehler wrote:
> > On Wed, Jan 26, 2022 at 04:42:04PM +0100, Claudio Jeker wrote:
> > > So the RFC is not very clear but in general the idea is that if multiple
> > > MFTs are available the newest one (highest manifest number) should be
> > > used.
> > >
> > > In our case there are two possible MFTs available the previously valid on
> > > and the now downloaded one. So adjust the parser code so that both files
> > > are opened and parsed and the x509 is verified. Checks like the
> > > thisUpdate/nextUpdate validity and FileAndHash sequence are postponed.
> > > Compare these two mfts and decide which one should be used.
> > > Now check everything that was postponed.
> > >
> > > When checking the hash of files in the MFT check both locations and
> > > remember which file was the actual match. It is important that later on
> > > the same file is opened.
> > >
> > > The error checking around MFTs had to be adjusted in some places since it
> > > turned out to be too noisy on stale caches.
> > >
> > > Please test and report unexpected behaviour.
> >
> > This seems to work fine here. I have read the diff and it looks good,
> > but have not reviewed it thoroughly. Do you consider it ready for that?
>
> I have parts I'm not super happy with but have no better idea yet.
> Mainly the changes to parse_entity() make that function even more complex
> and error prone. proc_parser_mft_check() is the other function where I'm
> not sure. So happy for any feedback to improve those bits.
The only suggestion I have is the usual one: factor the complicated bits
into a function. The diff below changes two things:
1. Add error checking to mft_compare()
2. Factor the RTYPE_MFT handling into parse_load_mft()
I think I like the more symmetric ownership handling of the two files
better this way. This could probably be improved quite a bit by tweaking
mft_compare() and by choosing better variable names than foo1 and foo2.
In a next pass we could polish the other RTYPE_* cases in entity_parse()
to resemble each other more.
I'm also happy to land your initial diff (ok tb for that) and improve
things in tree.
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.115
diff -u -p -r1.115 extern.h
--- extern.h24 Jan 2022 17:29:37 - 1.115
+++ extern.h27 Jan 2022 09:13:19 -
@@ -164,12 +164,19 @@ enum rtype {
RTYPE_FILE,
};
+enum location {
+ DIR_UNKNOWN,
+ DIR_TEMP,
+ DIR_VALID,
+};
+
/*
* Files specified in an MFT have their bodies hashed with SHA256.
*/
struct mftfile {
char*file; /* filename (CER/ROA/CRL, no path) */
enum rtype type; /* file type as determined by extension */
+ enum locationlocation; /* temporary or valid directory */
unsigned charhash[SHA256_DIGEST_LENGTH]; /* sha256 of body */
};
@@ -181,11 +188,13 @@ struct mftfile {
struct mft {
char*path; /* relative path to directory of the MFT */
struct mftfile *files; /* file and hash */
- size_t filesz; /* number of filenames */
char*seqnum; /* manifestNumber */
char*aia; /* AIA */
char*aki; /* AKI */
char*ski; /* SKI */
+ time_t valid_from;
+ time_t valid_until;
+ size_t filesz; /* number of filenames */
unsigned int repoid;
int stale; /* if a stale manifest */
};
@@ -349,6 +358,7 @@ struct entity {
unsigned int repoid;/* repository identifier */
int talid; /* tal identifier */
enum rtype type; /* type of entity (not RTYPE_EOF) */
+ enum locationlocation; /* which directroy the file lives in */
};
TAILQ_HEAD(entityq, entity);
@@ -416,12 +426,13 @@ struct cert *ta_parse(const char *, cons
struct cert*cert_read(struct ibuf *);
voidcert_insert_brks(struct brk_tree *, struct cert *);
+enum rtype rtype_from_file_extension(const char *);
voidmft_buffer(struct ibuf *, const struct mft *);
voidmft_free(struct mft *);
struct mft *mft_parse(X509 **, const char *, const unsigned char *,
size_t);
struct mft *mft_read(struct ibuf *);
-enum rtype rtype_from_file_extension(const char *);
+int mft_compare(const struct mft *, const struct mft *);
voidroa_buffer(struct ibuf *, const struct roa *);
voidroa_free(struct roa *);
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.186
diff -u -p -r1.186 main.c
--- main.c 26 Jan 2022
