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

Http1 removetx 5921 v12 #423

Closed
wants to merge 2 commits into from

Conversation

catenacyber
Copy link
Contributor

#418 with SV passing for http-sticky-server :
do not pop out of the list when removing a tx, as the list size is used for tx count

@victorjulien
Copy link
Member

@catenacyber
Copy link
Contributor Author

Looks better https://github.com/OISF/suricata/actions/runs/8812620289

So, will you merge this ?

@ct0br0
Copy link

ct0br0 commented Apr 29, 2024

Testing the auto-libhtp-PR pickup with this. Run should be done this afternoon ~4hr

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 20399

@victorjulien victorjulien self-requested a review June 4, 2024 15:57
Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still leads to a double free in Suricata

=================================================================
==24790==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000599c60 at pc 0x7f7550b76117 bp 0x7f751e932ca0 sp 0x7f751e932c90
READ of size 8 at 0x614000599c60 thread T98854 (W#44)
    #0 0x7f7550b76116 in htp_tx_get_user_data /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_transaction.c:228
    #1 0x55d5d5e9f861 in HTPStateFree (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2d1861)
    #2 0x55d5d5ec1083 in AppLayerParserStateProtoCleanup (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2f3083)
    #3 0x55d5d5ec114e in AppLayerParserStateCleanup (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2f314e)
    #4 0x55d5d60764a3 in FlowCleanupAppLayer (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4a84a3)
    #5 0x55d5d607d6f2 in FlowClearMemory (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4af6f2)
    #6 0x55d5d60a0d19 in CheckWorkQueue (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d2d19)
    #7 0x55d5d60a317b in FlowWorkerProcessLocalFlows (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d517b)
    #8 0x55d5d60a4bfc in FlowWorker (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d6bfc)
    #9 0x55d5d5e2eaf3 in TmThreadsSlotVarRun (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x260af3)
    #10 0x55d5d5e30d2f in TmThreadsSlotVar (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x262d2f)
    #11 0x7f7550724ac2  (/lib/x86_64-linux-gnu/libc.so.6+0x94ac2)
    #12 0x7f75507b5a03 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x125a03)

0x614000599c60 is located 32 bytes inside of 400-byte region [0x614000599c40,0x614000599dd0)
freed by thread T98854 (W#44) here:
    #0 0x7f7550c58537 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127
    #1 0x7f7550b7ad17 in htp_tx_destroy /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_transaction.c:122
    #2 0x7f7550b7ad17 in htp_tx_destroy /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_transaction.c:117
    #3 0x55d5d5e9fa8f in HTPStateTransactionFree (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2d1a8f)
    #4 0x55d5d5ebd9a8 in AppLayerParserTransactionsCleanup (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2ef9a8)
    #5 0x55d5d60a2a7d in FlowWorkerFlowTimeout (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d4a7d)
    #6 0x55d5d60a0599 in FlowFinish (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d2599)
    #7 0x55d5d60a09f8 in CheckWorkQueue (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d29f8)
    #8 0x55d5d60a317b in FlowWorkerProcessLocalFlows (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d517b)
    #9 0x55d5d60a4bfc in FlowWorker (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x4d6bfc)
    #10 0x55d5d5e2eaf3 in TmThreadsSlotVarRun (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x260af3)
    #11 0x55d5d5e30d2f in TmThreadsSlotVar (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x262d2f)
    #12 0x7f7550724ac2  (/lib/x86_64-linux-gnu/libc.so.6+0x94ac2)

previously allocated by thread T98854 (W#44) here:
    #0 0x7f7550c58a57 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x7f7550b75b8f in htp_tx_create /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_transaction.c:59
    #2 0x7f7550b59d9c in htp_connp_tx_create /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_connection_parser.c:210
    #3 0x7f7550b66479 in htp_connp_REQ_IDLE /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_request.c:966
    #4 0x7f7550b66479 in htp_connp_REQ_IDLE /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_request.c:958
    #5 0x7f7550b69be4 in htp_connp_req_data /builds/inliniac/suricata-ci/suricata/libhtp/htp/htp_request.c:1091
    #6 0x55d5d5ea11eb in HTPHandleRequestData (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2d31eb)
    #7 0x55d5d5ebfc8b in AppLayerParserParse (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2f1c8b)
    #8 0x55d5d5e81998 in TCPProtoDetect (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2b3998)
    #9 0x55d5d5e82b2d in AppLayerHandleTCPData (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x2b4b2d)
    #10 0x55d5d619d7dc in ReassembleUpdateAppLayer (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x5cf7dc)
    #11 0x55d5d619ddb4 in StreamTcpReassembleAppLayer (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x5cfdb4)
    #12 0x55d5d61a13c3 in StreamTcpReassembleHandleSegmentUpdateACK (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x5d33c3)
    #13 0x55d5d61a177e in StreamTcpReassembleHandleSegment (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x5d377e)
    #14 0x55d5d616994b in HandleEstablishedPacketToClient (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x59b94b)
    #15 0x55d5d616c789 in StreamTcpPacketStateEstablished (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x59e789)
    #16 0x55d5d617e8a1 in StreamTcpStateDispatch (/builds/inliniac/suricata-ci/suricata/src/.libs/suricata+0x5b08a1)

@catenacyber
Copy link
Contributor Author

Thinking giving up on this optimization, and working on libhtp-rs 🤔

@catenacyber
Copy link
Contributor Author

Giving up on this optimization, so summing up :

  • this optimization assumed that the transactions got freed in sequential order
  • this assumption is wrong (2 pipelined requests, first got responses, second gets not, and so the file logger delays the cleanup pf the first tx after the second one which has no file to log)

Because of the list structure, which can be realloced, and its elements moved, we cannot keep the index where we put it somewhere to reuse it when replacing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants