Let's bump priority a bit since this is a (slight) security issue. It would be nice to get this fixed within M-55.

Can you own this or do we need to find somebody else?

We should fix whatever causes the tty to fail (and presumably prevents us from displaying the progress bar), but ultimately we should also rewrite the code such that any sort of problem with pv or stty won't cause the dd to abort. Adding Mike who maybe has a good idea how to do this in bash. Relevant code snippet for reference:

    # Opening a TTY device with no one else attached will allocate and reset     
    # terminal attributes. To prevent user input echo and ctrl-breaks, we need   
    # to allocate and configure the terminal before using tty for output.        
    # Note &2 is currently used for debug messages dumping so we can't redirect  
    # subshell by 2>"${TTY}".                                                    
    ( stty -F "${TTY}" raw -echo -cread                                          
      pv -etp -s $((4 * 1024 * 1024 * ${FULL_BLKS})) /dev/zero 2>"${TTY}" |      
        dd of=${DEV} bs=4M count=${FULL_BLKS} iflag=fullblock oflag=sync         
    ) >"${TTY}"                                   

TTY currently defaults to /dev/tty1 which requires the VT to exist -- on a non-freon based build, the kernel does it before userspace starts, but for freon builds, frecon creates them on the fly.  looking at our boot scripts, there's a race here:
- pre-startup stops
- startup (run chromeos_startup) & udev run
- chromeos_startup executes clobber-state
- udev-trigger-early runs once udev finishes startup
- boot-splash runs once udev-trigger-early finishes executing (including waiting for many classes of devices)
- boot-splash runs frecon which handles vt creation
(iiuc the VT interaction correctly)

should be able to reproduce on any device by putting a sleep 60 in boot-splash at the start of its script code.

for terminal signals, we could ignore things by doing:
  trap '' INT QUIT KILL TSTP TERM
  stty ... || :

the redirect could be split out into a sep phase so failures can be skipped:
  exec >& ${TTY} || :
we could send the output to /dev/null in case it failed, but seems like it'd be better to just let the output go to the log so we know we won't miss anything weird ?

alternatively, if the shell is getting too dicey, we could always hack together our own little C program that'd do everything for us.

Hi Mike,

I uploaded a CL with your suggestions above:
https://chromium-review.googlesource.com/#/c/389871/

I am unfortunately not seeing the output of pv on the screen for any DUT that I've tried (although the dd operation is indeed finishing now).  Did I do something wrong?

Hi Mike,

Thanks for the code review.  I tried your suggestion of putting exec before the pv command (CL above has been updated to reflect this), but seems like the output to pv is still not being outputted to /dev/tty1.  I'm currently testing on a chell, where I know that the progress bar was printing to screen before my changes were implemented.

I also tried adding in the pipe from pv directly (although I know that this would have caused an early abort on the jerry) and that also didn't output the progress bar:

( trap '' INT QUIT KILL TSTP TERM
  stty -F "${TTY}" raw -echo -cread || :
  exec &> "${TTY}" || :
  pv -etp -s $((4 * 1024 * 1024 * ${FULL_BLKS})) /dev/zero 2> "{$TTY}" |
    dd of=${DEV} bs=4M count=${FULL_BLKS} iflag=fullblock oflag=sync
)

the redirect after the pv is incorrect (has a typo) and unneeded.  so delete this part:
  2> "{$TTY}"

Hi Mike, 

Sorry, I should've copied the code instead of putting it in the CL to make it more clear.  My original attempt was (which did not result in the progress bar being outputted to screen):

( trap '' INT QUIT KILL TSTP TERM
  stty -F "${TTY}" raw -echo -cread || :
  exec &> "${TTY}" || :
  pv -etp -s $((4 * 1024 * 1024 * ${FULL_BLKS})) /dev/zero |
    dd of=${DEV} bs=4M count=${FULL_BLKS} iflag=fullblock oflag=sync
)

w/out debugging the code myself, i don't know offhand why it isn't working.  you can try writing some debug code that writes to somewhere stable like /var/log/ or /usr/local/ to see what's going on.  like check the exit code of each command and see if $TTY even exists.

if the existing code works, then try changing only one part at a time until it stops working to narrow down.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/23e0916a8042189552abe8ce48db68799b7ccf34

commit 23e0916a8042189552abe8ce48db68799b7ccf34
Author: Shelley Chen <shchen@chromium.org>
Date: Wed Oct 12 21:49:28 2016

init: clobber-state: Add wait loop for TTY to get set up

Currently, the normal->dev transition on veyron_jerry
errors out with when pv tries to redirect output to an
invalid tty.  Adding polling before the call to make sure
that the tty is valid before trying to redirect to it.

BUG= chromium:647786 
BRANCH=None
TEST=run normal->dev transition on veyron_jerry,chell

Change-Id: If5003d76f0c035f86144203d6795c8ea1db73387
Signed-off-by: Shelley Chen <shchen@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/398539
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/23e0916a8042189552abe8ce48db68799b7ccf34/init/clobber-state

[Automated comment] Request affecting a post-stable build (M54), manual review required.

Your change meets the bar and is auto-approved for M55 (branch: 2883)

[Automated comment] Request affecting a post-stable build (M54), manual review required.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/16ca9bdcc3989947b92d6bed2c89c2baf32e636f

commit 16ca9bdcc3989947b92d6bed2c89c2baf32e636f
Author: Shelley Chen <shchen@chromium.org>
Date: Wed Oct 12 21:49:28 2016

init: clobber-state: Add wait loop for TTY to get set up

Currently, the normal->dev transition on veyron_jerry
errors out with when pv tries to redirect output to an
invalid tty.  Adding polling before the call to make sure
that the tty is valid before trying to redirect to it.

BUG= chromium:647786 
BRANCH=None
TEST=run normal->dev transition on veyron_jerry,chell

Change-Id: If5003d76f0c035f86144203d6795c8ea1db73387
Signed-off-by: Shelley Chen <shchen@chromium.org>
Previous-Reviewed-on: https://chromium-review.googlesource.com/398539
(cherry picked from commit afea330875699e36d55782e1744e034db49c743e)
Reviewed-on: https://chromium-review.googlesource.com/407824
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/16ca9bdcc3989947b92d6bed2c89c2baf32e636f/init/clobber-state

R54 is expired already, if this is fixed we can probably close it. 

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Has been merged into 55

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Verified in 9765.7.0/61.0.3163.13.