patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9430, altcp API multiples fixes

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9430: altcp API multiples fixes

Submitter:  David GIRAULT <dgirault>
Submitted:  Mon 07 Aug 2017 02:18:41 PM UTC
   
 
Category:  apps Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  2.1.0

Jump to the original submission

Tue 16 Jan 2018 08:49:42 PM UTC, comment #8: 

Sorry, could you pass me to the function where I removed that?

In general, in (al-)TCP a layer should free its memory when it is closed. In the simple case of "app <-> tls <-> tcp", the inner_conn is tcp, which should free itself when both communication directions are closed.

Do you see memory leaks in this area?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 16 Jan 2018 08:28:40 AM UTC, comment #7: 

Currently rebasing my own master to yours, I understood that you revert the altcp_free(inner_conn), because of double free, but I don't see where the inner_conn is freed... Could you give me some tips?

Regards,
David

David GIRAULT <dgirault>
Wed 15 Nov 2017 09:00:04 PM UTC, comment #6: 

After finally finding the time to continue on this: my apologies, the mbedtls part was nearly correct.

I found a double-free though, which I fixed in commit bd2e82082955aa3fd64cddda98d69acf5e918032.

Thanks for the patch.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 20 Sep 2017 07:19:59 PM UTC, comment #5: 

OK, I've pushed the fix to altcp_tcp.c (slightly modified).

However, I'm not sure the mbedtls close/dealloc fixes are correct. It looks like the changes work for your specific handshake abort issue, but there might still be memory leaks in other places? Give me some time to check that further...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 28 Aug 2017 08:01:36 PM UTC, comment #4: 

Thanks. I'll try to review this after fixing bug #51687

Simon Goldschmidt <goldsimon>
Group administrator
Wed 09 Aug 2017 05:31:46 PM UTC, comment #3: 

I'm not really conviced too for the altcp_tcp_close() changes. But, this fix is the only way I got altcp_pcb leaks fixed.
This change also made the altcp layer the same way the tcp layer.
pcb should not be used anymore after closed.

I have additionnal patch to do the same for altcp_mbedtls_close(), in addition of closing inner_conn.

I've just rebased my branch to last master to date (ba7a3204) and attach new patchset with remaining fixes.

David


(file #41464, file #41465)

David GIRAULT <dgirault>
Tue 08 Aug 2017 10:56:26 AM UTC, comment #2: 

I've pushed your patches with 2 changes:
a) I'm not yet convinced by the tcp_close() change: if it returns != ERR_OK, the callbacks are gone, so the pcb might be just leaked
b) instead of exposing altcp_tcp_setup(), I've added altcp_tcp_wrap(), which allocates the altcp_pcb.

Leaving this open for the tcp_close() issue

Simon Goldschmidt <goldsimon>
Group administrator
Tue 08 Aug 2017 08:56:11 AM UTC, comment #1: 

Thanks for the patches! The work on altcp is not finished yet, so any improvement is welcome!

Especially I'm not really happy with the way servers and connections are instantiated. There will be some API change to that, hopefully before 2.1.0...

In the meantime, I'll review your patches :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 07 Aug 2017 02:18:41 PM UTC, original submission:  

In our project, we use the altcp layer to support both MQTT and HTTP/HTTPS.

We faced a lot of problems and asserts so I try to fixe them with the following patches.

Thoses patches will resolve the following problems:

- altcp_free() result in memory leak,
- altcp_pcb not freed on lower error callback calls,
- no way to free mbedtls config,
- no way to retreive mbedtls ssl context to check certificates,
- no way to attach existing tcp_pcb to a new altcp_pcb.
- removed an assert in altcp_mbedtls since this condition (rx_pbuf == 0 at end of handshake) can fail.

David GIRAULT <dgirault>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dgirault (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-15 goldsimon StatusIn Progress Done
        Open/ClosedOpen Closed
        Planned ReleaseNone 2.1.0
    2017-08-09 dgirault Attached File- Added 0001-altcp_tcp-free-altcp_pcb-struct-in-altcp_tcp_close.patch, #41464
        Attached File- Added 0002-altcp_mbedtls-close-and-error-related-fixes.patch, #41465
    2017-08-08 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2017-08-07 dgirault Attached File- Added 0005-altcp_tls_mbedtls-remove-rx-pbufs-left-at-end-of-han.patch, #41447
    2017-08-07 dgirault Attached File- Added 0001-altcp_tls_mbedtls-fix-log-messages-by-include-a-n.patch, #41443
        Attached File- Added 0002-altcp-fix-altcp_free.patch, #41444
        Attached File- Added 0003-altcp_tcp-some-fixes.patch, #41445
        Attached File- Added 0004-altcp_tls-some-fixes.patch, #41446

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code