fix parseArgument #5230

d-a-v · 2018-10-11T15:15:52Z

url: 'foo'
args=1
arg(0): 'foo' = ''

url: 'foo='
args=1
arg(0): 'foo' = ''

url: 'foo=bar'
args=1
arg(0): 'foo' = 'bar'

url: 'foo&bar&lol=&ness=&key=val;another=way;to;separate'
args=8
arg(0): 'foo' = ''
arg(1): 'bar' = ''
arg(2): 'lol' = ''
arg(3): 'ness' = ''
arg(4): 'key' = 'val'
arg(5): 'another' = 'way'
arg(6): 'to' = ''
arg(7): 'separate' = ''

url: 'foo&bar&lol=&ness=&key=val;another=way;to;separate='
args=8
arg(0): 'foo' = ''
arg(1): 'bar' = ''
arg(2): 'lol' = ''
arg(3): 'ness' = ''
arg(4): 'key' = 'val'
arg(5): 'another' = 'way'
arg(6): 'to' = ''
arg(7): 'separate' = ''

url: 'foo&bar&lol=&ness=&key=val;another=way;to;separate=sobeit'
args=8
arg(0): 'foo' = ''
arg(1): 'bar' = ''
arg(2): 'lol' = ''
arg(3): 'ness' = ''
arg(4): 'key' = 'val'
arg(5): 'another' = 'way'
arg(6): 'to' = ''
arg(7): 'separate' = 'sobeit'

url: 'foo&;bar&;lol=&;ness=&;&key=val;&another=way;&to;&separate=sobeit'
args=8
arg(0): 'foo' = ''
arg(1): 'bar' = ''
arg(2): 'lol' = ''
arg(3): 'ness' = ''
arg(4): 'key' = 'val'
arg(5): 'another' = 'way'
arg(6): 'to' = ''
arg(7): 'separate' = 'sobeit'

url: 'foo&;bar&;lol=&;ness=&;&key=val;&another=way;&to;&separate=sobeit&&&;;;'
args=8
arg(0): 'foo' = ''
arg(1): 'bar' = ''
arg(2): 'lol' = ''
arg(3): 'ness' = ''
arg(4): 'key' = 'val'
arg(5): 'another' = 'way'
arg(6): 'to' = ''
arg(7): 'separate' = 'sobeit'

url: ';;;;'
args=0

url: ';;a;;'
args=1
arg(0): 'a' = ''

edit
~~url: ''~~
~~args=1~~
~~arg(0): '' = ''~~

it is really:

...

url: ';;;;'
args=0

url: ';;a;;'
args=1
arg(0): 'a' = ''

url: ''
args=0

(bad copy paste)

devyte · 2018-10-11T16:04:04Z

@d-a-v recursive calls are hard to understand at a glance, and they're dangerous on a stack-constrained platform like ours.
Instead, I suggest migrating to std::vector. To reduce fragmentation and overallocation, you could do a two pass approach: go over the buffer counting & and ; , then reserve(), then go over the buffer again parsing.

earlephilhower · 2018-10-11T16:04:43Z

Looks great from the tests, with one minor niggle.

The last check:

url: ''
args=1
arg(0): '' = ''

(I'm assuming "url" should be "string after 1st "?")

For, say "http://test.me/doit.cgi?" (the last test), shouldn't args==0?

d-a-v · 2018-10-11T20:03:42Z

@earlephilhower True. I made copy paste of some previous result. fixed in OP.

@devyte Recursion is only one level: the number of inner call is really 1 and does not depend on the request itself. For clarity I removed this call. Still not using vector<> because I realized that implication with the rest of the code is unknown (for example, allocation must be increased by one - see comments).

earlephilhower

Thanks for the check on the null-param case. LGTM!

d-a-v · 2018-10-11T20:38:45Z

@ascillato @andrethomas @reloxx13 I assume you have made tests with alexa ?
Were you able to talk to your ESPs like with the other PR ?

ascillato · 2018-10-11T21:14:01Z

@d-a-v

With this PR Alexa bug is stills there. Sorry. With the PR I proposed, Alexa was working.

I'm checking your code and it should work but I don't know why it doesn't. It needs something extra for the parameters?

Testing using Sonoff-Tasmota.

reloxx13 · 2018-10-11T21:26:08Z

@ascillato
cuz this line(198) is untouched in this pr:

Arduino/libraries/ESP8266WebServer/src/Parsing.cpp

Lines 198 to 203 in db0cd7d

    
           if(!isEncoded){ 
        
             //plain post json or other data 
        
             RequestArgument& arg = _currentArgs[_currentArgCount++]; 
        
             arg.key = F("plain"); 
        
             arg.value = String(plainBuf); 
        
           }

alexa does not or does send a FALSE encoding, leading to skip the parsing.

in my pr there is a check if their are args found

https://github.com/esp8266/Arduino/pull/4236/files#diff-b4a712936d95712be9c9c0e6fc7e80ffR198

ascillato · 2018-10-11T21:51:26Z

@d-a-v

Testing Workaround for Alexa Bug #4236 with actual STAGE version works with Alexa.
Testing Accept WebServer Query Arguments not containing an equals sign (and ALEXA issue) #5222 with actual STAGE version works with Alexa.

So, as any of the 2 options solves the Alexa Bug, can be added to your PR any of those approaches ?

d-a-v · 2018-10-11T22:45:51Z

@ascillato Thanks for your two tests and your links in #5222.
Here I'm a fixer and I can't fix without understanding.
Without having invited alexa at home, but with reading detailed @OFreddy's findings, I'll go for his solution.

ascillato · 2018-10-12T19:47:26Z

@d-a-v

Hi
we made several tests using the core from https://github.com/d-a-v/arduino/tree/parserequest and we could not make it work with Alexa.
I don't know why, in the parsing code, it is being ignored the alexa request.

If I just change in the last actual core only the line 198 from parsing.cpp to

if(!isEncoded||(0==_currentArgCount)){

Alexa works!

I don't know what is happening with the rest of the parsing you have added that this line stop working.

key_end_pos--; dont count down here, it will cut of every key by -1 ("save" will be "sav") (substring (end = up to, but not including, so no need to -1) Parsing cpp L329 arg.value = urlDecode(data.substring(equal_index + 1, next_index - 1)); => -1 is too less for substring (substring (end = up to, but not including, so no need to -1)

reloxx13 · 2018-10-13T00:36:31Z

latest switch hue on with alexa log

01:37:12 WIF: verbunden
method: PUT url: /api/dIUhc67uzIHamo5mYqfQVjkjgRMsvkXeZo4u4Fg/lights/1/state search:
headerName: Host
headerValue: 192.168.178.73
headerName: Accept
headerValue: */*
headerName: Content-type
headerValue: application/x-www-form-urlencoded

headerName: Content-Length
headerValue: 12
args: {"on": true}
args count: 1
args: {"on": true}
arg 0 key: {"on": true} value:
args count: 1
Plain: {"on": true}
Request: /api/dIUhc67uzIHamo5mYqfQVjkjgRMsvkXeZo4u4Fg/lights/1/state
 Arguments: {"on": true}
01:37:22 HTP: Hue-API (/dIUhc67uzIHamo5mYqfQVjkjgRMsvkXeZo4u4Fg/lights/1/state)
01:37:22 HTP: Hue POST-Argumente ()
01:37:22 HTP: Hue POST-Argumente ({"on": true})

reloxx13 · 2018-10-13T00:43:16Z

~~now alexa is not working cuz the json is detected as an arg and again its not going inside the if i mentioned before.~~
edit: nope, i dont think its that, but i have no clue now, need more investigation

its too late, good night ;p

devyte

As disccussed, please update per this gist:
https://gist.github.com/devyte/0e51bc55557af5a3b56bfb040515db08

fix pos substrings

…uest

d-a-v · 2018-10-14T00:57:03Z

@devyte I'll open an issue for your proposal. This PR was itself a pain to fix (no alexa around, me not used to http request world, difficulties to test, lots of cases and no emulated environment (yet :) to speed tests up). It should be part of a general cleaning pass.

@ alexa users, please tell me this is working (= I was working with good fake requests with AdvancedWebserver example).

special thanks to @reloxx13 who made a PR (merged) in my branch to fix a nasty parsing bugs I was not seeing in my original commit (the bug was in my testing process, mainly, so we need that emulated enviroment fully working #1715).

reloxx13 · 2018-10-14T16:57:16Z

just tested latest commit on https://github.com/d-a-v/Arduino/tree/parserequest and works for me with alexa and web saves/posts

log:

New client
method: PUT url: /api/dIUhc67uzIHamo5mYqfQVjkjgRMsvkXeZo4u4Fg/lights/1/state search:
headerName: Host
headerValue: 192.168.178.73
headerName: Accept
headerValue: */*
headerName: Content-type
headerValue: application/x-www-form-urlencoded
headerName: Content-Length
headerValue: 12
args:
args count: 0
args:
args count: 0

Request: /api/dIUhc67uzIHamo5mYqfQVjkjgRMsvkXeZo4u4Fg/lights/1/state
Arguments:
final list of key/value pairs:
  key:'plain' value:'{"on": true}'
request handler not found
18:55:40 HTP: Hue-API (/dIUhc67uzIHamo5mYqfQVjkjgRMsvkXeZo4u4Fg/lights/1/state)
18:55:40 HTP: Hue POST-Argumente ({"on": true})
18:55:40 SRC: Hue
18:55:40 RSL: RESULT = {"POWER":"ON"}
18:55:40 RSL: POWER = ON

ascillato · 2018-10-16T05:12:34Z

Hi,

I found that there is a memory leak using this PR.

If I use the Core at the commit c8497da the core works fine. No issues.

If I add this PR to that commit, In few minutes all RAM goes to zero and the device reboots.

ascillato

Please, review where is the memory leak.

Tested using request http://192.168.1.32/in?

ascillato · 2018-10-16T11:29:36Z

@d-a-v

Very Nice work! Bug solved. All working. 👍

Please, merge this PR.

ascillato · 2018-10-16T15:26:46Z

Tested using Sonoff-Tasmota for several hours and no issue. The actual core with this PR is working really good and fast. No memory leak, no reboots, no disconnections, no exceptions, sleep works, OTA URL works, and Alexa works.

Hope this got merged soon as to have a reference commit of a very stable version.

Jason2866 · 2018-10-16T15:34:59Z

@d-a-v Great Job :-) !!
Memory Leak is gone. No reboot. MQTT is stable tested with PUBSUBCLIENT and ARDUINOMQTT
IwIP 2hb and IwIP 1.4 tested. Both work good. WebFrontend works without errors. OTA Error solved.
Sleep works AND finally ALEXA works reliable. ALL bugs that i had are gone!!
Devices with Sonoff-Tasmota are fast and responsive :-)

Please do a Merge very very soon to Master to have a reference commit of a very well done stable Version.

T H X for your great work

andrethomas · 2018-10-16T19:29:56Z

Great work @d-a-v

I get the same result as @Jason2866

Everything seems to be working as it should.

Can we please get this PR merged so that we can continue to use this as a staged core for Tasmota development binaries.

Much appreciated, THX!

devyte · 2018-10-17T02:19:35Z

Closing in favor of #5252 .

fix parseArgument

4787753

d-a-v mentioned this pull request Oct 11, 2018

Accept WebServer Query Arguments not containing an equals sign (and ALEXA issue) #5222

Closed

ascillato approved these changes Oct 11, 2018

View reviewed changes

Merge branch 'master' into parserequest

454a065

andrethomas approved these changes Oct 11, 2018

View reviewed changes

remove recursion

aa4dac9

ascillato approved these changes Oct 11, 2018

View reviewed changes

earlephilhower approved these changes Oct 11, 2018

View reviewed changes

keep variable naming style consistent

db0cd7d

d-a-v added 2 commits October 12, 2018 00:47

inviting Alexa

ac47a03

Merge branch 'master' into parserequest

fa4cc7a

d-a-v and others added 2 commits October 12, 2018 23:40

fix alexa fix (well, trying...)

bdf2ace

devyte requested changes Oct 13, 2018

View reviewed changes

d-a-v added 5 commits October 13, 2018 22:44

Merge branch 'master' into parserequest

1bc93d8

alexa invite: add workaround for malformed x-www-form-urlencoded

0d62a41

Merge pull request #1 from reloxx13/patch-2

8be9539

fix pos substrings

Merge branch 'parserequest' of github.com:d-a-v/Arduino into parsereq…

f7b3aef

…uest

when !form, alway add content in key "plain"

cf2299f

ascillato approved these changes Oct 16, 2018

View reviewed changes

ascillato suggested changes Oct 16, 2018

View reviewed changes

fix memleak

9d1e411

ascillato approved these changes Oct 16, 2018

View reviewed changes

andrethomas approved these changes Oct 16, 2018

View reviewed changes

+simplify +readability

8abf98d

d-a-v mentioned this pull request Oct 16, 2018

fix parseArgument #2 #5252

Merged

Merge branch 'master' into parserequest

00694bb

devyte closed this Oct 17, 2018

d-a-v deleted the parserequest branch October 17, 2018 10:09

reloxx13 mentioned this pull request Oct 19, 2018

Webserver Parsing Issue #5261

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix parseArgument #5230

fix parseArgument #5230

d-a-v commented Oct 11, 2018 •

edited

Loading

devyte commented Oct 11, 2018

earlephilhower commented Oct 11, 2018 •

edited

Loading

d-a-v commented Oct 11, 2018

earlephilhower left a comment

d-a-v commented Oct 11, 2018 •

edited

Loading

ascillato commented Oct 11, 2018 •

edited

Loading

reloxx13 commented Oct 11, 2018 •

edited

Loading

ascillato commented Oct 11, 2018

d-a-v commented Oct 11, 2018

ascillato commented Oct 12, 2018 •

edited

Loading

reloxx13 commented Oct 13, 2018 •

edited

Loading

reloxx13 commented Oct 13, 2018 •

edited

Loading

devyte left a comment

d-a-v commented Oct 14, 2018

reloxx13 commented Oct 14, 2018 •

edited

Loading

ascillato commented Oct 16, 2018 •

edited

Loading

ascillato left a comment

ascillato commented Oct 16, 2018

ascillato commented Oct 16, 2018

Jason2866 commented Oct 16, 2018 •

edited

Loading

andrethomas commented Oct 16, 2018

devyte commented Oct 17, 2018

fix parseArgument #5230

fix parseArgument #5230

Conversation

d-a-v commented Oct 11, 2018 • edited Loading

devyte commented Oct 11, 2018

earlephilhower commented Oct 11, 2018 • edited Loading

d-a-v commented Oct 11, 2018

earlephilhower left a comment

Choose a reason for hiding this comment

d-a-v commented Oct 11, 2018 • edited Loading

ascillato commented Oct 11, 2018 • edited Loading

reloxx13 commented Oct 11, 2018 • edited Loading

ascillato commented Oct 11, 2018

d-a-v commented Oct 11, 2018

ascillato commented Oct 12, 2018 • edited Loading

reloxx13 commented Oct 13, 2018 • edited Loading

reloxx13 commented Oct 13, 2018 • edited Loading

devyte left a comment

Choose a reason for hiding this comment

d-a-v commented Oct 14, 2018

reloxx13 commented Oct 14, 2018 • edited Loading

ascillato commented Oct 16, 2018 • edited Loading

ascillato left a comment

Choose a reason for hiding this comment

ascillato commented Oct 16, 2018

ascillato commented Oct 16, 2018

Jason2866 commented Oct 16, 2018 • edited Loading

andrethomas commented Oct 16, 2018

devyte commented Oct 17, 2018

d-a-v commented Oct 11, 2018 •

edited

Loading

earlephilhower commented Oct 11, 2018 •

edited

Loading

d-a-v commented Oct 11, 2018 •

edited

Loading

ascillato commented Oct 11, 2018 •

edited

Loading

reloxx13 commented Oct 11, 2018 •

edited

Loading

ascillato commented Oct 12, 2018 •

edited

Loading

reloxx13 commented Oct 13, 2018 •

edited

Loading

reloxx13 commented Oct 13, 2018 •

edited

Loading

reloxx13 commented Oct 14, 2018 •

edited

Loading

ascillato commented Oct 16, 2018 •

edited

Loading

Jason2866 commented Oct 16, 2018 •

edited

Loading