Avoid keeping pointer to temporary strings

[estyrke]

substr() creates a temporary string which will be deallocated, leaving the c_str() pointer pointing to garbage.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

---

This change is 

[pyu10055]

Reviewable status: 0 of 1 approvals obtained (waiting on @estyrke)

---

tfjs-node/binding/tfjs_backend.cc, line 1142 at r1 (raw file):

      input_tensor_index = 0;
    } else {
      input_tensor_index = atoi(input_op_index.c_str());

Is the concern that the temporary string was not disposed? If so here the temporary string is created?

[estyrke]

Reviewable status: 0 of 1 approvals obtained (waiting on @estyrke)

tfjs-node/binding/tfjs_backend.cc, line 1142 at r1 (raw file):

      input_tensor_index = 0;
    } else {
      input_tensor_index = atoi(input_op_index.c_str());

Is the concern that the temporary string was not disposed? If so here the temporary string is created?

Not exactly. The problem is on this line:

const char *input_op_index = name.substr(index + 1).c_str();

The expression name.substr(index + 1) creates a temporary variable whose scope ends at the end of the "full-expression" (essentially the end of this line). The c_str() call then takes the address of the string pointed to by this temporary variable. Thus, after this line, the pointer in input_op_index points to an already deallocated memory area. When the pointer is then used on the line you pointed out, the result is undefined. In most cases, the memory contents may still be there and no problem is observed. But if the deallocated memory area has already been cleared or reused, the atoi (or the strlen before it) will fail. This is what we observed on our compiler (VS 2019) - the strlen returns 0 and the output_tensor_index (in our case) will always be zero no matter which output we try to select.

In general, it is risky to hold on to the value returned by c_str, since it is an unmanged C pointer which will be undefined if used after its backing std::string has been deallocated - It is better to only call it "temporarily" when it is needed, such as within the atoi call here.

[estyrke]

VC2019 prints a warning to this effect, unfortunately I don't have a windows computer at hand now so I can't quote it.

[pyu10055]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @estyrke)

---

tfjs-node/binding/tfjs_backend.cc, line 1142 at r1 (raw file):

Previously, estyrke (Emil Styrke) wrote…

Not exactly. The problem is on this line:

const char *input_op_index = name.substr(index + 1).c_str();

The expression name.substr(index + 1) creates a temporary variable whose scope ends at the end of the "full-expression" (essentially the end of this line). The c_str() call then takes the address of the string pointed to by this temporary variable. Thus, after this line, the pointer in input_op_index points to an already deallocated memory area. When the pointer is then used on the line you pointed out, the result is undefined. In most cases, the memory contents may still be there and no problem is observed. But if the deallocated memory area has already been cleared or reused, the atoi (or the strlen before it) will fail. This is what we observed on our compiler (VS 2019) - the strlen returns 0 and the output_tensor_index (in our case) will always be zero no matter which output we try to select.

In general, it is risky to hold on to the value returned by c_str, since it is an unmanged C pointer which will be undefined if used after its backing std::string has been deallocated - It is better to only call it "temporarily" when it is needed, such as within the atoi call here.

got it, thanks for the explanation.