`ptr char` implicitly converts to cstring, resulting in undefined behavior

[timotheecour]

• ptr char implicitly converts to cstring, resulting in undefined behavior
• ditto with ptr array[N, char] and ptr UncheckedArray[N, char]
  this introduces a weird special case in the language (eg $a doesn't compile for other types a: ptr[T] except for T=char) as well as weird bugs and undefined behavior in seemingly safe code

Example 1

when true:
  import std/unittest
  var x: ptr char
  var s: cstring
  doAssert cstring isnot ptr char
  doAssert (ptr char) isnot cstring

  proc fun(a: cstring)=discard
  proc fun2(a: ptr char)=discard
  doAssert not compiles(fun2 cstring"asdf")
  doAssert not compiles(block: x = s)

  # BUG
  check not compiles(fun(x))
  check not compiles(echo(x))
  check not compiles($(x))
  check not compiles(block: s = x)

Current Output

/Users/timothee/git_clone/nim/timn/tests/nim/all/t10433.nim(15, 8): Check failed: not compiles(fun(x))
compiles(fun(x)) was true
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10433.nim(16, 8): Check failed: not compiles(echo(x))
compiles(echo(x)) was true
/Users/timothee/git_clone/nim/timn/tests/nim/all/t10433.nim(17, 8): Check failed: not compiles($(x))
compiles($(x)) was true

Expected Output

tests pass
ie, it should be illegal to do those implicit conversions from ptr char to cstring

Example 2

in case you may think it's a good idea to have implicit conversion from ptr char to cstring, consider this:

when true:
  proc fun(a, b: var openArray[char]) =
    echo a[0].addr # prints abcdef sometimes, or sometimes other garbage
  proc main()=
    var a = ['a','b', 'c']
    var b = ['d','e', 'f']
    fun(a,b)
  main()

you get (different) garbage results when recompiling the same program:

nim c -r $timn_D/tests/nim/all/t10433.nim
abc`UG
nim c -r $timn_D/tests/nim/all/t10433.nim
abc`%

Example 3

in other cases, you'll end up with buffer overflow bugs/attacks, since a ptr char could come from any source (eg ffi calls, malloc / createU, etc) with nothing that should guarantee that it's 0 terminated.

That problem is exacerbated when a ptr char field is nested somewhere deep inside an object and we are calling $ on it

Example 4

likewise, implicit conversion from ptr array[0..2, char] to cstring should be disallowed. It has the same caveats, eg:

when true:
  var vals = @[['a','b','c'], ['d','e','f']]
  echo vals[0]
  echo vals[0].addr # should not compile (unless `$` has an overload for ptr)
  var z: cstring = vals[0].addr # should not compile

prints

['a', 'b', 'c']
abcdef # error prone!

and ditto for ptr UncheckedArray, eg:

when true:
  var vals1 = @[['a','b','c'], ['d','e','f']]
  echo cast[ptr UncheckedArray[char]](vals1[0].addr) # prints

Possible Solution

• prevent implicit conversion (sigmatch) from X to cstring where X is not cstring (eg ptr char, ptr[array[N, char] etc)
• add an easy way to convert explicitly, eg:

template toCstring*(a: ptr char): cstring = cast[cstring](a)

• if the worry is about backward compatibility, then make it an error but allow a flag:
  --legacy:implicitPtrCharToCstring (adding to existing list --legacy:allowSemcheckedAstModification|checkUnsignedConversions)

Additional Information

• latest devel 0a49fe5 1.1.1
• since at least 0.17 or forever
• D20200327T225837

[Araq]

Conversions from ptr array to cstring should remain IMO but ptr char is a legacy oversight and I doubt it's documented anywhere.

[timotheecour]

Conversions from ptr array to cstring should remain IMO

no, it's a pointless security hole that is just waiting to be exploited in some service one day.

It has exact same undefined behavior, eg:

when true:
  const N=3
  proc fun(a, b: ptr array[N, char]) =
    echo a
  proc main() =
    var a = ['a','b', 'c']
    var b = ['d','e', 'f']
    fun(a.addr,b.addr)
  main()

with -d:danger you always print abc and think you're ok.
then you run in debug mode and now you get different garbage each time:

nimb c -r $timn_D/tests/nim/all/t10433.nim
abc4[ # <- this could leak passwords etc
nimb c -r $timn_D/tests/nim/all/t10433.nim
abcj

or in other situations it'd be the reverse; or maybe only in some architecture; or maybe in your released product

all we need is explicit conversion via the toCstring is showed above, nim is really good about this in other parts of the language, but somehow fails here.

There's zero downside to explicit conversion, only upsides; users can use explicit conversion when they know what they're doing, or even a converter if they don't want to bother, or perhaps --legacy:implicitPtrCharToCstring if that's implemented.

working on a PR.

[timotheecour]

and here's another "fun" gotcha illustrating dangers of implicit conversions:

nim c -r --experimental:implicitDeref main
before PR: fails (bad)
after PR: work

  proc fun[T](): int =
    var a: array[2, T]
    var b: ptr array[2, T] = a.addr
    b.len
  doAssert fun[int]() == 2
  doAssert fun[int8]() == 2
  doAssert fun[char]() == 2 # fails

(and a O(1) operation became O(N)...)

it reminds me of the classic mistake in C++ which was specializing std::vector<bool>, causing in the end a lot more harm than good, when it should've been std::bitvector instead

[krux02]

@timotheecour You don't need to convince me any more about the danger of implicit conversions, especially not in this case. I am all on your side here. Just disable inplicit conversion to cstring, fix the problems with it and be done with it. Don't use this as an opportinity to add more stuff to system.nim. That behavior is very annoying.

[iacore]

implicit cast from ptr array to cstring is just weird. cstring has terminating null byte, and array[N, char] may not have the null byte.

To solve this problem, Zig has another datatype: "array with sentinel value". In Nim, the (imagined) type equivalent to cstring will be sentinelArray[N, char, '0'], meaning the array is terminated by a null byte that does not count in .len.