deprecate implicit conversion string => cstring

[timotheecour]

while reviewing https://github.com/nim-lang/Nim/pull/16320 I noticed a dangerous pattern was used:

proc rasPrivateDecrypt(fr: cstring): cstring =
  ...
  var to = newString(rsaLen)
  return to

The compiler allows this even though there's a use-after-free here, as illustrated below:

proc fn(): cstring =
  let n = 10
  result = newString(n)
  for i in 0..<n:
    result[i] = 'a'

proc main =
  let s = fn()
  # GC_fullCollect()
  echo (s,)
main()

nim r main ("aaaaaaaaaa",) ditto with GC_fullCollect() uncommented, for some reason

nim r --gc:arc main ("(\"",)

proposal

disallow implicit string => cstring conversion, by issuing a warning for now, and maybe an error at a later time. The explicit conversion is fine, eg:

let s = "abc"
var s1: cstring
s1 = s # issue warning
s1 = s.cstring # ok

This may affect potentially a lot of code, but the fix is trivial and could be automated with nimfix/nimlint if needed, or at least helped with. And we'd issue a warning, not an error, so code would not break, but warnings could reveal potential bugs, like above, which are pernicious because they get revealed depending on environmental conditions / compiler flags, potentiallly GC load etc.

related

nim is usually good at avoiding implicit conversions, with great benefits, but cstring in particular is too loose, see also these cases https://github.com/timotheecour/Nim/issues/454

[Araq]

We really need to do this. With a deprecation period --allowOldCstrings and probably also with special casing cases that are known to be safe statically. Conversion of string literals to cstring comes to mind, but there are other cases, like passing a string to C-style varargs (which cannot realistically take ownership).

[ringabout]

This was implemented. Now it gives a warning like Warning: implicit conversion to 'cstring' from a non-const location: s; this will become a compile time error in the future [CStringConv]