All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Damian Wrobel" <dwrobel@ertelnet.rybnik.pl>
To: "Seebs" <seebs@seebs.net>
Cc: "openembedded-core" <openembedded-core@lists.openembedded.org>
Subject: Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable
Date: Wed, 28 Jul 2021 11:36:22 +0200	[thread overview]
Message-ID: <17aec78de72.dbb79e7b5562.7908341287383433208@ertelnet.rybnik.pl> (raw)
In-Reply-To: <20210727115246.04b8ffa4@seebsdell>




 ---- 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
 > 
 > 
 > 
 > 

  reply	other threads:[~2021-07-28  9:36 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=17aec78de72.dbb79e7b5562.7908341287383433208@ertelnet.rybnik.pl \
    --to=dwrobel@ertelnet.rybnik.pl \
    --cc=openembedded-core@lists.openembedded.org \
    --cc=seebs@seebs.net \
    --subject='Re: [OE-core] [PATCH pseudo 4/4] Do not return address of local variable' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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.