Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
edrose-tewke opened this issue Dec 14, 2023 · 3 comments · Fixed by #376
Closed

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

edrose-tewke opened this issue Dec 14, 2023 · 3 comments · Fixed by #376
Labels

Comments

@edrose-tewke
Copy link

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)
@JKRhb JKRhb added the bug label Dec 14, 2023
@edrose-tewke
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Contributor

edrose commented Dec 17, 2023

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.

@JKRhb JKRhb linked a pull request Dec 17, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants