pkulchenko / serpent

Lua serializer and pretty printer.
Other
557 stars 78 forks source link

serpent.block fails with certain table keys #34

Open chrisgbk opened 6 years ago

chrisgbk commented 6 years ago
local test = {
    ["a"] = true,
    ["2147483648"] = true,
    ["9223372036854775808"] = true
}
print(serpent.block(test))

The sorting function invoked by serpent.block will try to convert string keys that are also integers into integers and pass them to format.

Lua 5.2 will reject an argument to format that is larger than a 32 bit signed integer bad argument #1 to 'format' (not a number in proper range)

Lua 5.3 will reject an argument to format that is larger than a 64 bit signed integer. bad argument #1 to 'format' (number has no integer representation)

This causes serpent.block to crash, as it doesn't validate the number falls within the bounds of format's limits. I would expect a graceful fallback in this situation to perhaps not sorting keys that can't be represented as signed integers as integers, and sorting as strings instead.

pkulchenko commented 6 years ago

Interesting; thank you for the report! I wonder if there is a simple test for those numbers (short of wrapping it into a pcall).

Maybe there is no need to check for those numbers at all, as the effect is not going to be different between using s or d modifier. Can you try with the following patch?

diff --git a/src/serpent.lua b/src/serpent.lua
index a043713..ca96381 100644
--- a/src/serpent.lua
+++ b/src/serpent.lua
@@ -36,7 +36,7 @@ local function s(t, opts)
     return (path or '')..(plain and path and '.' or '')..safe, safe end
   local alphanumsort = type(opts.sortkeys) == 'function' and opts.sortkeys or function(k, o, n) -- k=keys, o=originaltable, n=padding
     local maxn, to = tonumber(n) or 12, {number = 'a', string = 'b'}
-    local function padnum(d) return ("%0"..tostring(maxn).."d"):format(tonumber(d)) end
+    local function padnum(d) return ("%0"..tostring(maxn).."s"):format(tonumber(d)) end
     table.sort(k, function(a,b)
       -- sort numeric keys first: k[key] is not nil for numerical keys
       return (k[a] ~= nil and 0 or to[type(a)] or 'z')..(tostring(a):gsub("%d+",padnum))

But if this works, then tonumber conversion doesn't seem to be needed either:

diff --git a/src/serpent.lua b/src/serpent.lua
index a043713..36d6239 100644
--- a/src/serpent.lua
+++ b/src/serpent.lua
@@ -36,7 +36,7 @@ local function s(t, opts)
     return (path or '')..(plain and path and '.' or '')..safe, safe end
   local alphanumsort = type(opts.sortkeys) == 'function' and opts.sortkeys or function(k, o, n) -- k=keys, o=originaltable, n=padding
     local maxn, to = tonumber(n) or 12, {number = 'a', string = 'b'}
-    local function padnum(d) return ("%0"..tostring(maxn).."d"):format(tonumber(d)) end
+    local function padnum(d) return ("%0"..tostring(maxn).."s"):format(d) end
     table.sort(k, function(a,b)
       -- sort numeric keys first: k[key] is not nil for numerical keys
       return (k[a] ~= nil and 0 or to[type(a)] or 'z')..(tostring(a):gsub("%d+",padnum))

I'll have to check the sorting results, but so far I don't see any issues with the tests I have.

pkulchenko commented 6 years ago

I'll have to test the updated sorting against the examples here: http://notebook.kulchenko.com/algorithms/alphanumeric-natural-sorting-for-humans-in-lua

pkulchenko commented 5 years ago

@chrisgbk, did you have a chance to test the code with the patch? Let me know if it's working for you.

chrisgbk commented 5 years ago

Apologies for the delay; both patches work as far as not crashing lua 5.2.4 or 5.3.4, but whether or not they provide the intended sorting behaviour is a better question.