* [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations @ 2021-07-27 11:49 Damian Wrobel 2021-07-27 11:49 ` [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag() Damian Wrobel ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Damian Wrobel @ 2021-07-27 11:49 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag() 2021-07-27 11:49 [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Damian Wrobel @ 2021-07-27 11:49 ` Damian Wrobel 2021-07-27 15:48 ` [OE-core] " Seebs 2021-07-27 11:49 ` [PATCH pseudo 3/4] Use -pthread instead of -lpthread Damian Wrobel ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Damian Wrobel @ 2021-07-27 11:49 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag() 2021-07-27 11:49 ` [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag() Damian Wrobel @ 2021-07-27 15:48 ` Seebs 0 siblings, 0 replies; 17+ messages in thread From: Seebs @ 2021-07-27 15:48 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH pseudo 3/4] Use -pthread instead of -lpthread 2021-07-27 11:49 [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Damian Wrobel 2021-07-27 11:49 ` [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag() Damian Wrobel @ 2021-07-27 11:49 ` Damian Wrobel 2021-07-27 11:49 ` [PATCH pseudo 4/4] Do not return address of local variable Damian Wrobel 2021-07-27 15:49 ` [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Seebs 3 siblings, 0 replies; 17+ messages in thread From: Damian Wrobel @ 2021-07-27 11:49 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH pseudo 4/4] Do not return address of local variable 2021-07-27 11:49 [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Damian Wrobel 2021-07-27 11:49 ` [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag() Damian Wrobel 2021-07-27 11:49 ` [PATCH pseudo 3/4] Use -pthread instead of -lpthread Damian Wrobel @ 2021-07-27 11:49 ` Damian Wrobel 2021-07-27 15:47 ` [OE-core] " Seebs 2021-07-27 15:49 ` [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Seebs 3 siblings, 1 reply; 17+ messages in thread From: Damian Wrobel @ 2021-07-27 11:49 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable 2021-07-27 11:49 ` [PATCH pseudo 4/4] Do not return address of local variable Damian Wrobel @ 2021-07-27 15:47 ` Seebs 2021-07-27 16:30 ` Damian Wrobel 0 siblings, 1 reply; 17+ messages in thread From: Seebs @ 2021-07-27 15:47 UTC (permalink / raw) To: Damian Wrobel; +Cc: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable 2021-07-27 15:47 ` [OE-core] " Seebs @ 2021-07-27 16:30 ` Damian Wrobel 2021-07-27 16:52 ` Seebs 0 siblings, 1 reply; 17+ messages in thread From: Damian Wrobel @ 2021-07-27 16:30 UTC (permalink / raw) To: Seebs; +Cc: openembedded-core ---- 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable 2021-07-27 16:30 ` Damian Wrobel @ 2021-07-27 16:52 ` Seebs 2021-07-28 9:36 ` Damian Wrobel 0 siblings, 1 reply; 17+ messages in thread From: Seebs @ 2021-07-27 16:52 UTC (permalink / raw) To: Damian Wrobel; +Cc: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable 2021-07-27 16:52 ` Seebs @ 2021-07-28 9:36 ` Damian Wrobel 2021-07-28 20:16 ` Seebs 0 siblings, 1 reply; 17+ messages in thread From: Damian Wrobel @ 2021-07-28 9:36 UTC (permalink / raw) To: Seebs; +Cc: openembedded-core ---- 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 > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable 2021-07-28 9:36 ` Damian Wrobel @ 2021-07-28 20:16 ` Seebs 2021-07-28 20:49 ` Andre McCurdy 2021-07-29 12:37 ` [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified Damian Wrobel 0 siblings, 2 replies; 17+ messages in thread From: Seebs @ 2021-07-28 20:16 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable 2021-07-28 20:16 ` Seebs @ 2021-07-28 20:49 ` Andre McCurdy 2021-07-28 20:53 ` Seebs 2021-07-29 12:37 ` [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified Damian Wrobel 1 sibling, 1 reply; 17+ messages in thread From: Andre McCurdy @ 2021-07-28 20:49 UTC (permalink / raw) To: Seebs; +Cc: openembedded-core 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable 2021-07-28 20:49 ` Andre McCurdy @ 2021-07-28 20:53 ` Seebs 0 siblings, 0 replies; 17+ messages in thread From: Seebs @ 2021-07-28 20:53 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified 2021-07-28 20:16 ` Seebs 2021-07-28 20:49 ` Andre McCurdy @ 2021-07-29 12:37 ` Damian Wrobel 2021-07-29 15:08 ` Seebs 1 sibling, 1 reply; 17+ messages in thread From: Damian Wrobel @ 2021-07-29 12:37 UTC (permalink / raw) To: Seebs; +Cc: openembedded-core ---- 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 > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified 2021-07-29 12:37 ` [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified Damian Wrobel @ 2021-07-29 15:08 ` Seebs 0 siblings, 0 replies; 17+ messages in thread From: Seebs @ 2021-07-29 15:08 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations 2021-07-27 11:49 [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Damian Wrobel ` (2 preceding siblings ...) 2021-07-27 11:49 ` [PATCH pseudo 4/4] Do not return address of local variable Damian Wrobel @ 2021-07-27 15:49 ` Seebs 2021-07-27 16:35 ` Damian Wrobel 3 siblings, 1 reply; 17+ messages in thread From: Seebs @ 2021-07-27 15:49 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations 2021-07-27 15:49 ` [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Seebs @ 2021-07-27 16:35 ` Damian Wrobel 2021-07-27 16:46 ` Seebs 0 siblings, 1 reply; 17+ messages in thread From: Damian Wrobel @ 2021-07-27 16:35 UTC (permalink / raw) To: Seebs; +Cc: openembedded-core ---- 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 > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations 2021-07-27 16:35 ` Damian Wrobel @ 2021-07-27 16:46 ` Seebs 0 siblings, 0 replies; 17+ messages in thread From: Seebs @ 2021-07-27 16:46 UTC (permalink / raw) To: openembedded-core 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-07-29 15:08 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-27 11:49 [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Damian Wrobel 2021-07-27 11:49 ` [PATCH pseudo 2/4] Do not pass null argument to pseudo_diag() Damian Wrobel 2021-07-27 15:48 ` [OE-core] " Seebs 2021-07-27 11:49 ` [PATCH pseudo 3/4] Use -pthread instead of -lpthread Damian Wrobel 2021-07-27 11:49 ` [PATCH pseudo 4/4] Do not return address of local variable Damian Wrobel 2021-07-27 15:47 ` [OE-core] " Seebs 2021-07-27 16:30 ` Damian Wrobel 2021-07-27 16:52 ` Seebs 2021-07-28 9:36 ` Damian Wrobel 2021-07-28 20:16 ` Seebs 2021-07-28 20:49 ` Andre McCurdy 2021-07-28 20:53 ` Seebs 2021-07-29 12:37 ` [OE-core] [PATCH pseudo 4/4] Do not return address of local variable - unverified Damian Wrobel 2021-07-29 15:08 ` Seebs 2021-07-27 15:49 ` [OE-core] [PATCH pseudo 1/4] Remove -fno-strict-aliasing and -Wno-deprecated-declarations Seebs 2021-07-27 16:35 ` Damian Wrobel 2021-07-27 16:46 ` Seebs
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.