Project

General

Profile

Bug #10785

_get_tg_setting() broken with set -u

Added by anonym over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Persistence
Target version:
Start date:
12/21/2015
Due date:
% Done:

100%

QA Check:
Pass
Feature Branch:
bugfix/10785-_get_tg_setting-vs-set-u
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

I saw this in the journal:

Dec 21 15:53:40 amnesia add-GNOME-bookmarks[3543]: /usr/local/lib/add-GNOME-bookmarks: 1: eval: TAILS_PERSISTENCE_READONLY: parameter not set

Indeed, tails-greeter.sh's _get_tg_setting() (via persistence_is_enabled_read_write()) is not adapted for set -u.

Associated revisions

Revision 2d28e314 (diff)
Added by anonym over 2 years ago

Make _get_tg_setting() compatible with set -u.

Will-fix: #10785

Revision 7f82c158 (diff)
Added by anonym over 2 years ago

Make _get_tg_setting() compatible with set -u.

Will-fix: #10785

Revision 563b18c6
Added by intrigeri over 2 years ago

Merge remote-tracking branch 'origin/bugfix/10785-_get_tg_setting-vs-set-u' into devel

Fix-committed: #10785

History

#1 Updated by anonym over 2 years ago

Hmm. I would expect add-GNOME-bookmarks to fail to create the bookmark for ~/Persistent/Tor Browser, but it does work. Any explanation?

I still think we should fix this...

#2 Updated by anonym over 2 years ago

  • Status changed from Confirmed to In Progress

#3 Updated by anonym over 2 years ago

  • Assignee deleted (anonym)
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to bugfix/10785-_get_tg_setting-vs-set-u

#4 Updated by anonym over 2 years ago

Just noticed that create-tor-browser-directories is "addected" by the same issue, which this branch also will fix.

#5 Updated by intrigeri over 2 years ago

  • Assignee set to intrigeri

Thanks!

#6 Updated by intrigeri over 2 years ago

(Removed buggy explanation, I misread the question.)

#7 Updated by intrigeri over 2 years ago

anonym wrote:

Hmm. I would expect add-GNOME-bookmarks to fail to create the bookmark for ~/Persistent/Tor Browser, but it does work. Any explanation?

The buggy statement is eval'ed by a function that's itself called in a subshell with $(). Looks like set -e is lost somewhere along the way. Did I mention that I find it hard to write robust shell scripts? :)

#8 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed

I have merged this branch locally, built an ISO, upgraded a USB stick with it, booted, and I see the same message in my logs. So apparently the branch does not really address the problem. Am I doing it wrong, or?

#9 Updated by anonym over 2 years ago

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

The buggy statement is eval'ed by a function that's itself called in a subshell with $(). Looks like set -e is lost somewhere along the way.

No, set -e is not lost. Note that set -e will only make the current scope return with an error, so execution will only abort on the top level of the script. So the buggy reference of the undefined variable in _get_tg_setting() will just make it return false thanks to set -u + set -e, and then set -e will trigger in the caller, persistence_is_enabled_read_write(), making it return false, which is an expected output, but now it means something completely different. Yay. In a better language we'd have an exception being raised here instead of overloading the boolean value "false" with "execution error".

Did I mention that I find it hard to write robust shell scripts? :)

I could not agree more! :)

I have merged this branch locally, built an ISO, upgraded a USB stick with it, booted, and I see the same message in my logs. So apparently the branch does not really address the problem. Am I doing it wrong, or?

I have force pushed the fix. I believe I originally fixed the script in a live session and tested it successfully, but then transcribed the code incorrectly into Tails' Git. Sorry!

#10 Updated by intrigeri over 2 years ago

No, set -e is not lost. Note that set -e will only make the current scope return
with an error, so execution will only abort on the top level of the script. So the
buggy reference of the undefined variable in _get_tg_setting() will just make it
return false thanks to set -u + set -e, and then set -e will trigger in the
caller, persistence_is_enabled_read_write(), making it return false, which is an
expected output, but now it means something completely different. Yay. In a better
language we'd have an exception being raised here instead of overloading the boolean
value "false" with "execution error".

O.M.G. /me, fleeing.

I have force pushed the fix.

Thanks! I'll look at it shortly.

Glad to see you back, by the way! :)

#11 Updated by intrigeri over 2 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 50 to 100

#12 Updated by intrigeri over 2 years ago

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#13 Updated by anonym over 2 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF