fixes #18550 (#18553)

* fixes #18550

* update the manual to reflect reality

9 files changed, 57 insertions, 11 deletions

diff --git a/changelog.md b/changelog.md
index ece88f710..f2a9ca914 100644
--- a/changelog.md
+++ b/changelog.md
@@ -32,6 +32,10 @@
   echo a1.ord.B # produces no warning
   ```
 
+- A dangerous implicit conversion to `cstring` now triggers a `[CStringConv]` warning.
+  This warning will become an error in future versions! Use an explicit conversion
+  like `cstring(x)` in order to silence the warning.
+
 - Type mismatch errors now show more context, use `-d:nimLegacyTypeMismatch` for previous
   behavior.
 
diff --git a/compiler/lineinfos.nim b/compiler/lineinfos.nim
index a3243d5fc..3f31ceb97 100644
--- a/compiler/lineinfos.nim
+++ b/compiler/lineinfos.nim
@@ -71,6 +71,7 @@ type
     warnCannotOpen = "CannotOpen",
     warnFileChanged = "FileChanged",
     warnSuspiciousEnumConv = "EnumConv",
+    warnCstringConv = "CStringConv",
     warnUser = "User",
     # hints
     hintSuccess = "Success", hintSuccessX = "SuccessX",
@@ -155,6 +156,7 @@ const
     warnCannotOpen: "cannot open: $1",
     warnFileChanged: "file changed: $1",
     warnSuspiciousEnumConv: "$1",
+    warnCstringConv: "$1",
     warnUser: "$1",
     hintSuccess: "operation successful: $#",
     # keep in sync with `testament.isSuccess`
diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim
index 1e1672cad..ce21c3ed2 100644
--- a/compiler/sempass2.nim
+++ b/compiler/sempass2.nim
@@ -951,6 +951,15 @@ proc trackInnerProc(tracked: PEffects, n: PNode) =
   else:
     for ch in n: trackInnerProc(tracked, ch)
 
+proc allowCStringConv(n: PNode): bool =
+  case n.kind
+  of nkStrLit..nkTripleStrLit: result = true
+  of nkSym: result = n.sym.kind in {skConst, skParam}
+  of nkAddr: result = isCharArrayPtr(n.typ, true)
+  of nkCallKinds:
+    result = isCharArrayPtr(n.typ, n[0].kind == nkSym and n[0].sym.magic == mAddr)
+  else: result = isCharArrayPtr(n.typ, false)
+
 proc track(tracked: PEffects, n: PNode) =
   case n.kind
   of nkSym:
@@ -1157,6 +1166,13 @@ proc track(tracked: PEffects, n: PNode) =
       if tracked.owner.kind != skMacro:
         createTypeBoundOps(tracked, n.typ, n.info)
   of nkHiddenStdConv, nkHiddenSubConv, nkConv:
+    if n.kind in {nkHiddenStdConv, nkHiddenSubConv} and
+        n.typ.skipTypes(abstractInst).kind == tyCstring and
+        not allowCStringConv(n[1]):
+      message(tracked.config, n.info, warnCstringConv,
+        "implicit conversion to 'cstring' from a non-const location: $1; this will become a compile time error in the future" %
+          [$n[1]])
+
     if n.len == 2:
       track(tracked, n[1])
       if tracked.owner.kind != skMacro:
diff --git a/compiler/sighashes.nim b/compiler/sighashes.nim
index f6df422e6..0ff97017f 100644
--- a/compiler/sighashes.nim
+++ b/compiler/sighashes.nim
@@ -14,7 +14,9 @@ from hashes import Hash
 import types
 
 proc `&=`(c: var MD5Context, s: string) = md5Update(c, s, s.len)
-proc `&=`(c: var MD5Context, ch: char) = md5Update(c, unsafeAddr ch, 1)
+proc `&=`(c: var MD5Context, ch: char) =
+  # XXX suspicious code here; relies on ch being zero terminated?
+  md5Update(c, unsafeAddr ch, 1)
 proc `&=`(c: var MD5Context, r: Rope) =
   for l in leaves(r): md5Update(c, l, l.len)
 proc `&=`(c: var MD5Context, i: BiggestInt) =
diff --git a/compiler/types.nim b/compiler/types.nim
index 798f872c2..f0bd99a17 100644
--- a/compiler/types.nim
+++ b/compiler/types.nim
@@ -1664,3 +1664,18 @@ proc lookupFieldAgain*(ty: PType; field: PSym): PSym =
     if result != nil: break
     ty = ty[0]
   if result == nil: result = field
+
+proc isCharArrayPtr*(t: PType; allowPointerToChar: bool): bool =
+  let t = t.skipTypes(abstractInst)
+  if t.kind == tyPtr:
+    let pointsTo = t[0].skipTypes(abstractInst)
+    case pointsTo.kind
+    of tyUncheckedArray:
+      result = pointsTo[0].kind == tyChar
+    of tyArray:
+      result = pointsTo[1].kind == tyChar and firstOrd(nil, pointsTo[0]) == 0 and
+        skipTypes(pointsTo[0], {tyRange}).kind in {tyInt..tyInt64}
+    of tyChar:
+      result = allowPointerToChar
+    else:
+      discard
diff --git a/doc/manual.rst b/doc/manual.rst
index f0bd85638..9f614c2cd 100644
--- a/doc/manual.rst
+++ b/doc/manual.rst
@@ -1380,10 +1380,9 @@ variadic proc, it is implicitly converted to `cstring` too:
 
 Even though the conversion is implicit, it is not *safe*: The garbage collector
 does not consider a `cstring` to be a root and may collect the underlying
-memory. However, in practice, this almost never happens as the GC considers
-stack roots conservatively. One can use the builtin procs `GC_ref` and
-`GC_unref` to keep the string data alive for the rare cases where it does
-not work.
+memory. For this reason, the implicit conversion will be removed in future
+releases of the Nim compiler. Certain idioms like conversion of a `const` string
+to `cstring` are safe and will remain to be allowed.
 
 A `$` proc is defined for cstrings that returns a string. Thus to get a nim
 string from a cstring:
diff --git a/lib/system/io.nim b/lib/system/io.nim
index 591ea3e92..d8b2ac741 100644
--- a/lib/system/io.nim
+++ b/lib/system/io.nim
@@ -630,8 +630,8 @@ const
         ""
     else:
       ""
-  FormatOpen: array[FileMode, string] = [
-    "rb" & NoInheritFlag, "wb" & NoInheritFlag, "w+b" & NoInheritFlag,
+  FormatOpen: array[FileMode, cstring] = [
+    cstring("rb" & NoInheritFlag), "wb" & NoInheritFlag, "w+b" & NoInheritFlag,
     "r+b" & NoInheritFlag, "ab" & NoInheritFlag
   ]
     #"rt", "wt", "w+t", "r+t", "at"
@@ -678,7 +678,7 @@ proc open*(f: var File, filename: string,
   ## This throws no exception if the file could not be opened.
   ##
   ## The file handle associated with the resulting `File` is not inheritable.
-  var p = fopen(filename, FormatOpen[mode])
+  var p = fopen(filename.cstring, FormatOpen[mode])
   if p != nil:
     var f2 = cast[File](p)
     when defined(posix) and not defined(nimscript):
@@ -711,7 +711,7 @@ proc reopen*(f: File, filename: string, mode: FileMode = fmRead): bool {.
   ## Default mode is readonly. Returns true if the file could be reopened.
   ##
   ## The file handle associated with `f` won't be inheritable.
-  if freopen(filename, FormatOpen[mode], f) != nil:
+  if freopen(filename.cstring, FormatOpen[mode], f) != nil:
     when not defined(nimInheritHandles) and declared(setInheritable) and
          NoInheritFlag.len == 0:
       if not setInheritable(getOsFileHandle(f), false):
diff --git a/lib/system/widestrs.nim b/lib/system/widestrs.nim
index a999c9167..8b08959b5 100644
--- a/lib/system/widestrs.nim
+++ b/lib/system/widestrs.nim
@@ -171,7 +171,7 @@ proc newWideCString*(s: cstring): WideCStringObj =
   result = newWideCString(s, s.len)
 
 proc newWideCString*(s: string): WideCStringObj =
-  result = newWideCString(s, s.len)
+  result = newWideCString(cstring s, s.len)
 
 proc `$`*(w: WideCString, estimate: int, replacement: int = 0xFFFD): string =
   result = newStringOfCap(estimate + estimate shr 2)
diff --git a/tests/misc/tconv.nim b/tests/misc/tconv.nim
index 94677b1bf..1cb4b4fbf 100644
--- a/tests/misc/tconv.nim
+++ b/tests/misc/tconv.nim
@@ -1,5 +1,5 @@
 discard """
-  matrix: "--warningAsError:EnumConv"
+  matrix: "--warningAsError:EnumConv --warningAsError:CStringConv"
 """
 
 template reject(x) =
@@ -84,3 +84,11 @@ block: # https://github.com/nim-lang/RFCs/issues/294
 
   reject: Goo(k2)
   reject: k2.Goo
+
+reject:
+  # bug #18550
+  proc f(c: char): cstring =
+    var x = newString(109*1024*1024)
+    x[0] = c
+    x
+