Opened 4 years ago

Closed 4 years ago

#17744 closed defect (fixed)

Add quotes when comparing strings in configure script

Reported by: cypherpunks Owned by: zerosion
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.5
Severity: Trivial Keywords: easy, TorCoreTeam201603
Cc: Actual Points:
Parent ID: Points: small-remaning
Reviewer: Sponsor:

Description

In ticket:16651#comment:28 missing quotes caused parsing errors. The configure script still contains instances of strings being compared without quotes. AFAIK these remaining cases do not cause any problems right now but it is better to fix them now to avoid problems in the future.

Child Tickets

Attachments (2)

Change History (27)

comment:1 Changed 4 years ago by teor

Keywords: TorCoreTeam201512 added
Milestone: Tor: 0.2.8.x-final
Version: Tor: unspecifiedTor: 0.2.7.5

comment:2 Changed 4 years ago by teor

Keywords: easy added

comment:3 Changed 4 years ago by nickm

Keywords: TorCoreTeam201512 removed

I'd look at a patch for these if we got one, but AFAIK nobody's looking at them right now, and there's no reason to expect them to get done this month.

comment:4 Changed 4 years ago by zerosion

Owner: set to zerosion
Status: newassigned

comment:5 in reply to:  3 Changed 4 years ago by zerosion

Replying to nickm:

I'd look at a patch for these if we got one, but AFAIK nobody's looking at them right now, and there's no reason to expect them to get done this month.

Ok, I just assigned myself to solve this ticket. This is the first time I contribute using this system, so if I'm doing things wrong I'd like to know.

comment:6 Changed 4 years ago by nickm

Sounds good; just make sure you put the ticket into needs_review once the patch is written.

comment:7 Changed 4 years ago by zerosion

Status: assignedneeds_review

comment:8 in reply to:  6 Changed 4 years ago by zerosion

Replying to nickm:

Sounds good; just make sure you put the ticket into needs_review once the patch is written.

Ok, I think it's done, though I don't have a Windows machine to test it on. Whatever you need me to change, I'm here to help.

comment:9 Changed 4 years ago by nickm

ok; I'll be reviewing tickets again once 0.2.8.1-alpha is out -- probably in a day or two.

comment:10 Changed 4 years ago by cypherpunks

I'm curious about why you wrap single-word literal strings in quotes, I'm sure you know this is unnecessary.

I guess it can be argued that it's a bit more future-proof (if later that single-word string has to be turned into a multi-word string, one doesn't have to remember to add the quotes, they are already there); also it looks a bit more regular.

But then again you also _remove_ them in equally superfluous cases where those same arguments would apply.

For example you do:

-if test "$bwin32" = cross; then
+if test "$bwin32" = "cross"; then

But also:

-case "$host_os" in
+case $host_os in

What ho?

comment:11 in reply to:  10 Changed 4 years ago by cypherpunks

Different cypherpunks here.

I agree with comment:10 that strings in switch statements should stay quoted or be quoted if they are not.
The nitpicks that i have are to also quote the test parameters at the top of configure.ac; i.e.

if test -f /etc/redhat-release ; then
  if test -f /usr/kerberos/include ; then
    CPPFLAGS="$CPPFLAGS -I/usr/kerberos/include"
  fi
fi

and to add indentation to the lines you have wrapped in your patch for readability; i.e.

-if test x$enable_gcc_warnings = xyes || test x$enable_gcc_warnings_advisory = xyes; then
+if test "x$enable_gcc_warnings" = "xyes" ||
+test "x$enable_gcc_warnings_advisory" = "xyes"; then

becomes

-if test x$enable_gcc_warnings = xyes || test x$enable_gcc_warnings_advisory = xyes; then
+if test "x$enable_gcc_warnings" = "xyes" ||
+  test "x$enable_gcc_warnings_advisory" = "xyes"; then

comment:12 Changed 4 years ago by zerosion

I'm with you both, I mean, in the case ... in statements I was trying to generalize, but I'd prefer to quote them to avoid errors. Also, I think indentation for readability is a good point.

I'm publishing the patch as soon as I can. Thanks for the advice.

comment:13 Changed 4 years ago by zerosion

Done. I've seen a lot of syntax irregularities, like tabs mixed with spaces and different indentation along the file. Should we open a ticket to handle this, or it's fine?

comment:14 in reply to:  13 ; Changed 4 years ago by teor

Replying to zerosion:

Done.

Code review:

I think it's good to quote $host in the unlikely event this external info has spaces or special characters. Similarly, I would accept other patches that quoted external or user-specified input.

I think it's ok to quote literal file paths that don't currently have spaces. This is consistent with quoting user-specified paths, in case they have spaces or special characters.

I can't see the point of quoting or changing the quotes around internal variables or internal string literals. These are the majority of the changes in the patch, and I'm not sure if they gain us anything. They need to be weighed against the risk that a misplaced quote breaks the build on some obscure configuration.
Was there a reason for these changes?

And finally, if we're going to tidy up whitespace, let's consider that in another ticket. (I'm not sure if we will want to, see my comments below.)

I've seen a lot of syntax irregularities, like tabs mixed with spaces and different indentation along the file. Should we open a ticket to handle this, or it's fine?

Let's open a ticket to document it, but set it to low priority.
The issue with re-flowing the entire file is that it makes it hard to tell when the last non-whitespace change was made to each line.

comment:15 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

comment:16 in reply to:  14 ; Changed 4 years ago by cypherpunks

Replying to teor:

I can't see the point of quoting or changing the quotes around internal variables or internal string literals. These are the majority of the changes in the patch, and I'm not sure if they gain us anything. They need to be weighed against the risk that a misplaced quote breaks the build on some obscure configuration.
Was there a reason for these changes?

As stated in the ticket description, ticket:16651#comment:28 mentions a case where internal variables included spaces and caused issues. This was the reason for opening this ticket in order to prevent similar instances from causing problems in the future.

The issue with re-flowing the entire file is that it makes it hard to tell when the last non-whitespace change was made to each line.

I believe git has options to ignore whitespace changes so they do not interfere when tracking down changes. Furthermore, the tor codebase has numerous commits which purely fix whitespace problems to appease make check-spaces. In these cases change traceability isn't taken into account either so IMO we shouldn't start with it now.

comment:17 in reply to:  16 Changed 4 years ago by teor

Replying to cypherpunks:

Replying to teor:

I can't see the point of quoting or changing the quotes around internal variables or internal string literals. These are the majority of the changes in the patch, and I'm not sure if they gain us anything. They need to be weighed against the risk that a misplaced quote breaks the build on some obscure configuration.
Was there a reason for these changes?

As stated in the ticket description, ticket:16651#comment:28 mentions a case where internal variables included spaces and caused issues. This was the reason for opening this ticket in order to prevent similar instances from causing problems in the future.

I understand now. I'd be happy for the patch to be merged as-is.

The issue with re-flowing the entire file is that it makes it hard to tell when the last non-whitespace change was made to each line.

I believe git has options to ignore whitespace changes so they do not interfere when tracking down changes. Furthermore, the tor codebase has numerous commits which purely fix whitespace problems to appease make check-spaces. In these cases change traceability isn't taken into account either so IMO we shouldn't start with it now.

Fair enough. Please make whitespace changes in a separate commit. (And ideally on another ticket.)

If we can work out a way to keep the whitespace consistent, ideally using make check-spaces, then that will help us ensure that the initial change is correct, and that future changes don't re-introduce whitespace issues.

comment:18 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

comment:19 Changed 4 years ago by nickm

Points: small-remaning

comment:20 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged; and added a changes file. Thanks!

comment:21 Changed 4 years ago by toralf

2 are missing still :

~/devel/tor $ grep -e ' xyes' -e ' xno' configure.ac
AM_CONDITIONAL(UNITTESTS_ENABLED, test x$enable_unittests != xno)
AM_CONDITIONAL(COVERAGE_ENABLED, test x$enable_coverage = xyes)

comment:22 Changed 4 years ago by teor

Resolution: implemented
Status: closedreopened

comment:23 Changed 4 years ago by teor

Status: reopenedneeds_review

Let's merge after 0.2.8-rc

comment:24 Changed 4 years ago by nickm

Keywords: TorCoreTeam201603 added

comment:25 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Fixed in c8eb39d67f4ea5. (Those lines were redundant with the two before them, so I just removed them.)

Note: See TracTickets for help on using tickets.