coapjs / node-coap

CoAP - Node.js style
MIT License
528 stars 154 forks source link

Max packet size error when payload length close to `maxPacketSize` #375

Closed edrose-tewke closed 5 months ago

edrose-tewke commented 8 months ago

Description

When node-coap is used as a server it is normally not required to explicitly set the Block2 option when sending a response, even if the payload length is greater than the maxPacketSize setting. This is because of a check performed at server.ts:678 which should detect that the payload requires a blockwise transfer, and then automatically apply the blockwise transfer logic to ensure that the transfer goes ahead.

Unfortunately this check is performed on just the payload, without regard for the size of the header. This means that the logic does not work for payload sizes that are close to, but not over, the maxPacketSize parameter. Depending on the header size, this holds for packets that are roughly between 1268 - 1280 bytes long.

Code to Recreate

The problem can easily be recreated by modifying examples/blockwise.js. If the client does not add the Block2 header (since it does not know what size the response will be), and the server generates a payload of 1268 bytes, the server will crash with the message "Max packet size is 1280: current is 1281".

If the requested payload size is increased on line 16 to a value >= 1280 then the server automatically applies blockwise transfers and no error is thrown. If it is decreased to <= 1267 then the payload doesn't need to be split and gets sent in a single response. The error only appears when (in pseudocode) payload.length < maxPacketSize && payload.length + header.length > maxPacketSize.

const coap = require('../') // or coap

coap.createServer((req, res) => {
    // FIXME: This has became a bit ugly due to the
    //        replacement of the depracated url.parse
    //        with the URL constructor.
    //        This part of the exmample should be replaced
    //        once a nicer solution has been found.

    const splitURL = req.url.split('?')
    const time = parseInt(splitURL[1].split('=')[1])
    const pathname = splitURL[0].split('/')[1]

    res.end(new Array(time + 1).join(pathname)) // <-- Make it easier to specify exact size
}).listen(() => {
    const req = coap.request('coap://localhost/1?t=1268') // <-- Request a payload of 1268 bytes

    // edit this to adjust max packet
    //req.setOption('Block2', Buffer.of(0x2)) // <-- Remove client Block2 option

    req.on('response', (res) => {
        res.pipe(process.stdout)
        res.on('end', () => {
            process.exit(0)
        })
    })

    req.end()
})

Here is a patch that can be applied to the master branch to change the example if needed

iff --git a/examples/blockwise.js b/examples/blockwise.js
index ea48b55..1824099 100644
--- a/examples/blockwise.js
+++ b/examples/blockwise.js
@@ -11,12 +11,12 @@ coap.createServer((req, res) => {
     const time = parseInt(splitURL[1].split('=')[1])
     const pathname = splitURL[0].split('/')[1]

-    res.end(new Array(time + 1).join(pathname + ' '))
+    res.end(new Array(time + 1).join(pathname)) // <-- Make it easier to specify exact size
 }).listen(() => {
-    const req = coap.request('coap://localhost/repeat-me?t=400')
+    const req = coap.request('coap://localhost/1?t=1268') // <-- Request a payload of 1268 bytes

     // edit this to adjust max packet
-    req.setOption('Block2', Buffer.of(0x2))
+    //req.setOption('Block2', Buffer.of(0x2)) // <-- Remove client Block2 option

     req.on('response', (res) => {
         res.pipe(process.stdout)
edrose-tewke commented 8 months ago

It's probably worth following the logic in Section 4.6 of the RFC, which makes the fix quite easy:

If the Path MTU is not known for a destination, an IP MTU of 1280 bytes SHOULD be assumed; if nothing is known about the size of the headers, good upper bounds are 1152 bytes for the message size and 1024 bytes for the payload size.

So Block2 transfers should start once the payload size goes above 1024.

In addition, the default for the maxPacketSize parameter should be reduced to 1152 since 1280 is the IP MTU and doesn't include the UDP header. If the UDP payload goes above 1280 then fragmentation at the IP layer might occur.

Apollon77 commented 8 months ago

Thank you very much for the infos and also research. If you like we are happy about any PR or such (like your patch in the first post).

edrose commented 8 months ago

I've been working on this in my personal time outside of work, so I've switched to my personal account (same person as OP, slightly different username).

I'll probably have a PR ready by the end of the day.