pkg/lwip/netdev: fix return types in send() by mguetschow · Pull Request #21833 · RIOT-OS/RIOT

mguetschow · 2025-10-30T13:33:12Z

Contribution description

The netdev API returns int, with negative meaning error conditions. err_t, which is currently used as the type for the return values, is unsigned. This would lead to spurious issues with error conditions.

Testing procedure

sudo ip tuntap add tap0 mode tap user ${USER}; sudo ip link set tap0 up;
LWIP_IPV6=1 make -C examples/networking/coap/gcoap_dtls flash term
(in another terminal, with the correct IPv6 address) coap-client-gnutls "coaps://[fe80::xxxx:xxxx:xxxx:xxxx%tap0]/.well-known/core" -k "secretPSK" -u "Client_identity" (libcoap cli client)

On master this fails with ERR cannot send CoAP pdu and Wireshark shows a DTLS abort package sent by RIOT right after the successfully sent ServerHello. Enabling debug in sock_dtls.c, lwip_sock.c and lwip_netdev.c shows a problem.

With the changes from this PR, everything works as expected.

crasbe · 2025-10-30T13:44:21Z

According to the documentation, err_t is the right type to use. Why is it defined as unsigned... and where?
https://rtl-lwip.sourceforge.net/sources/err_8h.html#a13

I added some error messages to build/pkg/lwip/src/include/lwip/err.h:

/** Define LWIP_ERR_T in cc.h if you want to use
 *  a different type for your platform (must be signed). */
#ifdef LWIP_ERR_T
typedef LWIP_ERR_T err_t;
+#error Custom Type
#else /* LWIP_ERR_T */
typedef s8_t err_t;
+#error Default s8_t
#endif /* LWIP_ERR_T*/

And this is the result:

buechse@skyleaf:~/RIOTstuff/riot-nrfvendor/RIOT$ LWIP_IPV6=1 make -C examples/networking/coap/gcoap_dtls
make: Entering directory '/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls'
using BOARD="native64" as "native" on a 64-bit system
Building application "gcoap_example" for "native64" with CPU "native".

"make" -C /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/pkg/lwip/
"make" -C /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/api -f /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/Makefile.base MODULE=lwip_api
In file included from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/pbuf.h:42,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/netbuf.h:46,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/api.h:47,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/api/api_lib.c:62:
/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/err.h:98:2: error: #error Default s8_t
   98 | #error Default s8_t
      |  ^~~~~
make[2]: *** [/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/Makefile.base:157: /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls/bin/native64/lwip_api/api_lib.o] Error 1
make[1]: *** [Makefile:31: lwip_api] Error 2
make: *** [/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls/../../../../Makefile.include:807: pkg-build] Error 2
make: Leaving directory '/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls'

Therefore at some point, s8_t is defined as an unsigned?

crasbe · 2025-10-30T13:50:01Z

This is very odd:

This is where s8_t is defined:

buechse@skyleaf:~/RIOTstuff/riot-nrfvendor/RIOT$ grep -Rnw . -e "typedef .* s8_t;"
./build/pkg/lwip/src/include/lwip/arch.h:128:typedef int8_t    s8_t;

And the header is actually used:

buechse@skyleaf:~/RIOTstuff/riot-nrfvendor/RIOT$ LWIP_IPV6=1 make -C examples/networking/coap/gcoap_dtls
make: Entering directory '/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls'
using BOARD="native64" as "native" on a 64-bit system
Building application "gcoap_example" for "native64" with CPU "native".

"make" -C /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/pkg/lwip/
"make" -C /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/api -f /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/Makefile.base MODULE=lwip_api
In file included from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/debug.h:40,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/opt.h:52,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/api/api_lib.c:58:
/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/arch.h:40:2: error: #error I am included somewhere!
   40 | #error I am included somewhere!
      |  ^~~~~
In file included from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/pbuf.h:42,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/netbuf.h:46,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/api.h:47,
                 from /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/api/api_lib.c:62:
/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/build/pkg/lwip/src/include/lwip/err.h:98:2: error: #error Default s8_t
   98 | #error Default s8_t
      |  ^~~~~
make[2]: *** [/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/Makefile.base:157: /home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls/bin/native64/lwip_api/api_lib.o] Error 1
make[1]: *** [Makefile:31: lwip_api] Error 2
make: *** [/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls/../../../../Makefile.include:807: pkg-build] Error 2
make: Leaving directory '/home/buechse/RIOTstuff/riot-nrfvendor/RIOT/examples/networking/coap/gcoap_dtls'

mguetschow · 2025-10-30T13:53:50Z

According to the documentation, err_t is the right type to use. Why is it defined as unsigned... and where?
rtl-lwip.sourceforge.net/sources/err_8h.html#a13

Sure it's the right return type for the lwip callbacks, but not the right return type for the netdev callbacks.

crasbe · 2025-10-30T13:57:52Z

According to the documentation, err_t is the right type to use. Why is it defined as unsigned... and where?
rtl-lwip.sourceforge.net/sources/err_8h.html#a13

Sure it's the right return type for the lwip callbacks, but not the right return type for the netdev callbacks.

Not sure I follow, the return type of the function is literally err_t. The proposed change shouldn't make any difference:

RIOT/pkg/lwip/contrib/netdev/lwip_netdev.c

Lines 273 to 283 in 1438aac

    
           #if (IS_USED(MODULE_NETDEV_NEW_API)) 
        
           static err_t _common_link_output(struct netif *netif, netdev_t *netdev, iolist_t *iolist) 
        
           { 
        
               lwip_netif_dev_acquire(netif); 
        
               err_t res; 
        
               if (is_netdev_legacy_api(netdev)) { 
        
                   res = (netdev->driver->send(netdev, iolist) > 0) ? ERR_OK : ERR_BUF; 
        
                   lwip_netif_dev_release(netif); 
        
                   return res; 
        
               }

Same for the old API:

RIOT/pkg/lwip/contrib/netdev/lwip_netdev.c

Lines 328 to 335 in 1438aac

    
           #else /* only old API */ 
        
           static err_t _common_link_output(struct netif *netif, netdev_t *netdev, iolist_t *iolist) 
        
           { 
        
               lwip_netif_dev_acquire(netif); 
        
               err_t res = (netdev->driver->send(netdev, iolist) > 0) ? ERR_OK : ERR_BUF; 
        
               lwip_netif_dev_release(netif); 
        
               return res; 
        
           }

crasbe · 2025-10-30T13:59:32Z

Oh... The only reason an issue might occur is if the return value of send is bigger than 127, leading to a wrap-around. Might that be the case?

pkg/lwip/contrib/netdev/lwip_netdev.c

mguetschow · 2025-10-30T14:10:40Z

According to the documentation, err_t is the right type to use. Why is it defined as unsigned... and where?
rtl-lwip.sourceforge.net/sources/err_8h.html#a13

Sure it's the right return type for the lwip callbacks, but not the right return type for the netdev callbacks.

Not sure I follow, the return type of the function is literally err_t.

Yes, _common_link_output has err_t as return type, but netdev->driver->send has int as return type. When storing the return of the send function in res (which is currently err_t), it will loose negative values and wrap around for larger values.

crasbe · 2025-10-30T14:14:22Z

According to the documentation, err_t is the right type to use. Why is it defined as unsigned... and where?
rtl-lwip.sourceforge.net/sources/err_8h.html#a13

Sure it's the right return type for the lwip callbacks, but not the right return type for the netdev callbacks.

Not sure I follow, the return type of the function is literally err_t.

Yes, _common_link_output has err_t as return type, but netdev->driver->send has int as return type. When storing the return of the send function in res (which is currently err_t), it will loose negative values and wrap around for larger values.

Yes, that's the same conclusion I had after writing the first reply:

Oh... The only reason an issue might occur is if the return value of send is bigger than 127, leading to a wrap-around. Might that be the case?

Nevertheless the err_t is not unsigned as you wrote initially 😅

It's still odd that the return value of the function is implicitly cast to int8_t before checking if it's bigger than 0, but it is what it is I guess.

The function for the old API has to be changed too.

mguetschow · 2025-10-30T14:27:57Z

Nevertheless the err_t is not unsigned as you wrote initially 😅

I'll give you that :)

The function for the old API has to be changed too.

Good point, done now.

crasbe

If @maribu is also happy, you can squash :)

maribu · 2025-10-30T14:33:40Z

Never wait for me with regard to squashing 😉

riot-ci · 2025-10-30T15:50:20Z

Murdock results

✔️ PASSED

a045acb pkg/lwip/netdev: fix return types in send()

Success	Failures	Total	Runtime
10560	0	10560	11m:39s

Artifacts

Documentation preview

mguetschow · 2025-11-03T08:28:15Z

Thanks everyone for your reviews! 🤗

mguetschow requested a review from miri64 as a code owner October 30, 2025 13:33

github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports labels Oct 30, 2025

mguetschow mentioned this pull request Oct 30, 2025

pkg/lwip: document debug defines #21834

Merged

mguetschow requested review from crasbe and maribu October 30, 2025 13:34

mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 30, 2025

crasbe added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 30, 2025

maribu reviewed Oct 30, 2025

View reviewed changes

pkg/lwip/contrib/netdev/lwip_netdev.c Outdated Show resolved Hide resolved

mguetschow mentioned this pull request Oct 30, 2025

net/unicoap: Unified and Modular CoAP stack: Messaging and Minimal Server (pt 2) #21582

Open

crasbe approved these changes Oct 30, 2025

View reviewed changes

mguetschow added 2 commits October 30, 2025 15:38

pkg/lwip: improve debug output with tiny_strerror

432b1b5

pkg/lwip/netdev: fix return types in send()

a045acb

mguetschow force-pushed the lwip-netdev-type-fix branch from e53a6d6 to a045acb Compare October 30, 2025 14:38

crasbe added this pull request to the merge queue Oct 30, 2025

Merged via the queue into RIOT-OS:master with commit 4b55480 Oct 30, 2025
25 checks passed

mguetschow deleted the lwip-netdev-type-fix branch November 3, 2025 08:28

benpicco added this to the Release 2025.10 milestone Dec 3, 2025

Conversation

mguetschow commented Oct 30, 2025

Contribution description

Testing procedure

Uh oh!

crasbe commented Oct 30, 2025

Uh oh!

crasbe commented Oct 30, 2025

Uh oh!

mguetschow commented Oct 30, 2025

Uh oh!

crasbe commented Oct 30, 2025

Uh oh!

crasbe commented Oct 30, 2025

Uh oh!

Uh oh!

mguetschow commented Oct 30, 2025

Uh oh!

crasbe commented Oct 30, 2025

Uh oh!

mguetschow commented Oct 30, 2025

Uh oh!

crasbe left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Oct 30, 2025

Uh oh!

riot-ci commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

Uh oh!

mguetschow commented Nov 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

riot-ci commented Oct 30, 2025 •

edited

Loading