public inbox for cygwin-apps@cygwin.com
 help / color / mirror / Atom feed
* [PATCH setup 1/3] Fix the reading and writing of the "extrakeys" user setting
  2017-11-28 14:56 [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
  2017-11-28 14:56 ` [PATCH setup 2/3] Change the interpretation of '#' in setup.rc Ken Brown
  2017-11-28 14:56 ` [PATCH setup 3/3] Remove references to "last-extrakeys" Ken Brown
@ 2017-11-28 14:56 ` Ken Brown
  2017-11-28 15:28 ` [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
  2017-11-30 12:53 ` Jon Turney
  4 siblings, 0 replies; 6+ messages in thread
From: Ken Brown @ 2017-11-28 14:56 UTC (permalink / raw)
  To: cygwin-apps

ExtraKeysSetting::keybuffer is terminated by LF rather than NUL.  So
we have to replace NUL by LF after calling
UserSettings::get("extrakeys") in the ExtraKeysSetting constructor.
Otherwise the last saved key is discarded.  Also, bufsize has to be
set appropriately before the call to count_keys(), or else all saved
keys are discarded.

Similarly, the final LF in keybuffer has to be replaced by NUL in the
ExtraKeysSetting destructor before the call to
UserSettings::set("extrakeys", keybuffer).  Otherwise we get garbage
at the end of the "extrakeys" setting in setup.rc.
---
 KeysSetting.cc | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/KeysSetting.cc b/KeysSetting.cc
index a21c3c4..ec8e4f9 100644
--- a/KeysSetting.cc
+++ b/KeysSetting.cc
@@ -36,7 +36,10 @@ ExtraKeysSetting::ExtraKeysSetting ():
   const char *p = UserSettings::instance().get ("extrakeys");
   if (p)
     {
+      bufsize = strlen (p) + 1;	// Include final NUL.
       keybuffer = strdup (p);
+      // Replace final NUL by LF.
+      keybuffer[bufsize - 1] = 0x0a;
       // Calling count_keys gets the count but also sizes the buffer
       // correctly, discarding any trailing non-LF-terminated data.
       bufsize = count_keys ();
@@ -46,7 +49,11 @@ ExtraKeysSetting::ExtraKeysSetting ():
 ExtraKeysSetting::~ExtraKeysSetting ()
 {
   if (keybuffer)
-    UserSettings::instance().set ("extrakeys", keybuffer);
+    {
+      // Replace final LF by NUL.
+      keybuffer[bufsize - 1] = '\0';
+      UserSettings::instance().set ("extrakeys", keybuffer);
+    }
 }
 
 void
-- 
2.15.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH setup 3/3] Remove references to "last-extrakeys"
  2017-11-28 14:56 [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
  2017-11-28 14:56 ` [PATCH setup 2/3] Change the interpretation of '#' in setup.rc Ken Brown
@ 2017-11-28 14:56 ` Ken Brown
  2017-11-28 14:56 ` [PATCH setup 1/3] Fix the reading and writing of the "extrakeys" user setting Ken Brown
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Ken Brown @ 2017-11-28 14:56 UTC (permalink / raw)
  To: cygwin-apps

Extra gpg keys used to be stored in a file /etc/setup/last-extrakeys.
These keys are now saved in the "extrakeys" user setting, but there
were still references to "last-extrakeys" in comments and in a help
string.
---
 KeysSetting.h | 2 +-
 crypto.cc     | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/KeysSetting.h b/KeysSetting.h
index 9cd0f9a..f7b9336 100644
--- a/KeysSetting.h
+++ b/KeysSetting.h
@@ -34,7 +34,7 @@ class ExtraKeysSetting
     size_t numkeys;
     static ExtraKeysSetting *global;
   public:
-    // Loads keys from last-extrakeys
+    // Loads keys from the "extrakeys" user setting.
     ExtraKeysSetting ();
     // Saves them back again.
     ~ExtraKeysSetting ();
diff --git a/crypto.cc b/crypto.cc
index a606283..5a10e16 100644
--- a/crypto.cc
+++ b/crypto.cc
@@ -48,7 +48,7 @@ static StringArrayOption SexprExtraKeyOption ('S', "sexpr-pubkey",
 			"Extra public key in s-expr format");
 
 static BoolOption UntrustedKeysOption (false, 'u', "untrusted-keys",
-			"Use untrusted keys from last-extrakeys");
+			"Use untrusted saved extra keys");
 static BoolOption KeepUntrustedKeysOption (false, 'U', "keep-untrusted-keys",
 			"Use untrusted keys and retain all");
 
@@ -466,9 +466,9 @@ verify_ini_file_sig (io_stream *ini_file, io_stream *ini_sig_file, HWND owner)
   msg ("key:%d\n'%s'", n, sexprbuf);
 #endif /* CRYPTODEBUGGING */
 
-  /* Next we should extract the keys from the last-extrakeys
-  file, and flush it; we'll only return them to it if they
-  get used.  OTOH, should we do this at all?  The extrakeys
+  /* Next we should extract the keys from the extrakeys user
+  setting, and flush it; we'll only return them to it if they
+  get used.  OTOH, should we do this at all?  The user settings
   file isn't heavily protected.  So we only trust the extra
   keys if we're told to by the user.  We still read them in
   and write them back out, which canonicalises and eliminates
-- 
2.15.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH setup 0/3] Fix "extrakeys" issues
@ 2017-11-28 14:56 Ken Brown
  2017-11-28 14:56 ` [PATCH setup 2/3] Change the interpretation of '#' in setup.rc Ken Brown
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Ken Brown @ 2017-11-28 14:56 UTC (permalink / raw)
  To: cygwin-apps

The "extrakeys" user setting contains saved extra gpg keys.  It was
introduced in 2009 to replace the use of a file
/etc/setup/last-extrakeys.  It has apparently never worked right.
Here are the issues I've found:

 - User settings are read and written as NUL-terminated strings, but
   extra keys are terminated by LF instead.  On writing, this causes
   garbage to be written into setup.rc.  On reading, this causes the
   final saved key to be discarded.

 - The ExtraKeysSetting constructor calls count_keys() without setting
   bufsize to a positive value.  This causes *all* saved keys to be
   discarded.

 - Saved keys contain several '#' characters.  These are treated as
   comment characters in setup.rc, so that all keys are truncated when
   read.

 - There are still references to the "last-extrakeys" file in comments
   and in a help string.

This patch series attempts to fix all these problems.  In the case of
'#', the fix is to treat '#' as a comment character only if it's the
first non-whitespace character on a line.  I don't think this will
cause problems for any existing uses of '#', but I haven't done a
thorough check of this yet.

Ken Brown (3):
  Fix the reading and writing of the "extrakeys" user setting
  Change the interpretation of '#' in setup.rc
  Remove references to "last-extrakeys"

 KeysSetting.cc  | 9 ++++++++-
 KeysSetting.h   | 2 +-
 UserSettings.cc | 8 ++++----
 crypto.cc       | 8 ++++----
 4 files changed, 17 insertions(+), 10 deletions(-)

-- 
2.15.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH setup 2/3] Change the interpretation of '#' in setup.rc
  2017-11-28 14:56 [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
@ 2017-11-28 14:56 ` Ken Brown
  2017-11-28 14:56 ` [PATCH setup 3/3] Remove references to "last-extrakeys" Ken Brown
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Ken Brown @ 2017-11-28 14:56 UTC (permalink / raw)
  To: cygwin-apps

'#' was treated as a comment character in all circumstances.  Since
saved gpg keys contain '#', this caused the "extrakeys" user setting
to get truncated.  Change this so that '#' only indicates a comment if
it's the first non-whitespace character in a line.
---
 UserSettings.cc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/UserSettings.cc b/UserSettings.cc
index f4917ec..b90d795 100644
--- a/UserSettings.cc
+++ b/UserSettings.cc
@@ -42,14 +42,14 @@ public:
 
 UserSettings *UserSettings::global;
 
+// '#' indicates a comment if it's the first non-whitespace character.
 static char *
 trim (char *p)
 {
   p += strspn (p, " \t");
-  char *q = strchr (p, '#');
-  if (q)
-    *q = '\0';
-  for (q = strchr (p, '\0') - 1; q >= p && (*q == ' ' || *q == '\t' || *q == '\r' || *q == '\n'); q--)
+  if (*p == '#')
+    *p = '\0';
+  for (char *q = strchr (p, '\0') - 1; q >= p && (*q == ' ' || *q == '\t' || *q == '\r' || *q == '\n'); q--)
     *q = '\0';
   return p;
 }
-- 
2.15.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH setup 0/3] Fix "extrakeys" issues
  2017-11-28 14:56 [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
                   ` (2 preceding siblings ...)
  2017-11-28 14:56 ` [PATCH setup 1/3] Fix the reading and writing of the "extrakeys" user setting Ken Brown
@ 2017-11-28 15:28 ` Ken Brown
  2017-11-30 12:53 ` Jon Turney
  4 siblings, 0 replies; 6+ messages in thread
From: Ken Brown @ 2017-11-28 15:28 UTC (permalink / raw)
  To: cygwin-apps

On 11/28/2017 9:56 AM, Ken Brown wrote:
> The "extrakeys" user setting contains saved extra gpg keys.  It was
> introduced in 2009 to replace the use of a file
> /etc/setup/last-extrakeys.  It has apparently never worked right.
> Here are the issues I've found:
> 
>   - User settings are read and written as NUL-terminated strings, but
>     extra keys are terminated by LF instead.  On writing, this causes
>     garbage to be written into setup.rc.  On reading, this causes the
>     final saved key to be discarded.
> 
>   - The ExtraKeysSetting constructor calls count_keys() without setting
>     bufsize to a positive value.  This causes *all* saved keys to be
>     discarded.
> 
>   - Saved keys contain several '#' characters.  These are treated as
>     comment characters in setup.rc, so that all keys are truncated when
>     read.
> 
>   - There are still references to the "last-extrakeys" file in comments
>     and in a help string.
> 
> This patch series attempts to fix all these problems.  In the case of
> '#', the fix is to treat '#' as a comment character only if it's the
> first non-whitespace character on a line.  I don't think this will
> cause problems for any existing uses of '#', but I haven't done a
> thorough check of this yet.

I've now made a pretty complete search, and the only use of a comment in 
setup.rc I can find is the one in site.cc.  When reading and writing 
lists of URLs, it treats '#' as a comment only at the beginning of a 
line.  So I think my change is safe.

Ken

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH setup 0/3] Fix "extrakeys" issues
  2017-11-28 14:56 [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
                   ` (3 preceding siblings ...)
  2017-11-28 15:28 ` [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
@ 2017-11-30 12:53 ` Jon Turney
  4 siblings, 0 replies; 6+ messages in thread
From: Jon Turney @ 2017-11-30 12:53 UTC (permalink / raw)
  To: cygwin-apps

On 28/11/2017 14:56, Ken Brown wrote:
> The "extrakeys" user setting contains saved extra gpg keys.  It was
> introduced in 2009 to replace the use of a file
> /etc/setup/last-extrakeys.  It has apparently never worked right.
> Here are the issues I've found:
> 
>   - User settings are read and written as NUL-terminated strings, but
>     extra keys are terminated by LF instead.  On writing, this causes
>     garbage to be written into setup.rc.  On reading, this causes the
>     final saved key to be discarded.
> 
>   - The ExtraKeysSetting constructor calls count_keys() without setting
>     bufsize to a positive value.  This causes *all* saved keys to be
>     discarded.
> 
>   - Saved keys contain several '#' characters.  These are treated as
>     comment characters in setup.rc, so that all keys are truncated when
>     read.
> 
>   - There are still references to the "last-extrakeys" file in comments
>     and in a help string.
> 
> This patch series attempts to fix all these problems.  In the case of
> '#', the fix is to treat '#' as a comment character only if it's the
> first non-whitespace character on a line.  I don't think this will
> cause problems for any existing uses of '#', but I haven't done a
> thorough check of this yet.

Comments in setup.rc seem to have very little value (since they will be 
destroyed the next time setup writes it)

> 
> Ken Brown (3):
>    Fix the reading and writing of the "extrakeys" user setting
>    Change the interpretation of '#' in setup.rc
>    Remove references to "last-extrakeys"
> 
>   KeysSetting.cc  | 9 ++++++++-
>   KeysSetting.h   | 2 +-
>   UserSettings.cc | 8 ++++----
>   crypto.cc       | 8 ++++----
>   4 files changed, 17 insertions(+), 10 deletions(-)

Great. Please apply.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-11-30 12:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-28 14:56 [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
2017-11-28 14:56 ` [PATCH setup 2/3] Change the interpretation of '#' in setup.rc Ken Brown
2017-11-28 14:56 ` [PATCH setup 3/3] Remove references to "last-extrakeys" Ken Brown
2017-11-28 14:56 ` [PATCH setup 1/3] Fix the reading and writing of the "extrakeys" user setting Ken Brown
2017-11-28 15:28 ` [PATCH setup 0/3] Fix "extrakeys" issues Ken Brown
2017-11-30 12:53 ` Jon Turney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).