Conversation
| switch (set->type) { | ||
| case DATASET_TYPE_STRING: { | ||
| uint32_t decoded_size = SCBase64DecodeBufferSize(strlen(string)); | ||
| uint32_t decoded_size = SCBase64DecodeBufferSize((uint32_t)strlen(string)); |
There was a problem hiding this comment.
comes from unix socket
There was a problem hiding this comment.
Adding one as there is also the stack allocation below
| 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, |
There was a problem hiding this comment.
What do you think about DetectEngineContentInspection taking a u32 as input but every caller seems to pass a u64 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So it must be broken currently as DetectEngineContentInspection silently casts the u64 to u32, am I getting it right ?
| 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) |
There was a problem hiding this comment.
@jlucovsky do you have better ideas for these u32 times ?
There was a problem hiding this comment.
SCTime_t is also 64bit, but then with precision
| ciflags |= DETECT_CI_FLAGS_START; | ||
|
|
||
| if (buffer->inspect_offset > UINT32_MAX) { | ||
| local_file_id++; |
There was a problem hiding this comment.
still the bug above that we do not local_file_id++; before continue
| uint64_t age = SCTIME_SECS(p->flow->lastts) - SCTIME_SECS(p->flow->startts); | ||
| if (age > UINT32_MAX) { | ||
| age = UINT32_MAX; | ||
| } |
There was a problem hiding this comment.
Should we change flow.age behavior for a u64 ? cc @inashivb
There was a problem hiding this comment.
think we'll have many flows that will be longer than a 100 years?
There was a problem hiding this comment.
I can craft a pcap file like that but should I...
|
How should I label this PR as non urgent because not for 8beta ? |
| 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think with a rule using byte_extract with 8 bytes..?
| if (buffer == NULL) | ||
| continue; | ||
|
|
||
| if (buffer->inspect_offset > UINT32_MAX) { |
There was a problem hiding this comment.
I really dislike these kind of changes, as it should have been limited before. This all adds runtime overhead.
| DETECT_ENGINE_CONTENT_INSPECTION_MODE_STATE); | ||
| if (match) { | ||
| return DETECT_ENGINE_INSPECT_SIG_MATCH; | ||
| if (offset <= UINT32_MAX) { |
There was a problem hiding this comment.
an offset > 4GiB can be valid, even though the data we get will be much smaller. It will be the data at that offset
|
ERROR: ERROR: QA failed on ASAN_TLPR1_cfg. Pipeline 25373 |
|
Converting to draft for now. I guess I will split in multiple smaller PRs next... |
|
New version with only the simple changes ready in warnint-64to32-6186-v24.1 |
|
Next version in #12961 |
3 participants
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6186
Describe changes:
-Wshorten-64-to-32warnings for remaining files : detectLast commit of #9840 after #12633 merge
Still to do afterwards :