Skip to content

Warnint 64to32 6186 v23 #12843

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Warnint 64to32 6186 v23 #12843

wants to merge 2 commits into from

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • fix -Wshorten-64-to-32 warnings for remaining files : detect

Last commit of #9840 after #12633 merge

Still to do afterwards :

  • CI check

catenacyber
catenacyber commented Mar 27, 2025
View reviewed changes
src/datasets.c
@@ -1373,7 +1373,7 @@ static int DatasetOpSerialized(Dataset *set, const char *string, DatasetOpFunc D

switch (set->type) {
case DATASET_TYPE_STRING: {
uint32_t decoded_size = SCBase64DecodeBufferSize(strlen(string));
uint32_t decoded_size = SCBase64DecodeBufferSize((uint32_t)strlen(string));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

comes from unix socket

Copy link
Member

Choose a reason for hiding this comment

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

do we impose some limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding one as there is also the stack allocation below

catenacyber
catenacyber commented Mar 27, 2025
View reviewed changes
src/detect-engine-frame.c
if (data_offset > UINT32_MAX) {
SCLogDebug("DETECT_ENGINE_INSPECT_SIG_NO_MATCH data_offset > UINT32_MAX");
return more_chunks;
}
const bool match = DetectEngineContentInspection(det_ctx->de_ctx, det_ctx, s, engine->smd, p,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about DetectEngineContentInspection taking a u32 as input but every caller seems to pass a u64 ?

Copy link
Member

Choose a reason for hiding this comment

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

the size of the data passed should never even get close to the limit of a u32. But we do work with offsets that are higher, like in streaming buffers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it must be broken currently as DetectEngineContentInspection silently casts the u64 to u32, am I getting it right ?

catenacyber
catenacyber commented Mar 27, 2025
View reviewed changes
src/detect-engine-threshold.c
@@ -82,7 +82,7 @@ void ThresholdDestroy(void)
typedef struct ThresholdEntry_ {
uint32_t key[5];

uint32_t tv_timeout; /**< Timeout for new_action (for rate_filter)
uint64_t tv_timeout; /**< Timeout for new_action (for rate_filter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jlucovsky do you have better ideas for these u32 times ?

Copy link
Member

Choose a reason for hiding this comment

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

SCTime_t is also 64bit, but then with precision

catenacyber
catenacyber commented Mar 27, 2025
View reviewed changes
src/detect-file-data.c
@@ -425,8 +425,12 @@ uint8_t DetectEngineInspectFiledata(DetectEngineCtx *de_ctx, DetectEngineThreadC
if (buffer->inspect_offset == 0)
ciflags |= DETECT_CI_FLAGS_START;

if (buffer->inspect_offset > UINT32_MAX) {
local_file_id++;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

still the bug above that we do not local_file_id++; before continue

catenacyber
catenacyber commented Mar 27, 2025
View reviewed changes
src/detect-flow-age.c
uint64_t age = SCTIME_SECS(p->flow->lastts) - SCTIME_SECS(p->flow->startts);
if (age > UINT32_MAX) {
age = UINT32_MAX;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we change flow.age behavior for a u64 ? cc @inashivb

Copy link
Member

Choose a reason for hiding this comment

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

think we'll have many flows that will be longer than a 100 years?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can craft a pcap file like that but should I...

@catenacyber
Copy link
Contributor Author

How should I label this PR as non urgent because not for 8beta ?

victorjulien
victorjulien reviewed Mar 27, 2025
View reviewed changes
src/detect-engine-content-inspection.c
if (det_ctx->byte_values[cd->offset] > offset)
offset = det_ctx->byte_values[cd->offset];
if (det_ctx->byte_values[cd->offset] > offset) {
if (det_ctx->byte_values[cd->offset] > UINT32_MAX) {
Copy link
Member

Choose a reason for hiding this comment

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

how would this happen? Can we avoid it from getting set? It seems painful to put all these checks here if we can avoid it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think with a rule using byte_extract with 8 bytes..?

victorjulien
victorjulien reviewed Mar 27, 2025
View reviewed changes
src/detect-filename.c
@@ -264,8 +264,12 @@ static uint8_t DetectEngineInspectFilename(DetectEngineCtx *de_ctx, DetectEngine
if (buffer == NULL)
continue;

if (buffer->inspect_offset > UINT32_MAX) {
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike these kind of changes, as it should have been limited before. This all adds runtime overhead.

victorjulien
victorjulien reviewed Mar 27, 2025
View reviewed changes
src/detect-http-header.c
DETECT_ENGINE_CONTENT_INSPECTION_MODE_STATE);
if (match) {
return DETECT_ENGINE_INSPECT_SIG_MATCH;
if (offset <= UINT32_MAX) {
Copy link
Member

Choose a reason for hiding this comment

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

an offset > 4GiB can be valid, even though the data we get will be much smaller. It will be the data at that offset

@suricata-qa
Copy link

ERROR:
ERROR: SEGMENTATION FAULT in ASAN_TLPR1_cfg QA test

ERROR: QA failed on ASAN_TLPR1_cfg.

Pipeline 25373

@catenacyber catenacyber marked this pull request as draft March 29, 2025 09:42
@catenacyber
Copy link
Contributor Author

Converting to draft for now.

I guess I will split in multiple smaller PRs next...

@catenacyber
Copy link
Contributor Author

New version with only the simple changes ready in warnint-64to32-6186-v24.1

@catenacyber catenacyber added the needs rebase Needs rebase to master label Apr 3, 2025
@catenacyber catenacyber mentioned this pull request Apr 6, 2025
Closed
@catenacyber
Copy link
Contributor Author

Next version in #12961

@catenacyber catenacyber closed this Apr 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victorjulien victorjulien victorjulien left review comments

Assignees
No one assigned
Labels
needs rebase Needs rebase to master
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@catenacyber @suricata-qa @victorjulien