common: admin socket fallback to json-pretty format

[ghost]

If the format argument to a command sent to the admin socket is not
among the supported formats ( json, json-pretty, xml, xml-pretty ) the
new_formatter function will return null and the AdminSocketHook::call
function must fall back to a sensible default.

The CephContextHook::call and HelpHook::call failed to do that and a
malformed format argument would cause the mon to crash. A check is added
to each of them and fallback to json-pretty if the format is not
recognized.

To further protect AdminSocketHook::call implementations from similar
problems the format argument is checked immediately after accepting the
command in AdminSocket::do_accept and replaced with json-pretty if it is
not known.

A test case is added for both CephContextHook::call and HelpHook::call
to demonstrate the problem exists and is fixed by the patch.

http://tracker.ceph.com/issues/7378 fixes #7378

Backport: emperor, dumpling
Signed-off-by: Loic Dachary loic@dachary.org

[ghost]

backport in progress

[liewegas]

I'm a bit worried that there are a zillion callers for new_formatter. What if we added a second argument that was the fallback/default? And if it is null/empty then we are allowed to return null? Or do the other callers appear to be safe?

[ghost]

Line numbers are relative to b9a127e

• src/client/Client.cc:113 is not safe
• src/osdc/Objecter.cc:2509 is not safe
• src/osd/OSD.cc:912 is not safe because dump_ops_in_flight does not test if f is null
• src/mon/AuthMonitor.cc contains > 10 if to deal with the fact that f is null or not
• src/mon/MDSMonitor.cc contains < 10 if to deal with the fact that f is null or not
• src/mon/Monitor.cc contains > 10 if to deal with the fact that f is null or not
• src/mon/MonmapMonitor.cc contains < 10 if to deal with the fact that f is null or not
• src/mon/OSDMonitor.cc:2195 and src/mon/OSDMonitor.cc:2218 have an unsafe call where there is no risk to reach it http://tracker.ceph.com/issues/7388
• src/mon/Monitor.cc contains > 10 if to deal with the fact that f is null or not
• src/mon/PGMonitor.cc contains < 10 if to deal with the fact that f is null or not
• src/osd/OSD.cc contains < 10 if to deal with the fact that f is null or not
• src/osd/ReplicatedPG.cc contains < 10 if to deal with the fact that f is null or not
• src/test/bench/small_io_bench_fs.cc src/test/common/get_command_descriptions.cc src/test/common/test_sloppy_crc_map.cc and src/test/crush/indep.cc use hard-coded arguments

There is a total ol 44 call to new_formatter found in master but it appears that there are various strategies to deal with the return of a null formatter, a number of which are custom. For instance osd dump will print() if the formatter is null while osd crush rule list will fall back to json_pretty.

Adding an argument to new_formatter would require to go over all calls to new_formatter, add NULL where it appears to handle the case where it returns null and add the default where it does not. Another solution is to modify the three calls that are unsafe.

[ghost]

@liewegas pushed with fallbacks for the three unsafe calls mentionned above