[PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations

[PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations

[Damian Wrobel]

They seems to be no longer needed. Tested on:

 $ gcc --version
 gcc (GCC) 11.1.1 20210531 (Red Hat 11.1.1-3)

Signed-off-by: Damian Wrobel <dwrobel@ertelnet.rybnik.pl>
---
 Makefile.in | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Makefile.in b/Makefile.in
index 10441ef..1ad8836 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -42,7 +42,7 @@ LOCALSTATE=var/pseudo
 BINDIR=$(PREFIX)/$(BIN)
 LOCALSTATEDIR=$(PREFIX)/$(LOCALSTATE)
 
-CFLAGS_BASE=-pipe -std=gnu99 -Wall -W -Wextra -Wno-deprecated-declarations
+CFLAGS_BASE=-pipe -std=gnu99 -Wall -W -Wextra
 CFLAGS_CODE=-fPIC -D_LARGEFILE64_SOURCE -D_ATFILE_SOURCE $(ARCH_FLAGS)
 CFLAGS_DEFS=-DPSEUDO_PREFIX='"$(PREFIX)"' -DPSEUDO_SUFFIX='"$(SUFFIX)"' -DPSEUDO_BINDIR='"$(BIN)"' -DPSEUDO_LIBDIR='"$(LIB)"' -DPSEUDO_LOCALSTATEDIR='"$(LOCALSTATE)"' -DPSEUDO_VERSION='"$(VERSION)"' $(SQLITE_MEMORY) $(FORCE_ASYNC) -DPSEUDO_PASSWD_FALLBACK='$(PASSWD_FALLBACK)' $(OPTDEFS) $(EPOLL)
 CFLAGS_DEBUG=-O2 -g
@@ -152,9 +152,8 @@ pseudo_tables.c pseudo_tables.h: tables
 pseudo_tables.o: pseudo_tables.c
 	$(CC) $(CFLAGS) $(CFLAGS_PSEUDO) -c -o pseudo_tables.o pseudo_tables.c
 
-# no-strict-aliasing is needed for the function pointer trickery.
 pseudo_wrappers.o: $(GUTS) pseudo_wrappers.c pseudo_wrapfuncs.c pseudo_wrapfuncs.h pseudo_tables.h
-	$(CC) -fno-strict-aliasing $(CFLAGS) $(CFLAGS_PSEUDO) -D_GNU_SOURCE -c -o pseudo_wrappers.o pseudo_wrappers.c
+	$(CC) $(CFLAGS) $(CFLAGS_PSEUDO) -D_GNU_SOURCE -c -o pseudo_wrappers.o pseudo_wrappers.c
 
 offsets32:
 	$(CC) -m32 -o offsets32 offsets.c
-- 
2.31.1

[PATCH pseudo 2/4] Do not pass null argument to pseudo_diag()

[Damian Wrobel]

Fixes the following warning:

 pseudo_client.c: In function ‘pseudo_root_path’:
 pseudo_client.c:848:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
   848 |                 pseudo_diag("couldn't allocate absolute path for '%s'.\n",
       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   849 |                         path);
       |                         ~~~~~

Signed-off-by: Damian Wrobel <dwrobel@ertelnet.rybnik.pl>
---
 pseudo_client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pseudo_client.c b/pseudo_client.c
index 579db33..2583bca 100644
--- a/pseudo_client.c
+++ b/pseudo_client.c
@@ -846,7 +846,7 @@ pseudo_root_path(const char *func, int line, int dirfd, const char *path, int le
 	pseudo_magic();
 	if (!rc) {
 		pseudo_diag("couldn't allocate absolute path for '%s'.\n",
-			path);
+			path ? path : "null");
 	}
 	pseudo_debug(PDBGF_CHROOT, "root_path [%s, %d]: '%s' from '%s'\n",
 		func, line,
-- 
2.31.1

[PATCH pseudo 3/4] Use -pthread instead of -lpthread

[Damian Wrobel]

Signed-off-by: Damian Wrobel <dwrobel@ertelnet.rybnik.pl>
---
 Makefile.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile.in b/Makefile.in
index 1ad8836..162e729 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -61,8 +61,8 @@ DBOBJS=pseudo_db.o
 WRAPOBJS=pseudo_wrappers.o
 
 # needed for anything that links with pseduo_client.o, pretty much
-CLIENT_LDFLAGS=-ldl -lpthread
-DB_LDFLAGS=@SQLITE_LDARG@ -lpthread
+CLIENT_LDFLAGS=-ldl -pthread
+DB_LDFLAGS=@SQLITE_LDARG@ -pthread
 
 PSEUDO=$(BIN)/pseudo
 PSEUDODB=$(BIN)/pseudodb
-- 
2.31.1

[PATCH pseudo 4/4] Do not return address of local variable

[Damian Wrobel]

Fixes the following warning:
  pseudo_client.c: In function ‘pseudo_client_op’:
  cc1: warning: function may return address of local variable [-Wreturn-local-addr]
  pseudo_client.c:1592:22: note: declared here
   1592 |         pseudo_msg_t msg = { .type = PSEUDO_MSG_OP };
        |                      ^~~

Signed-off-by: Damian Wrobel <dwrobel@ertelnet.rybnik.pl>
---
 pseudo_client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pseudo_client.c b/pseudo_client.c
index 2583bca..f1d09ff 100644
--- a/pseudo_client.c
+++ b/pseudo_client.c
@@ -1889,7 +1889,7 @@ pseudo_client_op(pseudo_op_t op, int access, int fd, int dirfd, const char *path
 	case OP_CHROOT:
 		if (pseudo_client_chroot(path) == 0) {
 			/* return a non-zero value to show non-failure */
-			result = &msg;
+			result = pseudo_msg_dup(&msg);
 		}
 		do_request = 0;
 		break;
-- 
2.31.1

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable

[Seebs]

On Tue, 27 Jul 2021 13:49:06 +0200
"Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:

> Fixes the following warning:
>   pseudo_client.c: In function ‘pseudo_client_op’:
>   cc1: warning: function may return address of local variable
> [-Wreturn-local-addr] pseudo_client.c:1592:22: note: declared here
>    1592 |         pseudo_msg_t msg = { .type = PSEUDO_MSG_OP };
>         |                      ^~~
> 
> Signed-off-by: Damian Wrobel <dwrobel@ertelnet.rybnik.pl>
> ---
>  pseudo_client.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pseudo_client.c b/pseudo_client.c
> index 2583bca..f1d09ff 100644
> --- a/pseudo_client.c
> +++ b/pseudo_client.c
> @@ -1889,7 +1889,7 @@ pseudo_client_op(pseudo_op_t op, int access,
> int fd, int dirfd, const char *path case OP_CHROOT:
>  		if (pseudo_client_chroot(path) == 0) {
>  			/* return a non-zero value to show
> non-failure */
> -			result = &msg;
> +			result = pseudo_msg_dup(&msg);

This is a memory leak.

That said, I have no idea how the underlying bug escaped notice all
this time, it's definitely a bug. I think it is actually safe to just
make msg be static, because pseudo_client_op is protected by a lock
and is never executed more than once at a time.

On reflection: I think the way it worked is that in that case, the
actual message isn't looked at, just checked for nullness, but this
is still undefined behavior because the result is a pointer to storage
after the storage's lifetime, and formally you can't even check those
for "is or isn't null".

-s

Re: [OE-core] [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag()

[Seebs]

On Tue, 27 Jul 2021 13:49:04 +0200
"Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:

>  		pseudo_diag("couldn't allocate absolute path for
> '%s'.\n",
> -			path);
> +			path ? path : "null");

Is there any actual code path where pseudo_root_path gets called on a
null path? I don't really object to the change, but I think that's
caught by guards elsewhere.

-s

Re: [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations

[Seebs]

On Tue, 27 Jul 2021 13:49:03 +0200
"Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:

> -# no-strict-aliasing is needed for the function pointer trickery.
>  pseudo_wrappers.o: $(GUTS) pseudo_wrappers.c pseudo_wrapfuncs.c
> pseudo_wrapfuncs.h pseudo_tables.h
> -	$(CC) -fno-strict-aliasing $(CFLAGS) $(CFLAGS_PSEUDO)
> -D_GNU_SOURCE -c -o pseudo_wrappers.o pseudo_wrappers.c

I would be really uncomfortable relying on that not being needed, even
if it happened to work in specific cases. The function pointer trickery
is black magic and very much undefined behavior, and warning the
compiler away from it seems reasonably likely to be advantageous.

-s

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable

[Damian Wrobel]

 ---- On Tue, 27 Jul 2021 17:47:12 +0200 Seebs <seebs@seebs.net> wrote ----
 > On Tue, 27 Jul 2021 13:49:06 +0200
 > "Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:
 > 
 > > Fixes the following warning:
 > >   pseudo_client.c: In function ‘pseudo_client_op’:
 > >   cc1: warning: function may return address of local variable
 > > [-Wreturn-local-addr] pseudo_client.c:1592:22: note: declared here
 > >    1592 |         pseudo_msg_t msg = { .type = PSEUDO_MSG_OP };
 > >         |                      ^~~
 > > 
 > > Signed-off-by: Damian Wrobel <dwrobel@ertelnet.rybnik.pl>
 > > ---
 > >  pseudo_client.c | 2 +-
 > >  1 file changed, 1 insertion(+), 1 deletion(-)
 > > 
 > > diff --git a/pseudo_client.c b/pseudo_client.c
 > > index 2583bca..f1d09ff 100644
 > > --- a/pseudo_client.c
 > > +++ b/pseudo_client.c
 > > @@ -1889,7 +1889,7 @@ pseudo_client_op(pseudo_op_t op, int access,
 > > int fd, int dirfd, const char *path case OP_CHROOT:
 > >          if (pseudo_client_chroot(path) == 0) {
 > >              /* return a non-zero value to show
 > > non-failure */
 > > -            result = &msg;
 > > +            result = pseudo_msg_dup(&msg);
 > 
 > This is a memory leak.

If the function needs to return "pseudo_msg_t *", then it has to return a valid pointer.
Returning a pointer to a local non static variable is an error as this address becomes invalid immediately after this function returns.
The returned pointer has to be freed by the caller not by the callee function itself.

I didn't mention that in the commit message but with this fix I stopped to observing mysterious pseudo abort build failures (at least so far).
So I'm curious about your experience with it. In other words the revert made here[1] didn't help for our cases.

[1] http://git.yoctoproject.org/cgit/cgit.cgi/pseudo/commit/?h=oe-core&id=b988b0a6b8afd8d459bc9a2528e834f63a3d59b2

--
Regards,
Damian

 > 
 > That said, I have no idea how the underlying bug escaped notice all
 > this time, it's definitely a bug. I think it is actually safe to just
 > make msg be static, because pseudo_client_op is protected by a lock
 > and is never executed more than once at a time.
 > 
 > On reflection: I think the way it worked is that in that case, the
 > actual message isn't looked at, just checked for nullness, but this
 > is still undefined behavior because the result is a pointer to storage
 > after the storage's lifetime, and formally you can't even check those
 > for "is or isn't null".
 > 
 > -s
 > 

Re: [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations

[Damian Wrobel]

 ---- On Tue, 27 Jul 2021 17:49:27 +0200 Seebs <seebs@seebs.net> wrote ----
 > On Tue, 27 Jul 2021 13:49:03 +0200
 > "Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:
 > 
 > > -# no-strict-aliasing is needed for the function pointer trickery.
 > >  pseudo_wrappers.o: $(GUTS) pseudo_wrappers.c pseudo_wrapfuncs.c
 > > pseudo_wrapfuncs.h pseudo_tables.h
 > > -    $(CC) -fno-strict-aliasing $(CFLAGS) $(CFLAGS_PSEUDO)
 > > -D_GNU_SOURCE -c -o pseudo_wrappers.o pseudo_wrappers.c
 > 
 > I would be really uncomfortable relying on that not being needed, even
 > if it happened to work in specific cases. The function pointer trickery
 > is black magic and very much undefined behavior, and warning the
 > compiler away from it seems reasonably likely to be advantageous.

Based on my experience, It's usually better to see this warning and fix the code instead of relying on the back magic.

--
Regards,
Damian

 > 
 > -s
 > 
 > 
 > 
 > 

Re: [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations

[Seebs]

On Tue, 27 Jul 2021 18:35:50 +0200
"Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:

> Based on my experience, It's usually better to see this warning and
> fix the code instead of relying on the back magic.

Yes, but in the case of the strict aliasing stuff and function pointer
stuff, we really, really, *can't* not rely on the magic, because pseudo
*is* the magic.

-s

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable

[Seebs]

On Tue, 27 Jul 2021 18:30:33 +0200
Damian Wrobel <dwrobel@ertelnet.rybnik.pl> wrote:

> The returned pointer has to be freed by the caller not by the callee
> function itself.

So, this predates the public release, but long ago, that was indeed
how it worked, and then LONG ago it was changed so that the pseudo_ipc
stuff always used the same object for its returns, so we weren't doing
alloc/free cycles all the time.

Which means that, in every *other* code path, if we return a non-nil
msg, it *must not* be freed.

I think probably the solution is to change that object to be static.
We can't make callers free the results unless we want them ALL to be
freed, which we absolutely don't, that's devastatingly expensive.

There is exactly one call with OP_CHROOT, and all it checks is whether
the return is null or not-null. I'd be mildly surprised by the
theoretically-invalid address of stack garbage actually causing a
problem on most modern systems, except that I think some systems have
started doing stack guards. But all we care about here is that the
address returned be a valid non-null pointer. Heck, we could use
&xattrdb_data, that already exists, is already static, and we don't
care about it.

(The reason the `msg` in that function isn't static is so it gets its
initializer every time. This is not a great reason.)

-s

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable

[Damian Wrobel]

 ---- On Tue, 27 Jul 2021 18:52:46 +0200 Seebs <seebs@seebs.net> wrote ----
 > On Tue, 27 Jul 2021 18:30:33 +0200
 > Damian Wrobel <dwrobel@ertelnet.rybnik.pl> wrote:
 > 
 > > The returned pointer has to be freed by the caller not by the callee
 > > function itself.
 > 
 > So, this predates the public release, but long ago, that was indeed
 > how it worked, and then LONG ago it was changed so that the pseudo_ipc
 > stuff always used the same object for its returns, so we weren't doing
 > alloc/free cycles all the time.
 > 
 > Which means that, in every *other* code path, if we return a non-nil
 > msg, it *must not* be freed.
 > 
 > I think probably the solution is to change that object to be static.

That was already static before this revert[1] and as you could read from the git message
it was causing problems as well.

If above static version didn't work then very likely the following similar pattern of returning static pointer:

pseudo_client_op()
  pseudo_client_request()
    pseudo_msg_receive() {
      newmsg = pseudo_msg_new()
      free(incomming) // static pseudo_msg_t *incoming
      incomming = newmsg
      read(incomming)
      return incomming
    }

is causing the same sort of problems.

I'm seeing this code for the first time so the question is.

Do I correctly assume that pseudo_client_op() has to be fully reentrant?

If so, then under any circumstances any path shouldn't return a pointer to a static variable which
in fact is not const static.

[1] http://git.yoctoproject.org/cgit/cgit.cgi/pseudo/commit/?h=oe-core&id=b988b0a6b8afd8d459bc9a2528e834f63a3d59b2

--
Regards,
Damian

 > We can't make callers free the results unless we want them ALL to be
 > freed, which we absolutely don't, that's devastatingly expensive.
 > 
 > There is exactly one call with OP_CHROOT, and all it checks is whether
 > the return is null or not-null. I'd be mildly surprised by the
 > theoretically-invalid address of stack garbage actually causing a
 > problem on most modern systems, except that I think some systems have
 > started doing stack guards. But all we care about here is that the
 > address returned be a valid non-null pointer. Heck, we could use
 > &xattrdb_data, that already exists, is already static, and we don't
 > care about it.
 > 
 > (The reason the `msg` in that function isn't static is so it gets its
 > initializer every time. This is not a great reason.)
 > 
 > -s
 > 
 > 
 > 
 > 

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable

[Seebs]

On Wed, 28 Jul 2021 11:36:22 +0200
"Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:

> Do I correctly assume that pseudo_client_op() has to be fully
> reentrant?

No. It's never been even a tiny bit reentrant. We used to do the
allocate and free thing, and it was incredibly expensive, and the
nature of the thing requires confidence that we never, ever, have
more than one thing writing and reading over the socket at a time,
so it's just Not Reentrant. During one call to pseudo_client_op,
there will never be another, and all the IPC stuff uses a single
consistent local buffer that it returns the address of.

Declaring that as static without changing the initializer would indeed
break everything -- we rely on the initializer working. Changing it to
static means it only gets initialized once...

Changing it to:

	static pseudo_msg_t msg;
	msg = pseudo_msg_t { .type = PSEUDO_MSG_OP };

would probably be fine, because then it'd be initialized. Otherwise,
we'd get failures when msg got overwritten and reused.

Or just changing `result = &msg` to something like `result =
&xattrdb_data`, which would be nonsensical but it turns out not to
matter, as the only caller that reaches this case is the caller
that's just checking yes/no "is the return value not a null pointer".

-s

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable

[Andre McCurdy]

On Wed, Jul 28, 2021 at 1:16 PM Seebs <seebs@seebs.net> wrote:
>
> On Wed, 28 Jul 2021 11:36:22 +0200
> "Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:
>
> > Do I correctly assume that pseudo_client_op() has to be fully
> > reentrant?
>
> No. It's never been even a tiny bit reentrant. We used to do the
> allocate and free thing, and it was incredibly expensive, and the
> nature of the thing requires confidence that we never, ever, have
> more than one thing writing and reading over the socket at a time,
> so it's just Not Reentrant. During one call to pseudo_client_op,
> there will never be another, and all the IPC stuff uses a single
> consistent local buffer that it returns the address of.
>
> Declaring that as static without changing the initializer would indeed
> break everything -- we rely on the initializer working. Changing it to
> static means it only gets initialized once...
>
> Changing it to:
>
>         static pseudo_msg_t msg;
>         msg = pseudo_msg_t { .type = PSEUDO_MSG_OP };
>
> would probably be fine, because then it'd be initialized. Otherwise,
> we'd get failures when msg got overwritten and reused.
>
> Or just changing `result = &msg` to something like `result =
> &xattrdb_data`, which would be nonsensical but it turns out not to
> matter, as the only caller that reaches this case is the caller
> that's just checking yes/no "is the return value not a null pointer".

If the caller only cares about yes/no then how about returning 1/0
instead of a pointer?

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable

[Seebs]

On Wed, 28 Jul 2021 13:49:17 -0700
Andre McCurdy <armccurdy@gmail.com> wrote:

> If the caller only cares about yes/no then how about returning 1/0
> instead of a pointer?

Other callers are actually using the response, because the pseudo_msg_t*
returned from this is how, say, OP_STAT responds with the stat
information it got from the server. It's just chroot, specifically, that
wants to know that the client accepted-and-processed the chroot, but
there's no meaningful additional information past that.

I suppose in theory pseudo_client_op could be split into one function
for ops with message returns and another for ops with only
success/failure, or "just ignored", but it doesn't seem rewarding.

In Go, I'd probably have done it as
	func (c *PseudoClient) Op(whatever goes here) (*Message, error)
and this would just be returning (nil, nil) or (nil,
some-meaningful-error) in the chroot case, but here we are in C with
single return values. :)

-s

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified

[Damian Wrobel]

 ---- On Wed, 28 Jul 2021 22:16:41 +0200 Seebs <seebs@seebs.net> wrote ----
 > On Wed, 28 Jul 2021 11:36:22 +0200
 > "Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:
 > 
 > > Do I correctly assume that pseudo_client_op() has to be fully
 > > reentrant?
 > 
 > No. It's never been even a tiny bit reentrant. We used to do the
 > allocate and free thing, and it was incredibly expensive, and the
 > nature of the thing requires confidence that we never, ever, have
 > more than one thing writing and reading over the socket at a time,
 > so it's just Not Reentrant. During one call to pseudo_client_op,

 > there will never be another, and all the IPC stuff uses a single
 > consistent local buffer that it returns the address of.
 > 
 > Declaring that as static without changing the initializer would indeed
 > break everything -- we rely on the initializer working. Changing it to
 > static means it only gets initialized once...
 > 
 > Changing it to:
 > 
 >     static pseudo_msg_t msg;
 >     msg = pseudo_msg_t { .type = PSEUDO_MSG_OP };
 > 
 > would probably be fine, because then it'd be initialized. Otherwise,
 > we'd get failures when msg got overwritten and reused.

Looking at the code I see that:

$ grep -n msg pseudo_client.c | grep -A 1 ".type = PSEUDO_MSG_OP"
1592:	pseudo_msg_t msg = { .type = PSEUDO_MSG_OP };
1848:		pseudo_msg_stat(&msg, buf);
1875:	msg.type = PSEUDO_MSG_OP;
1876:	msg.op = op;

the PSEUDO_MSG_OP is being unconditionally assigned to the msg.type
before any real usage of the 'msg' structure. So, if I'm not mistaken that code was
already tested and didn't work well and was reverted here[1].

[1] http://git.yoctoproject.org/cgit/cgit.cgi/pseudo/commit/?h=oe-core&id=b988b0a6b8afd8d459bc9a2528e834f63a3d59b2

--
Regards,
Damian

 > 
 > Or just changing `result = &msg` to something like `result =
 > &xattrdb_data`, which would be nonsensical but it turns out not to
 > matter, as the only caller that reaches this case is the caller
 > that's just checking yes/no "is the return value not a null pointer".
 > 
 > -s
 > 
 > 
 > 
 > 

Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified

[Seebs]

On Thu, 29 Jul 2021 14:37:28 +0200
"Damian Wrobel" <dwrobel@ertelnet.rybnik.pl> wrote:

> the PSEUDO_MSG_OP is being unconditionally assigned to the msg.type
> before any real usage of the 'msg' structure. So, if I'm not mistaken
> that code was already tested and didn't work well and was reverted
> here[1].

I don't think that's identical.

	msg = (pseudo_msg_t) { .type = PSEUDO_MSG_OP };

zeros out a number of other fields.

Anyway, if it fails with msg declared static, I'd be interested in
a minimal reproducer for it, I don't see anything obvious that ought to
care about it, but I would easily believe that the other fields not
being zeroed out would cause weirdness, which is why I suggested a
complete assignment to it, not just overwriting the .type field.

-s