Skip to content

Add libjansson compile-time validation and support header#1004

Open
pjungkamp wants to merge 3 commits intomasterfrom
jansson-validator
Open

Add libjansson compile-time validation and support header#1004
pjungkamp wants to merge 3 commits intomasterfrom
jansson-validator

Conversation

@pjungkamp
Copy link
Contributor

Description

I've started to look into the places where we are parsing/unpacking JSON values and noticed several subtle errors. There are various places where the format strings are not matching the argument lists, so I've implemented a "quick-and-dirty" compile-time validated wrapper around json_unpack and json_pack that raises compile errors when the format string is not matching the arguments.

Compatiblility with #1001

I'm still working on #1001 where I want to move the configuration parsing from libjansson to nlohmann::json but the JanssonPtr smart pointer and format string validators are a nice first step in improving our existing JSON parsing code. And especially the smart pointer is really useful for the incremental conversion discussed in #1001.

Examples

Incompatible argument type

Let's look at this initial change I did in common/lib/hist.cpp:

-  json_hist = json_pack("{ s: f, s: f, s: i }", "low", low, "high", high,
-                        "total", total);
+  json_hist = janssonPack("{ s:f, s:f, s:i }", //
+                          "low", low,          //
+                          "high", high,        //
+                          "total", total)
+                  .release();

This is the error I received here:

/home/pjungkamp/Projects/VILLASframework/node/common/lib/hist.cpp:207:26: error: call to consteval function ‘villas::JanssonPackFormatString<const char*, double, const char*, double, const char*, long unsigned int>(((const char*)"{ s:f, s:f, s:i }"))’ is not a constant expression
  207 |   json_hist = janssonPack("{ s:f, s:f, s:i }", //
      |               ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
  208 |                           "low", low,          //
      |                           ~~~~~~~~~~~~~~~~~~~~~~~
  209 |                           "high", high,        //
      |                           ~~~~~~~~~~~~~~~~~~~~~~~
  210 |                           "total", total)
      |                           ~~~~~~~~~~~~~~~
In file included from /home/pjungkamp/Projects/VILLASframework/node/common/lib/hist.cpp:14:
/home/pjungkamp/Projects/VILLASframework/node/common/lib/hist.cpp:207:26:   in ‘constexpr’ expansion of ‘villas::JanssonPackFormatString<const char*, double, const char*, double, const char*, long unsigned int>(((const char*)"{ s:f, s:f, s:i }"))’
/home/pjungkamp/Projects/VILLASframework/node/common/include/villas/jansson.hpp:310:13:   in ‘constexpr’ expansion of ‘villas::JanssonPackFormatString<const char*, double, const char*, double, const char*, long unsigned int>::validate(fmt)’
/home/pjungkamp/Projects/VILLASframework/node/common/include/villas/jansson.hpp:444:11: error: expression ‘<throw-expression>’ is not a constant expression
  444 |           throw "Expected 'int' argument for 'i' specifier";
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The type of total is actually cnt_t (an alias to unsigned long) which is incompatible with int. The error isn't perfect, as it doesn't tell us which specific variable or format specifier is wrong here, but it is good enough as a basic direction.

Unused arguments

Let's look at this example in lib/nodes/webrtc.cpp:

  ret = json_unpack_ex(
      json, &err, 0, "{ s: s, s?: s, s?: s, s?: i, s?: i, s?: b, s?: o }",
      "session", &sess, "peer", &pr, "server", &svr, "wait_seconds",
      &wait_seconds, "max_retransmits", &rexmit, "ordered", &ord, "ice",
      &json_ice, "format", &json_format);

Can you spot the error? The "format", &json_format arguments do not have a corresponding specifier in the format string and thus were always just ignored.

This becomes more obvious after a little cleanup:

-  json_error_t err;
-  ret = json_unpack_ex(
-      json, &err, 0, "{ s: s, s?: s, s?: s, s?: i, s?: i, s?: b, s?: o }",
-      "session", &sess, "peer", &pr, "server", &svr, "wait_seconds",
-      &wait_seconds, "max_retransmits", &rexmit, "ordered", &ord, "ice",
-      &json_ice, "format", &json_format);
-  if (ret)
-    throw ConfigError(json, err, "node-config-node-webrtc");
+  janssonUnpack(json,
+                "{ s:s, s?s, s?s, s?i, s?i, s?b, s?{ s?o, s?b } }", //
+                "session", &sess,                                   //
+                "peer", &pr,                                        //
+                "server", &svr,                                     //
+                "wait_seconds", &wait_seconds,                      //
+                "max_retransmits", &rexmit,                         //
+                "ordered", &ord,                                    //
+                "ice",                                              //
+                /* ice */ "servers", &json_servers,                 //
+                /* ice */ "tcp", &tcp,                              //
+                "format", &json_format);

And the compiler helps me with this error here:

/home/pjungkamp/Projects/VILLASframework/node/lib/nodes/webrtc.cpp: In member function ‘virtual int villas::node::WebRTCNode::parse(json_t*)’:
/home/pjungkamp/Projects/VILLASframework/node/lib/nodes/webrtc.cpp:65:16: error: call to consteval function ‘villas::JanssonUnpackFormatString<const char*, const char**, const char*, const char**, const char*, const char**, const char*, int*, const char*, int*, const char*, int*, const char*, const char*, json_t**, const char*, int*, const char*, json_t**>(((const char*)"{ s:s, s?s, s?s, s?i, s?i, s?b, s?{ s?o, s?b } }"))’ is not a constant expression
   65 |   janssonUnpack(json,
      |   ~~~~~~~~~~~~~^~~~~~
   66 |                 "{ s:s, s?s, s?s, s?i, s?i, s?b, s?{ s?o, s?b } }", //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   67 |                 "session", &sess,                                   //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |                 "peer", &pr,                                        //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   69 |                 "server", &svr,                                     //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   70 |                 "wait_seconds", &wait_seconds,                      //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   71 |                 "max_retransmits", &rexmit,                         //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   72 |                 "ordered", &ord,                                    //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   73 |                 "ice",                                              //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   74 |                 /* ice */ "servers", &json_servers,                 //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   75 |                 /* ice */ "tcp", &tcp,                              //
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   76 |                 "format", &json_format);
      |                 ~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/pjungkamp/Projects/VILLASframework/node/lib/nodes/webrtc.cpp:14:
/home/pjungkamp/Projects/VILLASframework/node/lib/nodes/webrtc.cpp:65:16:   in ‘constexpr’ expansion of ‘villas::JanssonUnpackFormatString<const char*, const char**, const char*, const char**, const char*, const char**, const char*, int*, const char*, int*, const char*, int*, const char*, const char*, json_t**, const char*, int*, const char*, json_t**>(((const char*)"{ s:s, s?s, s?s, s?i, s?i, s?b, s?{ s?o, s?b } }"))’
/home/pjungkamp/Projects/VILLASframework/node/common/include/villas/jansson.hpp:157:13:   in ‘constexpr’ expansion of ‘villas::JanssonUnpackFormatString<const char*, const char**, const char*, const char**, const char*, const char**, const char*, int*, const char*, int*, const char*, int*, const char*, const char*, json_t**, const char*, int*, const char*, json_t**>::validate(fmt)’
/home/pjungkamp/Projects/VILLASframework/node/common/include/villas/jansson.hpp:290:7: error: expression ‘<throw-expression>’ is not a constant expression
  290 |       throw "Unused trailing arguments";
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de>
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.

1 participant