PeterPawn / YourFritz

dynamic package management for AVM routers
GNU General Public License v2.0
219 stars 63 forks source link

"expr: syntax error" in juis_check #19

Closed goligo closed 5 years ago

goligo commented 5 years ago

Tried juis on my Mac (Mojave 10.14.2) with my 6490, but the following error occurs when running it:

debug: ------------------------------------------------------- debug: Reading values from 'fritz.box:80/jason_boxinfo.xml': . debug: Read response from device: debug: ------------------------------------------------------- HTTP/1.1 200 OK Cache-Control: max-age=120 Connection: close Content-Type: text/xml;charset=utf-8 Date: Fri, 14 Dec 2018 17:38:40 GMT ETag: "D008DE7B64CF95EFB" Expires: Fri, 14 Dec 2018 17:40:40 GMT Last-Modified: Thu, 01 Jan 1970 00:01:33 GMT Mime-Version: 1.0

   <j:BoxInfo xmlns:j="http://jason.avm.de/updatecheck/">
   <j:Name>FRITZ!Box 6490 Cable</j:Name>
   <j:HW>213</j:HW>
   <j:Version>141.06.51</j:Version>
   <j:Revision>34089</j:Revision>
   <j:Serial>entfernt</j:Serial>
   <j:OEM>avm</j:OEM>
   <j:Lang>de</j:Lang>
   <j:Annex>Kabel</j:Annex>
   <j:Lab></j:Lab>
   <j:Country>049</j:Country>
   <j:Flag>crashreport</j:Flag>
   <j:UpdateConfig>1</j:UpdateConfig></j:BoxInfo>

debug: Splitting compound version number '141.06.51-34089' to: expr: syntax error debug: Major=141 debug: Minor=06 debug: Patch=51 debug: Buildnumber=34089 debug: Compound version number used for request: '34089.00.00-'

debug: ------------------------------------------------------- debug: Variables set: debug: ------------------------------------------------------- debug: Name="FRITZ!Box 6490 Cable" debug: HW="213" debug: OEM="avm" debug: Lang="de" debug: Annex="Kabel" debug: Country="049" debug: Serial="entfernt" debug: Major="" debug: Minor="" debug: Patch="" debug: Buildnumber="34089" [...]

The request to the update service is called without version info, causing a 500 response:

           <q:Name>FRITZ!Box 6490 Cable</q:Name>
           <q:HW>213</q:HW>
           <q:Major></q:Major>
           <q:Minor></q:Minor>
           <q:Patch></q:Patch>
           <q:Buildnumber>34089</q:Buildnumber>
           <q:Buildtype>1001</q:Buildtype>
goligo commented 5 years ago

The "expr: syntax error" seems not to do any harm - the empty Major/Minor/Patch version is caused by the last change

https://github.com/PeterPawn/YourFritz/commit/eaa8e3b2ba6f2eba06021e76a38a0ab679bc7010

When checking out the previous version it works as expected.

PeterPawn commented 5 years ago

Possibly the MacOS version of "expr" uses a different syntax ... once again.

It was necessary to apply the latest changes, because the new version number "07.08" raised an error.

If you look at your debug output, the error seems to sit between the "split_version_number" call (https://github.com/PeterPawn/YourFritz/blob/master/juis/juis_check#L1356) and the immediately following debug output. Therefore the error has to occur in the subfunction call ... but the output values (shown in the debug lines after this call) are looking good.

It surely a MacOS related problem, because the code runs without error on a "native" Linux system:

debug: Splitting compound version number '141.06.51-34089' to:
debug: Major=141
debug: Minor=06
debug: Patch=51
debug: Buildnumber=34089
debug: Compound version number used for request: '141.06.51-34089'

I have no idea, why the last version swaps the values on your system:

debug: Compound version number used for request: '34089.00.00-'

... that's obviously the wrong order and any later error parsing this string isn't really unexpected.

But at the same time I can't see any interrelation between the latest patch and the code used for parsing this version number ... as a result, I can't understand, why the previous version works for you.

If you want to get this error analyzed by me, please run the script using an explicit shell call with "-x" option and show me the trace output produced by your shell. I'm interested in fixing the problem - but I haven't a MacOS-based system anymore and the existing code runs in "pure POSIX mode" with a "dash" shell without errors.

The line, where the final version string is prepared (it's line 1458 in the latest version and the content of line 639 gets evaluated there - no "expr" call for miles around here), was not changed in the last commit.

goligo commented 5 years ago

Thanks for your response. I also tried on a FreeBSD 11.2 system which is showing the same issue. As I wrote above it seems to be unrelated to the expr error, which also occurs in the working version, but is just caused by the latest change.

There is a lot of output when doing the shell call with -x, the relevant part seems to be

+ eval 'v=$Major'
+ v=141
+ get_decimal_value 141
+ expr 141 : '0*\([0-9]\+\)'
+ eval 'Major='
+ Major=''
+ eval 'v=$Minor'
+ v=06
+ get_decimal_value 06
+ expr 06 : '0*\([0-9]\+\)'
+ eval 'Minor='
+ Minor=''
+ eval 'v=$Patch'
+ v=51
+ get_decimal_value 51
+ expr 51 : '0*\([0-9]\+\)'
+ eval 'Patch='
+ Patch=''
+ [ 0 -eq 1 ]
+ eval 'Version=$(printf %d.%02d.%02d-%s $Major $Minor $Patch $Buildnumber)'
+ printf %d.%02d.%02d-%s 34089
+ Version=34089.00.00-

As far I understand get_decimal_value does just return empty values.

goligo commented 5 years ago

POSIX basic regular expressions do not support + or ?, instead you need to write {1,} or {0,1}

get_decimal_value()
(
        expr "$1" : "0*\([0-9]\{1,\}\)"
)

does fix it.

PeterPawn commented 5 years ago

That's correct (let's ignore the "expr" error for now - but I want to fix this later, too) ... the real question is, why the regex is annoying the "expr" utility on BSD derivates.

Please try it again with the script from new branch 'juis' ... I've changed the content of the regex operand slightly. Maybe it's working now on BSD systems, too.

PeterPawn commented 5 years ago

BTW ... {0,1} fails (using "dash") on all-zeros input:

expr "0000" : "0*\([0-9]\{0,1\}\)"

The wanted result is "0" here. But I didn't see the additional message, before I committed the new version.

EDIT: Sorry, got it really late, but I see it now ... the {0,1} part was only the alternative to the question mark.

The other option {1,} could be used, but there's no problem with the expression [1-9]*[0-9] instead.


If the new version works as expected and you're able to locate the statement, which raises the earlier error message regarding the "expr" call (it should be a different problem), I'd like to fix this error, too.

goligo commented 5 years ago

This is where the syntax error occurs

+ __first_char -%Buildnumber%]
+ expr '(' -%Buildnumber%] : '\(.\).*' ')'
+ c=-
+ __remove_first_char -%Buildnumber%]
+ expr '(' -%Buildnumber%] : '.\(.*\)' ')'
+ m=%Buildnumber%]
+ [ - '=' % ]
+ expr '(' - : '\([A-Za-z]\)' ')'
expr: syntax error
PeterPawn commented 5 years ago

Hmm ... the outer parentheses should protect the single 'dash' character (as input) from being recognized as an "option starts here" marker.

Could you please try this as a single command:

c='-'; o=$(expr -- \( "$c" : "\([A-Za-z]\)" \) ); echo "${#o} $o"

and compare it with the version without the "double-dashes" in front of the other arguments?

The problem is, that some "expr" implementations (e.g. the one from BusyBox) do not accept a double-dash in front of the other arguments, because the "expr" command does not take any options usually (http://pubs.opengroup.org/onlinepubs/9699919799/) and the BSD version is a big exception here. Most other "expr" implementations haven't own options, but the double-dash will be detected and processed as expected:

peh@vidar:~> expr -- - : "\(.\)"
-
peh@vidar:~> busybox expr -- - : "\(.\)"
expr: non-numeric argument
peh@vidar:~> busybox expr - : "\(.\)"
-
peh@vidar:~>

As mentioned above, the parentheses around the whole expression were an alternative here, because they should start the evaluation of the "inner expression" and there can't be a valid option within the parentheses anymore.

I'm surprised and a bit helpless, why it does not work as expected in this case.

goligo commented 5 years ago

The problem seems to be with the standalone minus on the left side. I couldn't figure out any reasonable way to quote or to escape this - easiest way to solve seems to add some prefix on both sides, like

elif [ -z "$(expr \( "_$c" : "\(_[A-Za-z]\)" \) )" ]; then

goligo commented 5 years ago

c='-'; o=$(expr -- ( "$c" : "([A-Za-z])" ) ); echo "${#o} $o" Illegal variable name.

expr -- - : "(.)" expr: syntax error

expr - : "(.)" expr: syntax error

goligo commented 5 years ago

As far I understand the issue is not that expr is trying to interprete the minus as a command line option, but the minus is a valid operator, and the documentation of expr states

"An operand which is lexically identical to an operator will be considered a syntax error."

PeterPawn commented 5 years ago
peh@vidar:~> c='-'; o=$(expr -- \( "$c" : "\([A-Za-z]\)" \) ); echo "${#o} $o"
0
peh@vidar:~> c='-'; o=$(expr \( "$c" : "\([A-Za-z]\)" \) ); echo "${#o} $o"
0
peh@vidar:~> c='A'; o=$(expr \( "$c" : "\([A-Za-z]\)" \) ); echo "${#o} $o"
1 A
peh@vidar:~> c='A'; o=$(expr -- \( "$c" : "\([A-Za-z]\)" \) ); echo "${#o} $o"
1 A
peh@vidar:~>

It's a known problem to me (https://github.com/PeterPawn/YourFritz/commit/ed405745d0d371df91e3e1d49ebccc1fdffe811c and https://github.com/PeterPawn/freetz/commit/f7d6aecd02745f3214ea11f6b233d944c3644021) ... but I do not understand, why the version with outer parentheses will not work in this case.

Usually I'm restrained and do not blame other programs, they're working "wrong" ... but in this case it looks like an error by "expr" on BSD systems. Why should it be a syntax error, if the expression to evaluate starts with a group of parentheses? It's a "grouping" of operands, as described by the POSIX specification.

PeterPawn commented 5 years ago

Does it change anything, if you define a "EXPR_COMPAT" environment variable prior to executing the commands?

goligo commented 5 years ago

Well, it is very old, always worked like this, and is unlikely to be changed.

PeterPawn commented 5 years ago

I'm looking for a portable solution on all platforms. The last resort would be an appended prefix or suffix or an additional test, whether the character is a single dash, with the result, that the "expr" call is not made in this case.

goligo commented 5 years ago

$ set | grep EXPR_COMPAT EXPR_COMPAT=1 $ c='-'; o=$(expr ( "$c" : "([A-Za-z])" ) ); echo "${#o} $o" expr: syntax error 0

goligo commented 5 years ago

expr first appeared in Unix V7, released in 1979 ;-)

PeterPawn commented 5 years ago

Ok, I've published a new version with an additional check for the hyphen ... it's in the "juis" branch now.

Could you try this version once more please?

Thanks for your patience.

PeterPawn commented 5 years ago

I was sixteen at this time and started working with computers. :-)

goligo commented 5 years ago

Looks good now, only the response is not satisfying me... but that is a different issue.

juis_check: No newer version found, check was made with source version '141.06.51-34089'.

PeterPawn commented 5 years ago

Thanks again, I'll move the new version from its own branch to "master" and close this issue afterwards.

PeterPawn commented 5 years ago

Solved with new version from https://github.com/PeterPawn/YourFritz/commit/3a5570a6782321b024defa46d3750e01b8c5fe68 - closed.