Re: [PATCH] Add support for high-entropy-va flag to peflags.
On Mon, 17 May 2021, Corinna Vinschen wrote:
> Hi Jeremy,
>
> Thanks for the patch, but I have two nits:
>
> - The patch doesn't apply cleanly with `git am'. Please check again.
Probably got mangled in the mail. Attached this time.
>
> - I would prefer a massively reduced patch size, by *not* changing
> indentation on otherwise unaffected lines.
>
DoneFrom 26e7d716b5ecc49cc2e8d5ab05a1586c089c75fe Mon Sep 17 00:00:00 2001
From: Jeremy Drake
Date: Sat, 15 May 2021 12:07:26 -0700
Subject: [PATCH] Add support for high-entropy-va flag to peflags.
This allows for setting, clearing, and displaying the value of the "high
entropy va" dll characteristics flag.
Signed-off-by: Jeremy Drake
---
peflags.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/peflags.c b/peflags.c
index 4d22e4a..bb333d7 100644
--- a/peflags.c
+++ b/peflags.c
@@ -112,7 +112,7 @@ static const symbolic_flags_t pe_symbolic_flags[] = {
/*CF(0x0004, reserved_0x0004),*/
/*CF(0x0008, reserved_0x0008),*/
/*CF(0x0010, unspec_0x0010),*/
-/*CF(0x0020, unspec_0x0020),*/
+ CF(0x0020, high-entropy-va),
CF(0x0040, dynamicbase),
CF(0x0080, forceinteg),
CF(0x0100, nxcompat),
@@ -181,6 +181,7 @@ sizeof_values_t sizeof_vals[5] = {
static struct option long_options[] = {
{"dynamicbase", optional_argument, NULL, 'd'},
+ {"high-entropy-va", optional_argument, NULL, 'e'},
{"forceinteg", optional_argument, NULL, 'f'},
{"nxcompat", optional_argument, NULL, 'n'},
{"no-isolation", optional_argument, NULL, 'i'},
@@ -203,7 +204,7 @@ static struct option long_options[] = {
{NULL, no_argument, NULL, 0}
};
static const char *short_options
- = "d::f::n::i::s::b::W::t::w::l::S::x::X::y::Y::z::T:vhV";
+ = "d::e::f::n::i::s::b::W::t::w::l::S::x::X::y::Y::z::T:vhV";
static void short_usage (FILE *f);
static void help (FILE *f);
@@ -699,6 +700,11 @@ parse_args (int argc, char *argv[])
optarg,
IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE);
break;
+ case 'e':
+ handle_pe_flag_option (long_options[option_index].name,
+optarg,
+IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA);
+ break;
case 'n':
handle_pe_flag_option (long_options[option_index].name,
optarg,
@@ -1069,6 +1075,9 @@ help (FILE *f)
"\n"
" -d, --dynamicbase [BOOL] Image base address may be relocated using\n"
" address space layout randomization (ASLR).\n"
+" -e,\n"
+" --high-entropy-va [BOOL] Image is compatible with 64-bit address space\n"
+" layout randomization (ASLR).\n"
" -f, --forceinteg [BOOL] Code integrity checks are enforced.\n"
" -n, --nxcompat [BOOL] Image is compatible with data execution\n"
" prevention (DEP).\n"
--
2.31.1.windows.1
Re: [PATCH v6] Cygwin: rewrite cmdline parser
On May 14 21:38, Johannes Schindelin wrote: > Hi, > > first of all: thank you for working on this. I have run afoul of the > (de-)quoting differences between MSVCRT and Cygwin on more than one > occasion. > [...] > > * MSVCRT compatibility. Except for the single-quote handling (an > > extension for compatibility with old Cygwin), the parser now > > interprets option boundaries exactly like MSVCR since 2008. This fixes > > the issue where our escaping does not work with our own parsing. > > When I read this, I immediately think: This is probably going to break > backwards-compatibility, OMG this is making my life so much harder than it > already is. > [...] > I ask because as maintainer of Git for Windows (which bundles an MSYS2 > runtime which is based on the Cygwin runtime), it is my responsibility to > take care of backwards-compatibility on behalf of the millions of users > out there. Did you try it? AFAIU, the patch actually fixes up a Cygwin weirdness, which already results in broken behaviour, rather than breaking backward compat. IIRC we discussed this already a while back, you should find it in the archives. > > * A memory leak in the @file expansion is removed by rewriting it to use > > a stack of buffers. This also simplifies the code since we no longer > > have to move stuff. The "downside" is that tokens can no longer cross > > file boundaries. > > This bullet point sounds as if it cries out loud to be put into a separate > patch, accompanied by the corresponding refactored part of the diff. > I would like to encourage you to disentangle these separate concerns: > > - moving code (`git diff --color-moved` should tell the reader that > nothing was edited) > > - clarifying documentation > > - removing GLOB_NOCHECK > > - introducing an MSVCRT-compatible mode (and make it opt-in!) What? No. There's no point in doing that. We want a single way of handling the CLI when called natively. > - whatever else I missed in the 304 deleted and 367 inserted lines (which > is a tough read, and I have to admit that my attention faded after about > a sixth of the patch) > > In essence, pretend that you are a reviewer who wants to help by ensuring > that this patch (series) does not break anything, and that it does > everything as intended (i.e. no subtle bugs are lurking in there). Now, > how would you like the series to be presented (and I keep referring to it > as a _series_ because that's what it should be, for readability). Ideally > it would be a series of patches that tell an interesting story, in a > manner of speaking. I concur that it would be rather nice to see this patch converted into a series with patches handling one problem at a time. Thanks, Corinna
Re: [PATCH] Add support for high-entropy-va flag to peflags.
Hi Jeremy,
On May 15 12:17, Jeremy Drake via Cygwin-patches wrote:
> This allows for setting, clearing, and displaying the value of the "high
> entropy va" dll characteristics flag.
>
> Signed-off-by: Jeremy Drake
> ---
> I'm not sure this is the right list for this...
It's not, but given the low traffic, never mind that for this patch.
Thanks for the patch, but I have two nits:
- The patch doesn't apply cleanly with `git am'. Please check again.
- I would prefer a massively reduced patch size, by *not* changing
indentation on otherwise unaffected lines.
I.e., ignore the problem here:
{"dynamicbase", optional_argument, NULL, 'd'},
+ {"high-entropy-va", optional_argument, NULL, 'e'},
...and just add
" -d, --dynamicbase [BOOL] Image base address may be relocated using\n"
+" -e,
+ --high-entropy-va [BOOL] Image is compatible with 64-bit address
space\n"
+" layout randomization (ASLR).\n"
With that change I'll apply it and release a new version of rebase soon.
Oh and, btw., there's some problem with DMARC munging with your email
address. Would you mind to attach the git patch instead of inlining
it? I won't accidentally forget to fix your email address in the log
message then :-P
Thanks,
Corinna
