Skip to content

Commit 5e9bcac

Browse files
arnetheduckarnetheduck
authored
Fix leaks / update error handling (#178)
* modernise imports * introduce `raises` for most `async` functions * remove lots of spurious memory allocations and zeroing * check decompression max size before allocating memory (!) * simplify error handling when reading frames * avoid blocking connection for too long just to send errors * don't swallow original error when trying to send error to client * need more mem for deflate * fail autobahn job on autobahn failures * deprecate handshakeTimeout
1 parent 35ae76f commit 5e9bcac

28 files changed

+786
-842
lines changed

‎examples/autobahn_client.nim‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@
99

1010
import
1111
std/[strutils],
12-
pkg/[chronos, chronicles, stew/byteutils],
13-
../websock/[websock, types, frame, extensions/compression/deflate]
12+
chronos,
13+
chronicles,
14+
stew/byteutils,
15+
../websock/[websock, types, extensions/compression/deflate]
1416

1517
const
1618
clientFlags = {NoVerifyHost, NoVerifyServerName}

‎examples/client.nim‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77
## This file may not be copied, modified, or distributed except according to
88
## those terms.
99

10-
import pkg/[
10+
import
1111
chronos,
1212
chronicles,
13-
stew/byteutils]
13+
stew/byteutils
1414

1515
import ../websock/websock
1616

‎examples/server.nim‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@
77
## This file may not be copied, modified, or distributed except according to
88
## those terms.
99

10-
import std/uri
11-
import pkg/[chronos,
12-
chronicles,
13-
httputils]
10+
import
11+
std/uri,
12+
chronos,
13+
chronicles,
14+
../websock/[websock, extensions/compression/deflate]
1415

15-
import ../websock/[websock, extensions/compression/deflate]
16-
import ../tests/keys
16+
when defined tls:
17+
import ../tests/keys
1718

1819
proc handle(request: HttpRequest) {.async.} =
1920
trace "Handling request:", uri = request.uri.path

‎scripts/ws.sh‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
# prevent issue https://github.com/status-im/nimbus-eth1/issues/3661
1010

1111
set -e
12+
trap "trap - SIGTERM && pkill -P $$" SIGINT SIGTERM EXIT
1213

1314
# script arguments
1415
[[ $# -ne 1 ]] && { echo "Usage: $0 NIM_VERSION"; }
@@ -17,22 +18,23 @@ NIM_VERSION="$1"
1718
cd "$(dirname "${BASH_SOURCE[0]}")"/..
1819

1920
REPO_DIR="${PWD}"
21+
CFG="server"
22+
REPORT_DIR="autobahn/reports/$CFG-$NIM_VERSION"
23+
mkdir -p autobahn/reports/$CFG
2024

2125
nim c -d:release examples/server
2226
examples/server &
23-
server=$!
2427

25-
mkdir -p autobahn/reports
26-
27-
docker run \
28+
docker run --rm \
2829
-v ${REPO_DIR}/autobahn:/config \
2930
-v ${REPO_DIR}/autobahn/reports:/reports \
3031
--network=host \
3132
--name fuzzingclient \
3233
crossbario/autobahn-testsuite wstest --mode fuzzingclient --spec /config/fuzzingclient.json
3334

34-
kill $server
35+
mv autobahn/reports/$CFG "$REPORT_DIR"
3536

36-
mv autobahn/reports/server autobahn/reports/server-${NIM_VERSION}
37+
echo "* [Nim-${NIM_VERSION} $CFG summary report]($CFG-${NIM_VERSION}/index.html)" > "$REPORT_DIR.txt"
3738

38-
echo "* [Nim-${NIM_VERSION} ws server summary report](server-${NIM_VERSION}/index.html)" > "autobahn/reports/server-${NIM_VERSION}.txt"
39+
# squash to single line and look for errors
40+
(cat $REPORT_DIR/index.json | tr '\n' '!' | sed "s|\},\!|\n|g" | tr '!' ' ' | tr -s ' ' | grep -v -e '"behavior": "OK"' -e '"behavior": "NON-STRICT"' -e '"behavior": "INFORMATIONAL"' && exit 1) || true

‎scripts/wsc.sh‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
# prevent issue https://github.com/status-im/nimbus-eth1/issues/3661
1010

1111
set -e
12+
trap "trap - SIGTERM && pkill -P $$" SIGINT SIGTERM EXIT
1213

1314
# script arguments
1415
[[ $# -ne 1 ]] && { echo "Usage: $0 NIM_VERSION"; }
@@ -17,21 +18,25 @@ NIM_VERSION="$1"
1718
cd "$(dirname "${BASH_SOURCE[0]}")"/..
1819

1920
REPO_DIR="${PWD}"
21+
CFG="client"
22+
REPORT_DIR="autobahn/reports/$CFG-$NIM_VERSION"
23+
mkdir -p autobahn/reports/$CFG
2024

21-
mkdir -p autobahn/reports
22-
23-
docker run -d \
25+
docker run -d --rm \
2426
-v ${REPO_DIR}/autobahn:/config \
2527
-v ${REPO_DIR}/autobahn/reports:/reports \
2628
--network=host \
2729
--name fuzzingserver \
2830
crossbario/autobahn-testsuite wstest --webport=0 --mode fuzzingserver --spec /config/fuzzingserver.json
2931

32+
trap "docker kill fuzzingserver" SIGINT SIGTERM EXIT
33+
3034
nim c -d:release examples/autobahn_client
3135
examples/autobahn_client
3236

33-
docker kill fuzzingserver
37+
mv autobahn/reports/$CFG $REPORT_DIR
3438

35-
mv autobahn/reports/client autobahn/reports/client-${NIM_VERSION}
39+
echo "* [Nim-${NIM_VERSION} $CFG summary report]($CFG-${NIM_VERSION}/index.html)" > "$REPORT_DIR.txt"
3640

37-
echo "* [Nim-${NIM_VERSION} ws client summary report](client-${NIM_VERSION}/index.html)" > "autobahn/reports/client-${NIM_VERSION}.txt"
41+
# squash to single line and look for errors
42+
(cat $REPORT_DIR/index.json | tr '\n' '!' | sed "s|\},\!|\n|g" | tr '!' ' ' | tr -s ' ' | grep -v -e '"behavior": "OK"' -e '"behavior": "NON-STRICT"' -e '"behavior": "INFORMATIONAL"' && exit 1) || true

‎scripts/wss.sh‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88

99
# prevent issue https://github.com/status-im/nimbus-eth1/issues/3661
1010

11-
1211
set -e
12+
trap "trap - SIGTERM && pkill -P $$" SIGINT SIGTERM EXIT
1313

1414
# script arguments
1515
[[ $# -ne 1 ]] && { echo "Usage: $0 NIM_VERSION"; }
@@ -18,22 +18,23 @@ NIM_VERSION="$1"
1818
cd "$(dirname "${BASH_SOURCE[0]}")"/..
1919

2020
REPO_DIR="${PWD}"
21+
CFG="server_tls"
22+
REPORT_DIR="autobahn/reports/$CFG-$NIM_VERSION"
23+
mkdir -p autobahn/reports/$CFG
2124

2225
nim c -d:tls -d:release -o:examples/tls_server examples/server.nim
2326
examples/tls_server &
24-
server=$!
25-
26-
mkdir -p autobahn/reports
2727

28-
docker run \
28+
docker run --rm \
2929
-v ${REPO_DIR}/autobahn:/config \
3030
-v ${REPO_DIR}/autobahn/reports:/reports \
3131
--network=host \
3232
--name fuzzingclient_tls \
3333
crossbario/autobahn-testsuite wstest --mode fuzzingclient --spec /config/fuzzingclient_tls.json
3434

35-
kill $server
35+
mv autobahn/reports/$CFG "$REPORT_DIR"
3636

37-
mv autobahn/reports/server_tls autobahn/reports/server_tls-${NIM_VERSION}
37+
echo "* [Nim-${NIM_VERSION} $CFG summary report]($CFG-${NIM_VERSION}/index.html)" > "$REPORT_DIR.txt"
3838

39-
echo "* [Nim-${NIM_VERSION} wss server summary report](server_tls-${NIM_VERSION}/index.html)" > "autobahn/reports/server_tls-${NIM_VERSION}.txt"
39+
# squash to single line and look for errors
40+
(cat $REPORT_DIR/index.json | tr '\n' '!' | sed "s|\},\!|\n|g" | tr '!' ' ' | tr -s ' ' | grep -v -e '"behavior": "OK"' -e '"behavior": "NON-STRICT"' -e '"behavior": "INFORMATIONAL"' && exit 1) || true

‎scripts/wssc.sh‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
# prevent issue https://github.com/status-im/nimbus-eth1/issues/3661
1010

1111
set -e
12+
trap "trap - SIGTERM && pkill -P $$" SIGINT SIGTERM EXIT
1213

1314
# script arguments
1415
[[ $# -ne 1 ]] && { echo "Usage: $0 NIM_VERSION"; }
@@ -17,21 +18,25 @@ NIM_VERSION="$1"
1718
cd "$(dirname "${BASH_SOURCE[0]}")"/..
1819

1920
REPO_DIR="${PWD}"
21+
CFG="client_tls"
22+
REPORT_DIR="autobahn/reports/$CFG-$NIM_VERSION"
23+
mkdir -p autobahn/reports/$CFG
2024

21-
mkdir -p autobahn/reports
22-
23-
docker run -d \
25+
docker run -d --rm \
2426
-v ${REPO_DIR}/autobahn:/config \
2527
-v ${REPO_DIR}/autobahn/reports:/reports \
2628
--network=host \
2729
--name fuzzingserver_tls \
2830
crossbario/autobahn-testsuite wstest --webport=0 --mode fuzzingserver --spec /config/fuzzingserver_tls.json
2931

32+
trap "docker kill fuzzingserver_tls" SIGINT SIGTERM EXIT
33+
3034
nim c -d:tls -d:release -o:examples/autobahn_tlsclient examples/autobahn_client
3135
examples/autobahn_tlsclient
3236

33-
docker kill fuzzingserver_tls
37+
mv autobahn/reports/$CFG $REPORT_DIR
3438

35-
mv autobahn/reports/client_tls autobahn/reports/client_tls-${NIM_VERSION}
39+
echo "* [Nim-${NIM_VERSION} $CFG summary report]($CFG-${NIM_VERSION}/index.html)" > "$REPORT_DIR.txt"
3640

37-
echo "* [Nim-${NIM_VERSION} wss client summary report](client_tls-${NIM_VERSION}/index.html)" > "autobahn/reports/client_tls-${NIM_VERSION}.txt"
41+
# squash to single line and look for errors
42+
(cat $REPORT_DIR/index.json | tr '\n' '!' | sed "s|\},\!|\n|g" | tr '!' ' ' | tr -s ' ' | grep -v -e '"behavior": "OK"' -e '"behavior": "NON-STRICT"' -e '"behavior": "INFORMATIONAL"' && exit 1) || true

‎tests/extensions/base64ext.nim‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,11 @@
88
## those terms.
99

1010
import
11-
pkg/[stew/base64,
12-
chronos,
13-
chronicles,
14-
results],
15-
../../websock/types,
16-
../../websock/frame
11+
stew/base64,
12+
chronos,
13+
chronicles,
14+
results,
15+
../../websock/[frame, types]
1716

1817
type
1918
Base64Ext = ref object of Ext
@@ -23,7 +22,9 @@ type
2322
const
2423
extID = "base64"
2524

26-
method decode(ext: Base64Ext, frame: Frame): Future[Frame] {.async.} =
25+
method decode(
26+
ext: Base64Ext, frame: Frame
27+
): Future[Frame] {.async: (raises: [CancelledError, AsyncStreamError, WebSocketError]).} =
2728
if frame.opcode notin {Opcode.Text, Opcode.Binary, Opcode.Cont}:
2829
return frame
2930

@@ -48,10 +49,13 @@ method decode(ext: Base64Ext, frame: Frame): Future[Frame] {.async.} =
4849

4950
# bug in Base64.Decode when accepts seq[byte]
5051
let instr = cast[string](data)
51-
if ext.padding:
52-
frame.data = Base64Pad.decode(instr)
53-
else:
54-
frame.data = Base64.decode(instr)
52+
try:
53+
if ext.padding:
54+
frame.data = Base64Pad.decode(instr)
55+
else:
56+
frame.data = Base64.decode(instr)
57+
except Base64Error:
58+
raise newException(WSExtError, "invalid data")
5559

5660
trace "Base64Ext decode", input=frame.length, output=frame.data.len
5761

@@ -62,7 +66,9 @@ method decode(ext: Base64Ext, frame: Frame): Future[Frame] {.async.} =
6266

6367
return frame
6468

65-
method encode(ext: Base64Ext, frame: Frame): Future[Frame] {.async.} =
69+
method encode(
70+
ext: Base64Ext, frame: Frame
71+
): Future[Frame] {.async: (raises: [CancelledError, WebSocketError]).} =
6672
if frame.opcode notin {Opcode.Text, Opcode.Binary, Opcode.Cont}:
6773
return frame
6874

‎tests/extensions/hexext.nim‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,7 @@
77
## This file may not be copied, modified, or distributed except according to
88
## those terms.
99

10-
import
11-
pkg/[results,
12-
stew/byteutils,
13-
chronos,
14-
chronicles],
15-
../../websock/types,
16-
../../websock/frame
10+
import results, stew/byteutils, chronos, chronicles, ../../websock/[frame, types]
1711

1812
type
1913
HexExt = ref object of Ext
@@ -22,7 +16,9 @@ type
2216
const
2317
extID = "hex"
2418

25-
method decode(ext: HexExt, frame: Frame): Future[Frame] {.async.} =
19+
method decode(
20+
ext: HexExt, frame: Frame
21+
): Future[Frame] {.async: (raises: [CancelledError, AsyncStreamError, WebSocketError]).} =
2622
if frame.opcode notin {Opcode.Text, Opcode.Binary, Opcode.Cont}:
2723
return frame
2824

@@ -45,7 +41,11 @@ method decode(ext: HexExt, frame: Frame): Future[Frame] {.async.} =
4541
if data.len > ext.session.frameSize:
4642
raise newException(WSPayloadTooLarge, "payload exceeds allowed max frame size")
4743

48-
frame.data = hexToSeqByte(cast[string](data))
44+
frame.data = try:
45+
hexToSeqByte(cast[string](data))
46+
except ValueError:
47+
raise newException(WSExtError, "invalid data")
48+
4949
trace "HexExt decode", input=frame.length, output=frame.data.len
5050

5151
frame.length = frame.data.len.uint64
@@ -55,7 +55,9 @@ method decode(ext: HexExt, frame: Frame): Future[Frame] {.async.} =
5555

5656
return frame
5757

58-
method encode(ext: HexExt, frame: Frame): Future[Frame] {.async.} =
58+
method encode(
59+
ext: HexExt, frame: Frame
60+
): Future[Frame] {.async: (raises: [CancelledError, WebSocketError]).} =
5961
if frame.opcode notin {Opcode.Text, Opcode.Binary, Opcode.Cont}:
6062
return frame
6163

‎tests/extensions/testcompression.nim‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@
77
## This file may not be copied, modified, or distributed except according to
88
## those terms.
99

10-
import std/[os, strutils]
11-
import pkg/[chronos/unittest2/asynctests, stew/io2]
12-
import ../../websock/websock
13-
import ../../websock/extensions/compression/deflate
10+
import
11+
std/[os, strutils],
12+
chronos/unittest2/asynctests,
13+
stew/io2,
14+
../../websock/websock,
15+
../../websock/extensions/compression/deflate
1416

1517
const
1618
dataFolder = currentSourcePath.rsplit(os.DirSep, 1)[0] / "data"

0 commit comments

Comments
 (0)