Feature/http range/v23 #6312

catenacyber · 2021-08-27T15:30:17Z

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/1576

Describe changes:

Uses a generic container (a thread-safe hash table whose key is the url/filename) to store file parts with HTTP Range header
handles reassembly of these parts, while enforcing a global memcap

suricata-verify-pr: 517
OISF/suricata-verify#517

Continues #6308 :

Adds more comments
Added a SC_ATOMIC_SUB for memcap when HTTP_RANGES_RB_INSERT finds a duplicate. Is it right ? (it looked strange to have free and not diminishing the memcap)
Refactors the code to be more clear that we can avoid the lock in HTPFileStoreChunk
- Adds a fileOwning field to show
- refactoring HttpRangeAppendData to append not as a default case, but only if fileOwning is set, and asserting that the default case is the last case remaining
- Handling gaps not while appending, but on closing
- not using directly files->tail->size everywhere but a lastsize field who gets synced when we have the ownership of files

For testing : https://github.com/jasonish/suricata-http-range-test

Compares two buffers with their two sizes

adds a container, ie a thread safe hash table whose key is the filename keep a tree of unordered ranges, up to a memcap limit adds HTPFileOpenWithRange to handle like HTPFileOpen if there is a range : open 2 files, one for the whole reassembled, and one only for the current range

Simplify locking by using the THashData lock instead of a separate range lock. Avoid size_t in function arguments. Clean up file handling functions. Implement handling of alloc errors. Rename yaml entry to byterange Unify public api naming

Better structure design to ensure that one flow maximum is owning and appending into the file, adding fileOwning field. Adds also a gap field in a range buffer, so that we can feed the gap on closing, when we are protected from concurrency by a lock, (lock which got removed in the append path) Fixes memcap when encountering a duplicate while inserting in red and black tree Adds many comments

catenacyber · 2021-08-27T15:32:28Z

src/app-layer-htp-range.c

@@ -438,6 +458,7 @@ File *HttpRangeClose(HttpRangeContainerBlock *c, uint16_t flags)
                    HTTP_RANGES_RB_INSERT(&c->container->fragment_tree, c->current);
            if (res) {
                SCLogDebug("duplicate range fragment");
+                (void)SC_ATOMIC_SUB(ContainerUrlRangeList.ht->memuse, c->current->buflen);


@victorjulien is this correct ?

Everywhere else we SCFree(*->buffer), we diminish the memcap

I don't understand the question. We update the memcap on this free, right?

We update the memcap on this free, right?

I added this in this latest PR.
It was not there before

Gotcha. This is why I don't like updates to existing PRs btw.

This is why I don't like updates to existing PRs btw.

In this case, the change was added between #6308 and #6312, so it was right ?

catenacyber · 2021-08-27T15:41:14Z

I added last commit because CI failed quickly while rebasing S-V PR...

catenacyber · 2021-08-27T15:50:04Z

Left TODO : implement for HTTP2

codecov · 2021-08-27T18:05:46Z

Codecov Report

Merging #6312 (08eca7e) into master (7551247) will decrease coverage by 0.02%.
The diff coverage is 68.24%.

@@            Coverage Diff             @@
##           master    #6312      +/-   ##
==========================================
- Coverage   76.95%   76.93%   -0.03%     
==========================================
  Files         611      612       +1     
  Lines      185955   186368     +413     
==========================================
+ Hits       143102   143378     +276     
- Misses      42853    42990     +137

Flag	Coverage Δ
fuzzcorpus	`52.83% <45.19%> (-0.03%)`	⬇️
suricata-verify	`51.21% <71.15%> (+0.09%)`	⬆️
unittests	`63.00% <3.55%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2021-08-28T13:53:10Z

Information: QA ran without warnings.

Pipeline 3934

victorjulien · 2021-09-02T08:49:28Z

src/app-layer-htp-range.h

+    uint16_t flags;
+    /** wether a HttpRangeContainerBlock is currently
+     owning the FileContainer in order to append to the file */
+    bool fileOwned;


style file_owned

removed in next PR

victorjulien · 2021-09-02T08:50:14Z

src/app-layer-htp-range.h

+    /** pointer to the main file container, where to directly append data */
+    HttpRangeContainerFile *container;
+    /** we own the container's file for now */
+    bool fileOwning;


style file_owning

removed in next PR

victorjulien · 2021-09-02T08:54:52Z

src/app-layer-htp-range.c

@@ -438,6 +458,7 @@ File *HttpRangeClose(HttpRangeContainerBlock *c, uint16_t flags)
                    HTTP_RANGES_RB_INSERT(&c->container->fragment_tree, c->current);
            if (res) {
                SCLogDebug("duplicate range fragment");
+                (void)SC_ATOMIC_SUB(ContainerUrlRangeList.ht->memuse, c->current->buflen);


I don't understand the question. We update the memcap on this free, right?

victorjulien · 2021-09-02T09:02:08Z

src/app-layer-htp-range.h

+ * As this belongs to a flow, appending data to it is ensured to be thread-safe
+ * Only one block per file has the pointer to the container
+ */
+typedef struct HttpRangeContainerBlock {


after our conversation the other day I expected a FileContainer pointer to be here, where the flow handling the in-order chunk would move the FileContainer pointer into this block and return it to the flow on close.

I had understood a FileContainer pointer or a boolean...
Switching to the FileContainer pointer then ;-)

victorjulien · 2021-09-02T09:05:54Z

src/app-layer-htp-range.c

+            DEBUG_VALIDATE_BUG_ON(c->container->files == NULL);
+            if (data == NULL) {
+                // gap overlaping already known data
+                r = FileAppendData(c->container->files, NULL, len - c->toskip);


it looks like this is accessed w/o a lock in the HTPFileStoreChunk path. It may work due to c->fileOwning, but I'm not happy with this setup. I don't want to see any access of a shared data structure w/o holding a lock.

Like I wrote in another comment, I thought the idea was to move c->container->files into HttpRangeContainerBlock for the flow handling the in-order chunk. That way we can append to it w/o needing to access the shared data at c->container

Reworking with the FileContainer pointer from comment above

catenacyber · 2021-09-02T15:24:33Z

Replaced by #6331

Ticket: OISF#6312

victorjulien and others added 8 commits August 24, 2021 10:20

detect: suppress scan-build warning

f3bcab3

util: adds util function SCBufferCmp

d6c6a91

Compares two buffers with their two sizes

util: export Djb2 hash string function

470241c

http/range: fix memory leak on out of order ranges

92c3ce9

http/range: optimize struct layout

c705fe8

http/range: cleanup and simplification

879c76a

Simplify locking by using the THashData lock instead of a separate range lock. Avoid size_t in function arguments. Clean up file handling functions. Implement handling of alloc errors. Rename yaml entry to byterange Unify public api naming

thash: add debug validation check for use_cnt

9680650

catenacyber requested a review from a team as a code owner August 27, 2021 15:30

catenacyber mentioned this pull request Aug 27, 2021

Feature/http range/v22 #6308

Closed

catenacyber force-pushed the feature/http-range/v23 branch 2 times, most recently from b46d128 to 75ce5e6 Compare August 27, 2021 15:31

catenacyber commented Aug 27, 2021

View reviewed changes

ci: dummy git configuration for rebase

0c96b5f

catenacyber requested a review from victorjulien as a code owner August 27, 2021 15:37

fixup warnings

ef29258

fixup SV

08eca7e

victorjulien marked this pull request as draft September 2, 2021 08:46

victorjulien requested changes Sep 2, 2021

View reviewed changes

catenacyber mentioned this pull request Sep 2, 2021

Feature/http range/v24 #6331

Closed

catenacyber closed this Sep 2, 2021

AkakiAlice added a commit to AkakiAlice/suricata that referenced this pull request Oct 22, 2024

detect/analyzer: add more details for flow_age

4a1f9a3

Ticket: OISF#6312

AkakiAlice mentioned this pull request Oct 22, 2024

detect/analyzer: add more details for flow_age - v1 #12011

Closed

5 tasks

victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Oct 24, 2024

detect/analyzer: add more details for flow_age

5e563b0

Ticket: OISF#6312

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/http range/v23 #6312

Feature/http range/v23 #6312

catenacyber commented Aug 27, 2021

catenacyber Aug 27, 2021

victorjulien Sep 2, 2021

catenacyber Sep 2, 2021

victorjulien Sep 2, 2021

catenacyber Sep 2, 2021

catenacyber commented Aug 27, 2021

catenacyber commented Aug 27, 2021

codecov bot commented Aug 27, 2021

suricata-qa commented Aug 28, 2021

victorjulien Sep 2, 2021

catenacyber Sep 2, 2021

victorjulien Sep 2, 2021

catenacyber Sep 2, 2021

victorjulien Sep 2, 2021

victorjulien Sep 2, 2021

catenacyber Sep 2, 2021

victorjulien Sep 2, 2021

catenacyber Sep 2, 2021

catenacyber commented Sep 2, 2021

Feature/http range/v23 #6312

Feature/http range/v23 #6312

Conversation

catenacyber commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber commented Aug 27, 2021

catenacyber commented Aug 27, 2021

codecov bot commented Aug 27, 2021

Codecov Report

suricata-qa commented Aug 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber commented Sep 2, 2021