Warnint util 4516 v5 #7006

catenacyber · 2022-02-15T17:11:31Z

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

Describe changes:

Fix integer warnings -Wimplicit-int-conversion in all files

Extends #6792 : complete with CI check...

Replaces #6820 with CIFuzz fix for defrag

Thoughts about the commit history ?
This PR can be split into smaller PRs as some points need a careful review (detect-content.c for instance)

victorjulien · 2022-02-15T19:01:25Z

Converted to draft due to "fixup" commit.

Ticket: 4516

catenacyber · 2022-02-15T21:21:37Z

Converted to draft due to "fixup" commit.

Ah I had indeed forgotten to fix it up in its real commit

victorjulien · 2022-02-15T19:12:48Z

src/detect-engine-prefilter.c

@@ -286,7 +286,9 @@ int PrefilterAppendTxEngine(DetectEngineCtx *de_ctx, SigGroupHead *sgh,
    e->PrefilterTx = PrefilterTxFunc;
    e->pectx = pectx;
    e->alproto = alproto;
-    e->tx_min_progress = tx_min_progress;
+    // TODO change function prototype ?


That would be good, but lets do that separately. So ticket?

victorjulien · 2022-02-15T19:36:36Z

src/detect-flow.c

@@ -449,17 +448,15 @@ static void
 PrefilterPacketFlowSet(PrefilterPacketHeaderValue *v, void *smctx)
 {
    const DetectFlowData *fb = smctx;
-    v->u8[0] = fb->flags;
-    v->u8[1] = fb->match_cnt;
+    v->u16[0] = fb->flags;


this seems like a real bug that would need a ticket and backports

victorjulien · 2022-02-15T20:28:05Z

src/detect-isdataat.c

@@ -156,8 +156,7 @@ static DetectIsdataatData *DetectIsdataatParse (DetectEngineCtx *de_ctx, const c
            if (*offset == NULL)
                goto error;
        } else {
-            if (StringParseUint16(&idad->dataat, 10,
-                                        strlen(args[0]), args[0]) < 0 ) {
+            if (StringParseUint16(&idad->dataat, 10, (uint16_t)strlen(args[0]), args[0]) < 0) {


can we avoid these casts somehow? The uint16_t seems a bit arbitrary, the max accepted value is 23 so turning it into uint8_t might make sense but then again I think size_t might make even more sense to we can just pass strlen() directly into it.

victorjulien · 2022-02-15T20:30:49Z

src/detect-transform-urldecode.c

@@ -86,7 +86,9 @@ static bool BufferUrlDecode(const uint8_t *input, const uint32_t input_len, uint
            if (i + 2 < input_len) {
                if ((isxdigit(input[i+1])) && (isxdigit(input[i+2]))) {
                    // Decode %HH encoding.
-                    *oi = (input[i+1] >= 'A' ? ((input[i+1] & 0xdf) - 'A') + 10 : (input[i+1] - '0')) << 4;
+                    *oi = (uint8_t)((input[i + 1] >= 'A' ? ((input[i + 1] & 0xdf) - 'A') + 10


wow clang-format... really? Can't say this improves things. Might even opt for a clang-format exception here

victorjulien · 2022-02-15T20:35:21Z

src/flow.h

@@ -580,7 +580,7 @@ int FlowGetPacketDirection(const Flow *, const Packet *);

 void FlowCleanupAppLayer(Flow *);

-void FlowUpdateState(Flow *f, enum FlowState s);
+void FlowUpdateState(Flow *f, const FlowStateType s);


the FlowStateType is the more compact version, but in function args I prefer a real enum to the compiler can check cases and such. Can we keep that here?

What do you expect from the compiler ?

With current code (ie enum FlowState s)
FlowUpdateState(p->flow, 123456); gets no warning on my setup

(And I do not get more warnings with this change, like clang is ok to pass 123456 to an uint16_t )

Tried something here #7120

victorjulien · 2022-02-15T21:08:48Z

src/util-decode-mime.c

@@ -686,7 +686,11 @@ static uint8_t * GetFullValue(DataValue *dv, uint32_t *len)
 static inline uint8_t * FindBuffer(const uint8_t *src, uint32_t len, const uint8_t *find, uint32_t find_len)
 {
    /* Use utility search function */
-    return BasicSearchNocase(src, len, find, find_len);
+    if (find_len > UINT16_MAX) {
+        // TODO rewrite all this to have app-layer events


Thanks for pointing this
I did #7074
I managed to get rid of this check at this level (like find_len is always small enough) so I do not think we need a ticket for this

suricata-qa · 2022-02-27T05:10:03Z

ERROR:

ERROR: QA failed on build_asan.

Pipeline 6404

catenacyber · 2022-02-28T08:15:55Z

Status : splitting this in multiple PRs

style: use u8_tolower everywhere #7073 easy fix (tolower)
util/mime: fix integer warnings #7074 complex fix (util-decode-mime)

Waiting to rebase on what gets merged
Keeping this open with the rest of the code changes to get, and comments to take into account

catenacyber · 2022-06-03T07:24:30Z

Finally replaced by #7470

catenacyber requested review from victorjulien and a team as code owners February 15, 2022 17:11

catenacyber mentioned this pull request Feb 15, 2022

Warnint util 4516 v4 #6820

Closed

victorjulien marked this pull request as draft February 15, 2022 19:01

catenacyber added 14 commits February 15, 2022 22:21

util: fix int warnings

2f6b86f

Ticket: 4516

util: fix int warnings in unit tests

1993b12

Ticket: 4516

util: fix int warnings in tm threads

eefaea0

Ticket: 4516

stream-tcp: fix integer warnings

815a956

Ticket: 4516

utils: fix integer warnings in r files

dfcf6b4

Ticket: 4516

fuzz: fix integer warnings

6d0d375

Ticket: 4516

output: fix integer warnings

5d872a4

Ticket: 4516

flow: fix integer warnings

a34c369

Ticket: 4516

app-layer: fix integer warnings

5dbb6eb

Ticket: 4516

decode: fix integer warnings

c8da8c5

Ticket: 4516

defrag: fix integer warnings

9aa7377

Ticket: 4516

detect: fix integer warnings

f9bb9ea

Ticket: 4516

ci: adds warning flag about integer conversions

fa747c7

defrag: use u32 for length

08eb259

catenacyber force-pushed the warnint-util-4516-v5 branch from 8264474 to 08eb259 Compare February 15, 2022 21:21

victorjulien reviewed Feb 15, 2022

View reviewed changes

catenacyber mentioned this pull request Feb 17, 2022

app-layer: fix integer warnings #6649

Closed

This was referenced Feb 28, 2022

style: use u8_tolower everywhere #7073

Closed

util/mime: fix integer warnings #7074

Merged

This was referenced Feb 28, 2022

src: use u8_tolower everywhere #7081

Closed

Warnint util 4516 v7.1 #7100

Closed

Warnint util 4516 v7.2 #7102

Closed

This was referenced Mar 4, 2022

Warnint util 4516 v7.3 #7107

Closed

Warnint util 4516 v8.1 #7120

Closed

This was referenced Apr 8, 2022

output: fix integer warnings #7219

Merged

Warnint util 4516 v9.1 #7239

Closed

catenacyber mentioned this pull request Apr 25, 2022

decode: fix integer warnings #7298

Closed

suricata-qa added the needs rebase Needs rebase to master label Apr 25, 2022

catenacyber mentioned this pull request May 2, 2022

Warnint util 4516 v11.1 #7351

Closed

catenacyber mentioned this pull request Jun 3, 2022

Warnint util 4516 v12.1 #7470

Closed

catenacyber closed this Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warnint util 4516 v5 #7006

Warnint util 4516 v5 #7006

catenacyber commented Feb 15, 2022

victorjulien commented Feb 15, 2022

catenacyber commented Feb 15, 2022

victorjulien Feb 15, 2022

victorjulien Feb 15, 2022

victorjulien Feb 15, 2022

victorjulien Feb 15, 2022

victorjulien Feb 15, 2022

catenacyber Mar 3, 2022

catenacyber Mar 6, 2022

victorjulien Feb 15, 2022

catenacyber Feb 28, 2022

suricata-qa commented Feb 27, 2022

catenacyber commented Feb 28, 2022

catenacyber commented Jun 3, 2022

Warnint util 4516 v5 #7006

Warnint util 4516 v5 #7006

Conversation

catenacyber commented Feb 15, 2022

victorjulien commented Feb 15, 2022

catenacyber commented Feb 15, 2022

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

suricata-qa commented Feb 27, 2022

catenacyber commented Feb 28, 2022

catenacyber commented Jun 3, 2022