Skip to content

Regenerate payloads for cgroups tests using string2printable#11560

Merged
oranagra merged 2 commits intoredis:unstablefrom
enjoy-binbin:fix_cgroups_test_payload
Dec 1, 2022
Merged

Regenerate payloads for cgroups tests using string2printable#11560
oranagra merged 2 commits intoredis:unstablefrom
enjoy-binbin:fix_cgroups_test_payload

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented Dec 1, 2022

The test failed with ERR DUMP payload version or checksum are wrong.
And it only fails on CentOS, this is due to the fact that tcl8.5 does not correctly
parse the hexadecimal abbreviation. And in Ubuntu we are using tcl8.6.

Payload regenerated using string2printable, introduced in #11099

The test failed with `ERR DUMP payload version or checksum are wrong.`
And it only fails on CentOS, i guess this payload is not suitable on
CentOS.

Payload regenerated using string2printable, introduced in redis#11099
@enjoy-binbin
Copy link
Contributor Author

@sundb
Copy link
Collaborator

sundb commented Dec 1, 2022

This is due to the fact that tcl8.5 does not correctly parse the hexadecimal abbreviation.

@enjoy-binbin
Copy link
Contributor Author

enjoy-binbin commented Dec 1, 2022

oh, so that's the case, I vaguely remember seeing this question before, thanks for the tip

btw, why only centos, can you recall it? I guess all our CIs use tcl8.5

@sundb
Copy link
Collaborator

sundb commented Dec 1, 2022

some is using 8.6 and others are not, like ubuntu

 sudo apt-get install tcl8.6 tclx
  shell: /usr/bin/bash -e {0}

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, TCL 8.5 can only correctly handle \x if the entire string is escaped. it can't mix escaping and plain chars in the same string.

but i see this payload was completely changed (started with \x13 and now it starts with \x15).
i.e.

  1. it's not that we just fixed the escaping, it's a different content.
  2. how did we generate it? is it still the old RDB format? seems like we changed it from RDB_TYPE_STREAM_LISTPACKS_2 to RDB_TYPE_STREAM_LISTPACKS_3

@enjoy-binbin
Copy link
Contributor Author

enjoy-binbin commented Dec 1, 2022

how did we generate it?

i used a trick, like i used to do in corrupt payload one.
Fistr, the payload is ok in redis-cli
i start the test, add a after 20000, something like this in the test, make the test server hang
then, i use a redis-cli to connect the test server, and restore the payload
after that, we can a a print in the test, after the after 20000, dump the key, and use string2printable

and now i see, the rdb type is the wrong one
i regenerated using the above way, make sure the rdb type is the RDB_TYPE_STREAM_LISTPACKS_2
(added some serverLog and debug log to verified it)

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good that i caught it, otherwise this test would be completely missing its purpose. 8-)

p.s. you could also just use TCL8.6 to pass that string through string2printable

@guybe7
Copy link
Collaborator

guybe7 commented Dec 1, 2022

was a bit quick on the trigger with my +1 😂

@oranagra oranagra merged commit 79fe450 into redis:unstable Dec 1, 2022
@enjoy-binbin enjoy-binbin deleted the fix_cgroups_test_payload branch December 1, 2022 07:12
madolson pushed a commit to madolson/redis that referenced this pull request Apr 19, 2023
…1560)

The test failed with ERR DUMP payload version or checksum are wrong.
And it only fails on CentOS, this is due to the fact that tcl8.5 does not correctly
parse the hexadecimal abbreviation. And in Ubuntu we are using tcl8.6.
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
…1560)

The test failed with ERR DUMP payload version or checksum are wrong.
And it only fails on CentOS, this is due to the fact that tcl8.5 does not correctly
parse the hexadecimal abbreviation. And in Ubuntu we are using tcl8.6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants