JSONFormattable incorrectly sets a string starting with digit as int #35870
Conversation
soumyakoduri
changed the title
|
@soumyakoduri the passed string can also be a json structure, removing the Referring to bitcoin/bitcoin@181771b , I have made similar changes to fix json_spirit::read_string itself. Kindly review the changes. |
I was wondering about the same too. But I wasn't able to test multiple array objects (like 'acls') set in a single string to validate this. So can we alternatively - use decode_json for data of obj_type & array_type and for rest of the types, copy string in the same format? Does it sound okay? |
|
retest this please |
|
json_spirit is used in a lot of other components in ceph, including rbd osd mon and mgr - it's hard to tell what impact this change will have on them |
okay...As the main issue is in the json_spirit template , this seemed right way to fix it (I referred to bitcoin/bitcoin@181771b ). Do you suggest we add special checks in "JSONFormattable" structure (which seem to be used in only rgw sync module code), as a workaround for now? |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution! |
@yehudasa @cbodley ..so for this particular issue, do you suggest we workaround it in rgw layer itself? |
|
@soumyakoduri if we're not fixing json-spirit, we could perhaps just use the newer, seemingly better json parsing in RGW (i.e., rapid-json)? |
soumyakoduri
force-pushed
the
json_fix
branch
from
db127e4
to
ba66e52
Compare
soumyakoduri
force-pushed
the
json_fix
branch
2 times, most recently
from
a215b31
to
417a25b
Compare
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
unstale |
|
jenkins test windows |
|
jenkins test make check |
|
jenkins test make check arm64 |
soumyakoduri
force-pushed
the
json_fix
branch
2 times, most recently
from
1d21a22
to
25f3049
Compare
|
looks great, thanks. just needs a qa run? |
Started 'rgw' tests - http://pulpito.front.sepia.ceph.com/soumyakoduri-2022-04-11_08:46:28-rgw-wip-skoduri-json-fix-distro-basic-smithi/ Please let me know if it needs any other test-suite run. |
|
@vshankar @idryomov @neha-ojha @epuertat can you please help us evaluate this potentially-breaking fix to given input like it's hard to know what this change could break, so i didn't want to merge it before the quincy release. but now seems like a good time to try it, with a whole release cycle to identify any regressions does anyone object to this approach? if not, could i request your assistance in running this through your teuthology suites? |
JSONFormattable::set calls JSONParser::parse() to check if the given string is a valid JSON object, which internally uses json_spirit::read to determine the string type and copy it into data. This routine incorrectly sets data type to integer if the string starts with digit and copies only the first set of numeric values of the string. To work-around this issue, verify if the entire string is parsed to determine if its valid json data type. Signed-off-by: Soumya Koduri <skoduri@redhat.com>
|
jenkins test make check arm64 |
|
jenkins test api |
|
jenkins test windows |
|
latest run (for rgw) - pulpito.front.sepia.ceph.com/soumyakoduri-2022-07-22_16:46:59-rgw-json_fix-distro-default-smithi/ |
|
jenkins test api |
|
@cbodley ceph/api tests have failed with error reported in https://tracker.ceph.com/issues/53582 |
|
jenkins test api |
JSONFormattable::set calls JSONParser::parse() to check if the given string
is a valid JSON object, which internally uses json_spirit::read to determine
the string type and copy it into data.
This routine incorrectly sets data type to integer if the string starts with
digit and copies only the first set of numeric values of the string.
For eg:
radosgw-admin zone modify --rgw-zone=cloud-zone --rgw-zonegroup=default --tier-config=connection.access_key=312CNV15CC2ZN44IPB24A
Output:
"tier_config": {
"connection": {
"access_key": 312,
"secret": "W0X3NbPXNW1B7Ru79xuuUI53ftSKieEl2ouuHP8C"
}
},
As can be seen in above output only first set of digits of the input string are copied. The same issue exists for even real & bool types.
The right fix seems to be to check if the entire string is read before processing the data value set.
Fixes: https://tracker.ceph.com/issues/55212