[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAH9TF6OQAYx4Dc-BV-kebj88HC+PRwburP8Ma7q6xVyHhyiMGQ@mail.gmail.com>
Date: Mon, 9 Dec 2024 14:55:40 +0100
From: Alex Rønne Petersen <alex@...xrp.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] clone: clear the frame pointer in the child
process on relevant ports
On Mon, Dec 9, 2024 at 2:07 PM Rich Felker <dalias@...c.org> wrote:
>
> On Mon, Dec 09, 2024 at 02:04:11PM +0100, Alex Rønne Petersen wrote:
> > On Mon, Dec 9, 2024 at 1:30 PM Rich Felker <dalias@...c.org> wrote:
> > >
> > > On Sat, Dec 07, 2024 at 07:47:38PM +0100, Alex Rønne Petersen wrote:
> > > > On Sat, Dec 7, 2024 at 6:04 AM Rich Felker <dalias@...c.org> wrote:
> > > > >
> > > > > On Fri, Dec 06, 2024 at 07:48:53PM +0100, Alex Rønne Petersen wrote:
> > > > > > This just mirrors what is done in the start code for the affected ports, as well
> > > > > > as what is already done for the three x86 ports. For consistency, I also changed
> > > > > > the x86 ports and the powerpc port to have the child process portion at the end
> > > > > > of clone().
> > > > >
> > > > > Can you submit without this independent change? Readers of history
> > > > > should be able to confirm that the patch does not make any other
> > > > > functional change, and it's hard to do that when reorganizing code is
> > > > > mixed with the change.
> > > >
> > > > Can do. Should I send it as a separate patch or would you rather I
> > > > just omit that particular change?
> > >
> > > My leaning would be omit. For changes like this, my heuristic is to
> > > ask (1) whether, if I were a user, I would be angry if a regression
> > > somehow got introduced through the change (which offered nothing of
> > > value to me as a user in return for the risk), and (2) whether, as a
> > > reviewer reading git log looking for accidentally or intentionally
> > > introduced bugs, I'd appreciate spending my time reading the change.
> >
> > Just for some background: The reason I included the change was that in
> > the equivalent Zig standard library code, we did exactly this because
> > we needed to add DWARF CFI annotations (.cfi_undefined) to the child
> > process portion of clone(). If the child process portion isn't moved
> > last, then the CFI annotations end up covering parent process code
> > that they shouldn't. My thinking was that it might make sense to
> > similarly rearrange the code in musl just in case CFI annotations ever
> > need to be added there.
>
> Thanks for explaning the motivation. I think it would make sense to do
> that change with adding CFI if the latter becomes something we want to
> do at some point (it might be wanted just as debug info if nothing
> else?) but not unless/until that.
It would also be for unwind protection; it just protects DWARF-based
unwinders, whereas physically clearing the frame pointer protects
FP-based unwinders. For example, on x86 it'd be .cfi undefined eip, on
aarch64 it'd be .cfi_undefined x30, etc. If there's appetite for this
in musl, I'd be happy to send a separate patch. (FWIW, glibc does this
as well.)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
