A common source of remote exploits in Nim is that of enum conversions done like so:
type X = enum
Z
var a = 42
var b = X(a)
This will fail with a RangeDefect which subsequently crashes the application - unfortunately it gets used liberally in code we review, often when dealing with untrusted data, for example coming from the network or a file.
The (fairly) safe way around this is of course a case expression:
case a of
ord(Z): Z
else: ...
but it gets tedious - any better alternatives out there? A few non-starters include try/catch (Defect cannot be caught in some dialects of Nim), low/high checks (doesn't work with gappy enums).
Winner gets an entry in the style guide :)
Stealing a macro from the std/setutils in devel, got an ok implementation for holey and non-holey enums:
import std/macros
type
X = enum
Z
type
HoleyEnum = enum
AVal = 3
BVal = 5
macro enumElementsAsSet(enm: typed): untyped = result = newNimNode(nnkCurly).add(enm.getType[1][1..^1])
proc toEnum*(val: SomeInteger, E: typedesc[enum]): E =
const enmRange = E.low.ord .. E.high.ord
when E is Ordinal:
if val in enmRange:
E(val)
else:
raise (ref ValueError)(msg: $val & " cannot be converted to the enum: " & $E)
else:
if val in enmRange and val.E in E.enumElementsAsSet:
E(val)
else:
raise (ref ValueError)(msg: $val & " cannot be converted to the enum: " & $E)
var a = 42
let b = a.toEnum(X)
let c = 4.toEnum(HoleyEnum)
Just trolling, but that's why in Go, type casts also return a boolean for you to check, and since it's a tuple result, you have to handle it instead of chaining your value to something else. Couldn't this be implemented in a macro returning an Option[X]?
type X = enum
Z
var a = 42
var b = a.toEnum(X)
if b.isSome:
# use b
Well it does not even need a macro the above works fine simply modified.
import std/options
proc toEnum*(val: SomeInteger, E: typedesc[enum]): Option[E] =
const enmRange = E.low.ord .. E.high.ord
when E is Ordinal:
if val in enmRange:
some(val)
else:
none(E)
else:
if val in enmRange and val.E in E.enumElementsAsSet:
some(val)
else:
none(E)
My submission:
import macros, fusion/astdsl
macro toEnumImpl(x, res: typed): untyped =
template raiseInvalidConv() =
raise newException(ValueError, $x & "' can't be converted to " & $typeSym)
let typeSym = getTypeInst(res)
let typeNode = getTypeImpl(typeSym)
result = buildAst(stmtListExpr):
let tmp = genSym(nskVar, "toEnumResult")
varSection(identDefs(tmp, typeSym, empty()))
caseStmt(x):
for i in 1..<typeNode.len:
let field = newLit(typeNode[i])
ofBranch(newCall(getTypeInst(x), field)):
asgn(tmp, field)
`else`:
getAst(raiseInvalidConv())
tmp
proc toEnum*[T: enum](x: SomeInteger, t: typedesc[T]): T =
toEnumImpl(x, result)
type
Foo = enum
bar, baz
let x = toEnum(2, Foo)
Can't fix that mysterious type mismatch: got <ref NimNodeObj> but expected 'int' error but once I do, I will update it.
I'm a bit puzzled to see this question coming from you of all people. This is a basic pattern of a failable data conversion which calls for the Result type.
Am I not getting something? What's the catch?
https://github.com/status-im/nim-stew/blob/master/stew/results.nim
Just a tip, you can use
raise newException(ValueError, "msg goes here")
Thanks all :)
I realize now this is probably too much for your style guide :D
I realize that a macro might be the least bad option here :)
if val in enmRange and val.E
@ElegantBeef this performs the E conversion before it's checked as being part of the set, which a little bit ... defeats the purpose ;) more to the point though, enmRange isn't valid for a holey enum - try your code with a valid ordinal :)
fusion/astdsl
@planetis Nice! I'd probably go for a fusion-less version of that, fusion is difficult to depend on in a larger project. Another small detail is that the generated AST is in the form var x; case .. of ..: x = .. - it doesn't really matter for trivial cases like this, but I'd probably strive to keep the let x = case ... form for the generated code which lets the compiler know that this is a total conversion and for example avoid the zero-init.
This is a basic pattern of a failable data conversion which calls for the Result type.
Yes, I'd of course use a Result/Option but it's an unfortunate reality that the standard library uses ValueError for parsing failures in general, so it was easier to ask the question this way. The focus is really on good tricks to determine the valid set of ordinals in the presence of gaps - after that, the world is your oyster. https://github.com/arnetheduck/nim-result/ might be more convenient if you don't want all of stew (though stew comes with a bunch of Result-based utilities for all kinds of things like IO).
raise newException(ValueError, "msg goes here")
I like to rant about this anti-pattern, so here goes: it hides the true type of the exception (ref ValueError), it doesn't allow conveniently setting other fields (parent or custom fields etc), and it's more typing.
This will fail with a RangeDefect which subsequently crashes the application - unfortunately it gets used liberally in code we review, often when dealing with untrusted data, for example coming from the network or a file.
This isn't good, of course -- but that leads to DoS, right? or can it lead to worse outcomes? (not that DoS is acceptable....)
more to the point though, enmRange isn't valid for a holey enum - try your code with a valid ordinal
Uncertain the implication here. The enmRange is the valid integer range of values, so the following set removes all the improper values. Though I do concede the early conversion is silly.
Unfortunately, changing the exception kind or introducing a new exception kind is a significantly breaking change - this is the enum we have to live with.
I suspect the best thing we can do probably is ensure nobody ever actually uses the "plain" conversion: https://github.com/nim-lang/Nim/issues/18430
The distinction between catchable exceptions and defects
Yes, it's quite arbitrary - at best, they're named Defect and Error though some of the standard library doesn't even do that leading to confusion over what can/should be caught and what cannot.
The other related issue here is that except: catches the "uncatchable" exceptions also depending on the dialect of Nim - ie the manual says one thing, the compiler does different things depending on flags passed to it, and different exceptions are raised, also depending on flags. It's quite a mess to try to use exceptions sanely in Nim, specially when creating libraries that don't have control over which dialect is used.
Also, when the compiler raises Defect, it also does so through a different mechanism that when user code raises Defect - this introduces yet more spice into the bug sauce.
I think that regarding the possibility to catch defects, we are in a transitional state. By default, defects are catchable but this will change in the future. Personally, I never rely on the possibility to catch defects. That means that if I want to check for overflow, I don’t try to do the operation and catch the exception, but I do some inefficient tests to prevent the overflow. I find this ugly but, at least, there is a good reason to use a defect here: it is more efficient.
In other cases, doing a check before doing the operation is not a problem, for instance before dividing an integer or before indexing, because the check is easy to write and implied by the logic of the program. If the divisor may be zero or the index out of range without this being an error, it is normal to check these cases.
As regards the conversion to enums, I don’t think that making them raise a catchable exception rather than a defect will be such a breaking change. The breaking change has already be done been when exceptions have been split in two categories using a different mechanism. Switching wouldn’t break a lot of thing. And, as I said, parseEnum already raises a ValueError not a RangeDefect.
For non-holy enums you can always do something like this:
type X = enum
Z
template toEnum[T](x: int): T =
if x in T.low.int..T.high.int:
T(x)
else:
raise newException(ValueError, "Value not convertible to enum")
var a = 42
echo toEnum[X](a)
To be honest a isValidFor function that takes an enum type would be nice to have in the stdlib.
To be honest a isValidFor function that takes an enum type would be nice to have in the stdlib.
Nice! I'd probably go for a fusion-less version of that, fusion is difficult to depend on in a larger project.
Strong disagree, its a matter of doing koch fusion or nimble install fusion. Btw are you going to announce the results?
nimble install fusion
We discourage developers from using nimble install since it installs a random version in a hidden location and changes dependency resolution in nimble in general for all projects, making it hard to reproduce builds (or even successfully make one), work on different versions of the same project, work on multiple projects etc. It's a significant anti-pattern in non-solo/non-script development.
Btw are you going to announce the results?
lol for sure, just need to weight them all together - lots of good points raised, including how to eventually improve the language.
it created a less silly implementation, but still not ideal in my opinon.
We discourage developers from using nimble install since it installs a random version in a hidden location and changes dependency resolution in nimble in general for all projects, making it hard to reproduce builds (or even successfully make one), work on different versions of the same project, work on multiple projects etc. It's a significant anti-pattern in non-solo/non-script development.
I guess, proposing for astdsl to be included in Nim's stdlib, is not going to work for you either, since you're stuck on an older Nim version. ./koch fusion used to git checkout on a fixed commit which was perfect for your reproducible builds, perhaps you/we can ask for the commit that removed this: https://github.com/nim-lang/Nim/pull/16925 to be reverted?