module: fix recently introduced coverity warning

[mhdawson]

No description provided.

[mhdawson]

Coverity warning - 3. Condition error, taking false branch.
   	4. Condition simdjson::simdjson_result<simdjson::fallback::ondemand::object>(document.get_object()).get(main_object), taking false branch.
141  if (error || document.get_object().get(main_object)) {
142    return throw_invalid_package_config();
143  }
144
   	5. write_zero_model: raw_json_string sets key.buf to nullptr. [[show details](https://scan9.scan.coverity.com/eventId=9481392-7&modelId=9481392-0&fileInstanceId=124214292&filePath=%2Fdeps%2Fsimdjson%2Fsimdjson.h&fileStart=38692&fileEnd=38692)]
145  simdjson::ondemand::raw_json_string key;
146  simdjson::ondemand::value value;
147  std::string_view field_value;
148  simdjson::ondemand::json_type field_type;
149
   	6. Iterating over another element of main_object.
150  for (auto field : main_object) {
151    // Throw error if getting key or value fails.
   	7. Condition simdjson::simdjson_result<simdjson::fallback::ondemand::raw_json_string>(field.key()).get(key), taking false branch.
   	8. Condition simdjson::simdjson_result<simdjson::fallback::ondemand::value>(field.value()).get(value), taking false branch.
152    if (field.key().get(key) || field.value().get(value)) {
153      return throw_invalid_package_config();
154    }
155
   	
CID 331027 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
9. var_deref_model: Passing key to operator ==, which dereferences null key.buf. [[show details](https://scan9.scan.coverity.com/eventId=9481392-13&modelId=9481392-1&fileInstanceId=124214292&filePath=%2Fdeps%2Fsimdjson%2Fsimdjson.h&fileStart=44376&fileEnd=44378)]
156    if (key == "name") {

[mhdawson]

I had tried to do the check in the earlier check for an invalid package config. That caused failures so I think that means that key.raw() is null in some cases and even though we dereffed those we got lucky because the contents never mached what we were checking against.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/55960/

[lemire]

@mhdawson

I think that means that key.raw() is null in some cases

The raw() call should not return null because field.key().get(key) returns simdjson::SUCCESS (0). It could be null right before the previous check, but not after. This being said, the new check is very cheap so it not objectionable. But if you do have the case where it happens, if it does happen, I'd be very interested in checking it out.

[lemire]

The new check is cheap. It should never fire in practice.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56033/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/56035/

[jkeiser]

It really shouldn't be null... I wonder if the coverage tool got confused by the ||?

if (field.key().get(key) || field.value().get(value)) {

That said, the extra check isn't expensive or super objectionable (certainly don't let it get in the way of writing nodejs). It's just either unnecessary or Very Very Bad. If a successful key can point to null then a while lot of things will go wrong in a lot of people's code. Might be a SIMDJSON_ASSUME needs to be inserted to keep coverity sane.

[jkeiser]

(I'll take a look at the simdjson side to see if there are any holes anywhere, but don't let that stop this train.)

[jkeiser]

Looking through it, the for loop (for (auto field : main_object) {) should initialize field.key when * is called on the iterator. Ultimately that code should call this:

  const uint8_t *key = _json_iter->return_current_and_advance();
  if (*(key++) != '"') { return report_error(TAPE_ERROR, "Object key is not a string"); }
  return raw_json_string(key);

If coverity was checking the simdjson code itself I might expect it to squawk at *(key++). Is it possible it is wondering if key might be 0xFFFFFFFFFFFFFFFF before being incremented? We know this absolutely cannot be the case because the string is padded. We might be able to head it off with something like SIMDJSON_ASSUME(key != nullptr) there ...

[jkeiser]

If this theory is correct, coverity should trigger anytime you ever try to access a key while iterating over a simdjson object. Adding a SIMDJSON_ASSUME(key != nullptr) in there, or some kind of ASSUME that tells coverity there are at least SIMDJSON_PADDING bytes left in the string (so that it knows ++ won't overflow the pointer), should be a good fix in simdjson.h.

The if () you added will suffice, though I hope the compiler can be told that it's not an issue so that it can optimize this (and other similar checks) away. That's one of the great things about having padding.

[jkeiser]

@mhdawson how does one reproduce this? The CI checks in main are succeeding--does coverity run on this code in CI or does one need to run something extra when compiling?

[mhdawson]

@jkeiser coverity runs as a separate job in - https://ci.nodejs.org/job/node-daily-coverity/.

Unfortunately, we've not found a way to easily run it locally.

What I think you could do is add the null check in the earlier line

if (field.key().get(key) || field.value().get(value)) {
...
}

and then run the Node.js test suite (make node test)

That is what I had tried to do earlier and which resulted in failures when I ran the Node.js test suite. If you see the failures it should confirm that the null can happen in some cases.

I'll land this which will at least quiet the coverity warning and if you find I messed up my earlier check you can submit a PR to move the code into a better way of addressing the warning.

[mhdawson]

Landed in d4bcdd8

[lemire]

@jkeiser I'll run a test following @mhdawson's instructions this morning.

Note that it is remarkably easy to build node. It is the old configure && make.

[jkeiser]

Yeah, I really want to know if null is possible, because it's not designed to be possible--this is either a bug or a limitation with coverity. Particularly the key of a field: you absolutely shouldn't be given a field object in the first place unless there is a quote, and if there is a quote there is definitely enough padding and a whole string (otherwise the parser.iterate call would have failed.

[jkeiser]

(And in the meantime merging is the right thing.)

[lemire]

Here is what I just did. I got the latest node code (from the main branch). I made the following change...

index 9217b94852e..41359b0de1b 100644
--- a/src/node_modules.cc
+++ b/src/node_modules.cc
@@ -155,8 +155,11 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
 
     // based on coverity using key with == derefs the raw value
     // avoid derefing if its null
-    if (key.raw() == nullptr) continue;
-
+//    if (key.raw() == nullptr) continue;
+    if (key.raw() == nullptr) {
+      printf("bug with '.*s'\n", package_config.raw_json.size(), package_config.raw_json.data());
+      abort();
+    }
     if (key == "name") {
       // Though there is a key "name" with a corresponding value,
       // the value may not be a string or could be an invalid JSON string

I typed...

./configure -C --ninja && make -j32

This takes a few minutes (but only the first time, if you have already built the project, making changes and hitting make again is fast).

I then typed...

make test

(To go faster do make test -j32.)

This is on a Linux and AVX-512 capable x64 box. GCC 12.

I get one failure that is unrelated...

=== release test-strace-openat-openssl ===                                    
Path: parallel/test-strace-openat-openssl
node:assert:408
    throw err;
    ^

AssertionError [ERR_ASSERTION]: /etc/crypto-policies/back-ends/opensslcnf.config is not in the list of allowed openat calls
    at Interface.<anonymous> (/home/dlemire/CVS/github/node/test/parallel/test-strace-openat-openssl.js:46:5)
    at Interface.emit (node:events:519:28)
    at [_onLine] [as _onLine] (node:internal/readline/interface:416:12)
    at [_normalWrite] [as _normalWrite] (node:internal/readline/interface:610:22)
    at Socket.ondata (node:internal/readline/interface:243:23)
    at Socket.emit (node:events:531:35)
    at addChunk (node:internal/streams/readable:559:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
    at Readable.push (node:internal/streams/readable:390:5)
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

The end result is...

@mhdawson Do you find anything wrong in what I described above?

My current hypothesis is that it is a false positive. I think that coverity thinks that we can dereference a null pointer, but that's because it fails to do a complete analysis.

There are false positives with coverity: https://community.synopsys.com/s/article/REVERSE-INULL-False-Positive-Dereference-before-NULL-check So it would not be surprising if it were a false positive.

[marco-ippolito]

to land on v20 dependes on #50965