Merge lp:~csurbhi/ubuntu/natty/plymouth/plymouth.bug-fix566818 into lp:ubuntu/natty/plymouth

[Surbhi Palande]

At boot the user passsword for a crypted device is captured by plymouth when it is available. By default the "details" plugin is used in text based logins. On every key stroke, the "details" plugin in plymouth clears only the current line before overwriting it with the password prompt. If the prompt is multilined then the last line ends up being overwritten but the previous lines are repeated. Reprinting only the last line of the password prompt string on every key press.

Tested on kvm.

Please consider this change for natty.

[Surbhi Palande (csurbhi)]

Following are some details of the patch:
1) The patch prints the entire prompt the first time (i.e when number of bullets to be printed is zero). It prints only the last line when the user presses a key (i.e bullets > 0)

2) The patch removes the independent printing of ':' as this is included in the prompt. No other plugin prints the ':' separately. So it makes sense to remove it from here and add it to the default prompt or expect it in the user defined prompt (just like in other plugins)

[Steve Langasek (vorlon)]

Hi Surbhi,

+ * Returns the position of the last newline character.
+ * If the newline character is at the end of the string,
+ * it returns the previous newline character in the string
+ * if any.

Why backing up to the previous newline character? This seems to just result in additional special handling down the line when write_on_views() is called, and is inconsistent anyway with the current handling as it means the prompt will lose the final newline, and the bullets will be drawn on the same line as the prompt instead of on the line after it. (Do you have examples of such a prompt today that ends in a newline?) It wouldn't be good form to end the prompt with a newline, but I don't think the details plugin is the place to correct for this.

+ write_on_views (plugin,
+ prompt,
+ prompt[len-1] == '\n' ? len -1 : len);
+ } else
+ write_on_views (plugin,
+ prompt,
+ strlen (prompt));
+ }

If final newlines don't need special treatment (and I think they shouldn't), then we can reduce the code duplication here as well.

   else
     write_on_views (plugin,
- "Password",
- strlen ("Password"));
-
- write_on_views (plugin, ":", strlen (":"));
+ "Password:",
+ strlen ("Password:"));

This is a fix for the separate issue of a doubled ':' in the prompt, right? It would be best if this were called out separately - perhaps in a dep3 header in debian/patches/ubuntu_details.patch that explains what the patch is for. (No need to split this as a separate commit or go into great detail in the changelog, but it should be documented somewhere so that it's clear this isn't an accidental change.)

review: Needs Fixing

[Surbhi Palande (csurbhi)]

Hi Steve,

Thanks a lot for reviewing the code. Here is what I thought:

1) If the prompt accidentally ends in a "\n" (which ideally it should not and I dont have an example thankfully to show this :), but if it does anyway), then the repetition of the prompt wont be fixed. Every other plugin can handle the case where there is a "\n" at the end except for the details plugin. So not printing the last character -'\n' in this special (probably a never should happen) is to prevent the case where a prompt does not appear and bullets get repeated. This will be very difficult for someone to debug - very non intuitive:
Eg: prompt: "Enter \n passphrase \n"

What gets printed is:
Enter
passphrase:
* (on next keystroke this is then cleared and \n is printed)
** (on next keystroke this is then cleared and \n is printed)
***
and so on!

Removing the last '\n' would rather print:
Enter
passphrase: ***

Please do let me know what you think!
I will make a separate patch of the ':'. Indeed that will be better to comprehend.

[Steve Langasek (vorlon)]

On Tue, Apr 19, 2011 at 08:08:28AM -0000, Surbhi Palande wrote:

> Thanks a lot for reviewing the code. Here is what I thought:

> 1) If the prompt accidentally ends in a "\n" (which ideally it should not
> and I dont have an example thankfully to show this :), but if it does
> anyway), then the repetition of the prompt wont be fixed. Every other
> plugin can handle the case where there is a "\n" at the end except for the
> details plugin. So not printing the last character -'\n' in this special
> (probably a never should happen) is to prevent the case where a prompt
> does not appear and bullets get repeated. This will be very difficult for
> someone to debug - very non intuitive: Eg: prompt: "Enter \n passphrase
> \n"

> What gets printed is:
> Enter
> passphrase:
> * (on next keystroke this is then cleared and \n is printed)
> ** (on next keystroke this is then cleared and \n is printed)
> ***
> and so on!

> Removing the last '\n' would rather print:
> Enter
> passphrase: ***

> Please do let me know what you think!

My understanding of the code is that the prompt is only shortened when
bullets > 0. So since we start out with 0 bullets, first you would have
this:

Enter
passphrase
_

Then because we're printing the part of the prompt between the
second-to-last and last newlines, typing a letter would refresh this to:

Enter
passphrase
passphrase *_

If we always cut everything up to and including the last \n, we instead get
an empty prompt when redrawing. And that's ok; given a confusingly
newline-terminated prompt, that's still probably better, as it gives us
this:

Enter
passphrase
***_

So I stand by my recommendation to not special-case a prompt that ends in a
newline.

> I will make a separate patch of the ':'. Indeed that will be better to
> comprehend.

Ok!

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[Surbhi Palande (csurbhi)]

Yes, if we keep the last_nl pointing to the second to last '\n' as it does right now then yes, thats correct: the second line will be repeated over and over again - but then that has a simple fix too - by changing the prompt without the last '\n'.

Ok! I will rewrite the patch and upload here! Thanks!

[Surbhi Palande (csurbhi)]

So: the shorten_prompt() needs to be kept the way it is right now: by returning the second to the last '\n' in case the '\n' is the last character - to get a non empty prompt. I was assuming that needed to be changed to return the last '\n' (in which case we will get an empty string and a '\n' for every key stroke). Roughly speaking, I will change the part:
write_on_views (plugin,
+ prompt,
+ prompt[len-1] == '\n' ? len -1 : len);

to

write_on_views (plugin,
+ prompt,
+ len);

[Surbhi Palande (csurbhi)]

Hi Steve,

I have pushed the changes requested. Please do have a look! Thanks!

Regards,
Surbhi.

[Steve Langasek (vorlon)]

@@ -363,16 +384,16 @@
                     strlen (CLEAR_LINE_SEQUENCE));
   plugin->state = PLY_BOOT_SPLASH_DISPLAY_PASSWORD_ENTRY;

- if (prompt)
+ if (prompt) {
+ shorten_prompt(&prompt);
     write_on_views (plugin,
                     prompt,
                     strlen (prompt));
+ }
   else

This seems to drop the special handling of the bullets == 0 case. Sorry if my comments were unclear, I do believe that we still need to handle that case specially. Testing with the patch as-is, I never see the first line of the cryptsetup prompt.

review: Needs Fixing

[lp:~csurbhi/ubuntu/natty/plymouth/plymouth.bug-fix566818]

1392. By Surbhi Palande on 2011-04-21

* details/plugin.c: On every key stroke, the "details" plugin in plymouth
  clears only the current line before overwriting it with the password
  prompt. If the prompt is multilined then the last line ends up being
  overwritten but the previous lines are repeated. Re-printing only the
  last line of the password prompt on every keystroke. (LP:#566818)
* details/plugin.c: Removed the explicit printing of ':'. Expected to be a
  part of the prompt.

[Surbhi Palande (csurbhi)]

Steve, I have added a tested patch now. Sorry for the re-reviews!

Regards,
Surbhi.

[Steve Langasek (vorlon)]

Thanks, merged and pushed! Unfortunately due to freeze timing, this is now going to be an SRU rather than included in the natty release pocket; I've still merged it to the 'natty' branch since that's the only available branch for pushing.

BTW, I had to refresh the patches to apply cleanly against the current plymouth source. With dpkg 3.0 (quilt) format, dpkg-source requires the patches to apply with no fuzz.

=== modified file 'debian/changelog'
--- debian/changelog	2011-04-15 13:37:15 +0000
+++ debian/changelog	2011-04-21 07:09:09 +0000
@@ -1,3 +1,15 @@
+plymouth (0.8.2-2ubuntu23) natty; urgency=low
+
+  * details/plugin.c: On every key stroke, the "details" plugin in plymouth
+    clears only the current line before overwriting it with the password
+    prompt. If the prompt is multilined then the last line ends up being
+    overwritten but the previous lines are repeated. Re-printing only the
+    last line of the password prompt on every keystroke. (LP:#566818)
+  * details/plugin.c: Removed the explicit printing of ':'. Expected to be a
+    part of the prompt.
+
+ -- Surbhi Palande <[e-mail]>  Thu, 21 Apr 2011 09:54:07 +0300
+
 plymouth (0.8.2-2ubuntu22) natty; urgency=low
 
   * Install slave alternative for plymouth-theme-ubuntu-logo, telling GRUB

=== added file 'debian/patches/details_removeSeparator.patch'
--- debian/patches/details_removeSeparator.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/details_removeSeparator.patch	2011-04-21 07:09:09 +0000
@@ -0,0 +1,17 @@
+Index: b/src/plugins/splash/details/plugin.c
+===================================================================
+--- a/src/plugins/splash/details/plugin.c
++++ b/src/plugins/splash/details/plugin.c
+@@ -393,10 +393,8 @@
+   }
+   else
+     write_on_views (plugin,
+-                    "Password",
+-                    strlen ("Password"));
+-
+-  write_on_views (plugin, ":", strlen (":"));
++                    "Password:",
++                    strlen ("Password:"));
+ 
+   for (i = 0; i < bullets; i++)
+     write_on_views (plugin, "*", strlen ("*"));

=== modified file 'debian/patches/series'
--- debian/patches/series	2011-03-24 12:01:21 +0000
+++ debian/patches/series	2011-04-21 07:09:09 +0000
@@ -1,3 +1,5 @@
+ubuntu_details.patch
+details_removeSeparator.patch
 misc-changes.patch
 vga16fb-renderer.patch
 lbm-nouveau.patch

=== added file 'debian/patches/ubuntu_details.patch'
--- debian/patches/ubuntu_details.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/ubuntu_details.patch	2011-04-21 07:09:09 +0000
@@ -0,0 +1,47 @@
+Index: b/src/plugins/splash/details/plugin.c
+===================================================================
+--- a/src/plugins/splash/details/plugin.c
++++ b/src/plugins/splash/details/plugin.c
+@@ -349,6 +349,27 @@
+     }
+ }
+ 
++/* @shorten_prompt:
++ *
++ * Points prompt to the character immediately after the
++ * last '\n' in prompt
++ */
++static void shorten_prompt(const char ** prompt)
++{
++  int last_nl=-1, i=0;
++  const char * str = *prompt;
++  for(i=0; str[i] != '\0'; i++) {
++    if(str[i] == '\n') {
++        last_nl = i;
++    }
++  }
++  if (last_nl >= 0) {
++    if((str[last_nl] == '\n') && (last_nl < i)){
++      *prompt = &str[last_nl + 1];
++    }
++  }
++}
++
+ static void
+ display_password (ply_boot_splash_plugin_t *plugin,
+                   const char               *prompt,
+@@ -363,10 +384,13 @@
+                     strlen (CLEAR_LINE_SEQUENCE));
+   plugin->state = PLY_BOOT_SPLASH_DISPLAY_PASSWORD_ENTRY;
+ 
+-  if (prompt)
++  if (prompt) {
++    if (bullets > 0)
++      shorten_prompt(&prompt);
+     write_on_views (plugin,
+                     prompt,
+                     strlen (prompt));
++  }
+   else
+     write_on_views (plugin,
+                     "Password",

=== modified file 'src/plugins/splash/details/plugin.c'
--- src/plugins/splash/details/plugin.c	2010-09-08 09:10:15 +0000
+++ src/plugins/splash/details/plugin.c	2011-04-21 07:09:09 +0000
@@ -349,6 +349,27 @@
     }
 }
 
+/* @shorten_prompt:
+ *
+ * Points prompt to the character immediately after the
+ * last '\n' in prompt
+ */
+static void shorten_prompt(const char ** prompt)
+{
+  int last_nl=-1, i=0;
+  const char * str = *prompt;
+  for(i=0; str[i] != '\0'; i++) {
+    if(str[i] == '\n') {
+        last_nl = i;
+    }
+  }
+  if (last_nl >= 0) {
+    if((str[last_nl] == '\n') && (last_nl < i)){
+      *prompt = &str[last_nl + 1];
+    }
+  }
+}
+
 static void
 display_password (ply_boot_splash_plugin_t *plugin,
                   const char               *prompt,
@@ -363,16 +384,17 @@
                     strlen (CLEAR_LINE_SEQUENCE));
   plugin->state = PLY_BOOT_SPLASH_DISPLAY_PASSWORD_ENTRY;
 
-  if (prompt)
+  if (prompt) {
+    if (bullets > 0)
+      shorten_prompt(&prompt);
     write_on_views (plugin,
                     prompt,
                     strlen (prompt));
+  }
   else
     write_on_views (plugin,
-                    "Password",
-                    strlen ("Password"));
-
-  write_on_views (plugin, ":", strlen (":"));
+                    "Password:",
+                    strlen ("Password:"));
 
   for (i = 0; i < bullets; i++)
     write_on_views (plugin, "*", strlen ("*"));