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.

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

Cppcheck/v1 #11946

Closed
wants to merge 6 commits into from
Closed

Cppcheck/v1 #11946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b259cd
stream: track pcap log segments timestamp with SCTime_t
victorjulien Oct 12, 2024
0a5de98
time: remove unused function
victorjulien Oct 12, 2024
80ed2e3
util/hash-string: fix compile warning
victorjulien Oct 12, 2024
8cd1d31
time: add SCTIME_CMP_EQ
victorjulien Sep 18, 2024
c245e70
threads: track wall clock time precisely
victorjulien Sep 15, 2024
bd72dc0
util/var-names: switch to SCTime_t
victorjulien Oct 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/log-pcap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,10 @@ static int PcapLogSegmentCallback(
struct PcapLogCallbackContext *pctx = (struct PcapLogCallbackContext *)data;

if (seg->pcap_hdr_storage->pktlen) {
pctx->pl->h->ts.tv_sec = seg->pcap_hdr_storage->ts.tv_sec;
pctx->pl->h->ts.tv_usec = seg->pcap_hdr_storage->ts.tv_usec;
struct timeval tv;
SCTIME_TO_TIMEVAL(&tv, seg->pcap_hdr_storage->ts);
pctx->pl->h->ts.tv_sec = tv.tv_sec;
pctx->pl->h->ts.tv_usec = tv.tv_usec;
pctx->pl->h->len = seg->pcap_hdr_storage->pktlen + buflen;
pctx->pl->h->caplen = seg->pcap_hdr_storage->pktlen + buflen;
MemBufferReset(pctx->buf);
Expand Down
9 changes: 3 additions & 6 deletions src/stream-tcp-list.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,8 +569,7 @@ static int DoHandleData(ThreadVars *tv, TcpReassemblyThreadCtx *ra_ctx,
static void StreamTcpSegmentAddPacketDataDo(TcpSegment *seg, const Packet *rp, const Packet *pp)
{
if (GET_PKT_DATA(rp) != NULL && GET_PKT_LEN(rp) > pp->payload_len) {
seg->pcap_hdr_storage->ts.tv_sec = SCTIME_SECS(rp->ts);
seg->pcap_hdr_storage->ts.tv_usec = SCTIME_USECS(rp->ts);
seg->pcap_hdr_storage->ts = rp->ts;
seg->pcap_hdr_storage->pktlen = GET_PKT_LEN(rp) - pp->payload_len;
/*
* pkt_hdr members are initially allocated 64 bytes of memory. Thus,
Expand All @@ -582,8 +581,7 @@ static void StreamTcpSegmentAddPacketDataDo(TcpSegment *seg, const Packet *rp, c
seg->pcap_hdr_storage->alloclen, seg->pcap_hdr_storage->pktlen);
if (tmp_pkt_hdr == NULL) {
SCLogDebug("Failed to realloc");
seg->pcap_hdr_storage->ts.tv_sec = 0;
seg->pcap_hdr_storage->ts.tv_usec = 0;
seg->pcap_hdr_storage->ts = SCTIME_INITIALIZER;
seg->pcap_hdr_storage->pktlen = 0;
return;
} else {
Expand All @@ -594,8 +592,7 @@ static void StreamTcpSegmentAddPacketDataDo(TcpSegment *seg, const Packet *rp, c
memcpy(seg->pcap_hdr_storage->pkt_hdr, GET_PKT_DATA(rp),
(size_t)GET_PKT_LEN(rp) - pp->payload_len);
} else {
seg->pcap_hdr_storage->ts.tv_sec = 0;
seg->pcap_hdr_storage->ts.tv_usec = 0;
seg->pcap_hdr_storage->ts = SCTIME_INITIALIZER;
seg->pcap_hdr_storage->pktlen = 0;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/stream-tcp-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ RB_PROTOTYPE(TCPSACK, StreamTcpSackRecord, rb, TcpSackCompare);
* used if the session-dump option is enabled.
*/
typedef struct TcpSegmentPcapHdrStorage_ {
struct timeval ts;
SCTime_t ts;
uint32_t pktlen;
uint32_t alloclen;
uint8_t *pkt_hdr;
Expand Down
4 changes: 2 additions & 2 deletions src/stream-tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7022,8 +7022,8 @@ int StreamTcpSegmentForSession(
}
server_node = TCPSEG_RB_NEXT(server_node);
} else {
if (TimevalEarlier(
&client_node->pcap_hdr_storage->ts, &server_node->pcap_hdr_storage->ts)) {
if (SCTIME_CMP_LT(
client_node->pcap_hdr_storage->ts, server_node->pcap_hdr_storage->ts)) {
StreamingBufferSegmentGetData(
&client_stream->sb, &client_node->sbseg, &seg_data, &seg_datalen);
ret = CallbackFunc(p, client_node, data, seg_data, seg_datalen);
Expand Down
36 changes: 23 additions & 13 deletions src/tm-threads.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2075,7 +2075,7 @@ typedef struct Thread_ {

SCTime_t pktts; /**< current packet time of this thread
* (offline mode) */
uint32_t sys_sec_stamp; /**< timestamp in seconds of the real system
SCTime_t sys_sec_stamp; /**< timestamp in real system
* time when the pktts was last updated. */
} Thread;

Expand Down Expand Up @@ -2199,14 +2199,24 @@ void TmThreadsSetThreadTimestamp(const int id, const SCTime_t ts)
int idx = id - 1;
Thread *t = &thread_store.threads[idx];
t->pktts = ts;
struct timeval systs;
gettimeofday(&systs, NULL);
t->sys_sec_stamp = (uint32_t)systs.tv_sec;
struct timeval tv;
gettimeofday(&tv, NULL);
SCTime_t now = SCTIME_FROM_TIMEVAL(&tv);

if (t->sys_sec_stamp.secs != 0) {
SCTime_t tmpts = SCTIME_ADD_SECS(t->sys_sec_stamp, 3);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for using ~4 seconds == long time?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's ore a left over of some debugging where I wanted to know the effect of inactive threads on the timestamp the FM used. Turned it into debug.

if (SCTIME_CMP_LT(tmpts, now)) {
SCLogNotice("%s: thread slept for %u secs", t->name, (uint32_t)(now.secs - tmpts.secs));
Copy link
Member

Choose a reason for hiding this comment

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

SCLogWarning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure actually, it's not necessarily an issue. In a large pcap that starts with a single flow you might see this once packets for other flows are processed. I'll turn it into debug for now.

}
}

t->sys_sec_stamp = SCTIME_FROM_TIMEVAL(&tv);
SCMutexUnlock(&thread_store_lock);
}

bool TmThreadsTimeSubsysIsReady(void)
{
static SCTime_t nullts;
Copy link
Member

Choose a reason for hiding this comment

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

Should we use SCTIME_INITIALIZER just to be sure?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

bool ready = true;
SCMutexLock(&thread_store_lock);
for (size_t s = 0; s < thread_store.threads_size; s++) {
Expand All @@ -2215,7 +2225,7 @@ bool TmThreadsTimeSubsysIsReady(void)
break;
if (t->type != TVT_PPT)
continue;
if (t->sys_sec_stamp == 0) {
if (SCTIME_CMP_EQ(t->sys_sec_stamp, nullts)) {
ready = false;
break;
}
Expand All @@ -2236,19 +2246,20 @@ void TmThreadsInitThreadsTimestamp(const SCTime_t ts)
if (t->type != TVT_PPT)
continue;
t->pktts = ts;
t->sys_sec_stamp = (uint32_t)systs.tv_sec;
t->sys_sec_stamp = SCTIME_FROM_TIMEVAL(&systs);
}
SCMutexUnlock(&thread_store_lock);
}

void TmThreadsGetMinimalTimestamp(struct timeval *ts)
{
struct timeval local = { 0 };
static struct timeval nullts;
static SCTime_t nullts;
bool set = false;
size_t s;
struct timeval systs;
gettimeofday(&systs, NULL);
struct timeval tv;
gettimeofday(&tv, NULL);
SCTime_t systs = SCTIME_FROM_TIMEVAL(&tv);

SCMutexLock(&thread_store_lock);
for (s = 0; s < thread_store.threads_size; s++) {
Expand All @@ -2258,11 +2269,10 @@ void TmThreadsGetMinimalTimestamp(struct timeval *ts)
/* only packet threads set timestamps based on packets */
if (t->type != TVT_PPT)
continue;
struct timeval pkttv = { .tv_sec = SCTIME_SECS(t->pktts),
.tv_usec = SCTIME_USECS(t->pktts) };
if (!(timercmp(&pkttv, &nullts, ==))) {
if (SCTIME_CMP_NEQ(t->pktts, nullts)) {
SCTime_t sys_sec_stamp = SCTIME_ADD_SECS(t->sys_sec_stamp, 1);
/* ignore sleeping threads */
if (t->sys_sec_stamp + 1 < (uint32_t)systs.tv_sec)
if (SCTIME_CMP_LT(sys_sec_stamp, systs))
continue;

if (!set) {
Expand Down
4 changes: 2 additions & 2 deletions src/util-hash-string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ uint32_t StringHashFunc(HashTable *ht, void *data, uint16_t datalen)
char StringHashCompareFunc(void *data1, uint16_t datalen1,
void *data2, uint16_t datalen2)
{
int len1 = strlen((char *)data1);
int len2 = strlen((char *)data2);
size_t len1 = strlen((char *)data1);
size_t len2 = strlen((char *)data2);

if (len1 == len2 && memcmp(data1, data2, len1) == 0) {
return 1;
Expand Down
10 changes: 1 addition & 9 deletions src/util-time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ typedef struct {
#define SCTIME_CMP_LT(a, b) SCTIME_CMP((a), (b), <)
#define SCTIME_CMP_LTE(a, b) SCTIME_CMP((a), (b), <=)
#define SCTIME_CMP_NEQ(a, b) SCTIME_CMP((a), (b), !=)
#define SCTIME_CMP_EQ(a, b) SCTIME_CMP((a), (b), ==)

void TimeInit(void);
void TimeDeinit(void);
Expand All @@ -115,15 +116,6 @@ SCTime_t TimeGet(void);
/** \brief initialize a 'struct timespec' from a 'struct timeval'. */
#define FROM_TIMEVAL(timev) { .tv_sec = (timev).tv_sec, .tv_nsec = (timev).tv_usec * 1000 }

/** \brief compare two 'struct timeval' and return if the first is earlier than the second */
static inline bool TimevalEarlier(struct timeval *first, struct timeval *second)
{
/* from man timercmp on Linux: "Some systems (but not Linux/glibc), have a broken timercmp()
* implementation, in which CMP of >=, <=, and == do not work; portable applications can instead
* use ... !timercmp(..., >) */
return !timercmp(first, second, >);
}

#ifndef timeradd
#define timeradd(a, b, r) \
do { \
Expand Down
21 changes: 10 additions & 11 deletions src/util-var-name.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ typedef struct VarNameStore_ {
HashListTable *names;
HashListTable *ids;
uint32_t max_id;
struct timeval free_after;
SCTime_t free_after;
TAILQ_ENTRY(VarNameStore_) next;
} VarNameStore;
typedef VarNameStore *VarNameStorePtr;
Expand Down Expand Up @@ -260,13 +260,11 @@ int VarNameStoreActivate(void)
if (new_active) {
VarNameStore *old_active = SC_ATOMIC_GET(active);
if (old_active) {
struct timeval ts, add;
struct timeval ts;
memset(&ts, 0, sizeof(ts));
memset(&add, 0, sizeof(add));
gettimeofday(&ts, NULL);
add.tv_sec = 60;
timeradd(&ts, &add, &ts);
old_active->free_after = ts;
SCTime_t free_after = SCTIME_ADD_SECS(SCTIME_FROM_TIMEVAL(&ts), 60);
old_active->free_after = free_after;

TAILQ_INSERT_TAIL(&free_list, old_active, next);
SCLogDebug("old active is stored in free list");
Expand All @@ -275,16 +273,17 @@ int VarNameStoreActivate(void)
SC_ATOMIC_SET(active, new_active);
SCLogDebug("new store active");

struct timeval now;
memset(&now, 0, sizeof(now));
gettimeofday(&now, NULL);
struct timeval tv;
memset(&tv, 0, sizeof(tv));
Copy link
Member

Choose a reason for hiding this comment

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

Is the memset needed? If yes, it seems to be missing in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have it in other places, see

git grep -B1 gettimeofday|grep memset
src/source-ipfw.c-        memset (&IPFWts, 0, sizeof(struct timeval));
src/source-nflog.c-        memset(&tv, 0, sizeof(tv));
src/source-nfq.c-        memset(&tv, 0, sizeof(tv));
src/suricata.c-    memset(&suri->start_time, 0, sizeof(suri->start_time));
src/suricata.c-    memset(&end_time, 0, sizeof(end_time));
src/util-random.c-    memset(&tv, 0, sizeof(tv));
src/util-var-name.c-            memset(&ts, 0, sizeof(ts));
src/util-var-name.c-        memset(&tv, 0, sizeof(tv));

However I agree it's probably not needed as there are many more places were it's not done. Not sure how I developed this as a half baked pattern.

gettimeofday(&tv, NULL);
SCTime_t now = SCTIME_FROM_TIMEVAL(&tv);

VarNameStore *s = NULL;
while ((s = TAILQ_FIRST(&free_list))) {
char timebuf[64];
CreateIsoTimeString(SCTIME_FROM_TIMEVAL(&s->free_after), timebuf, sizeof(timebuf));
CreateIsoTimeString(s->free_after, timebuf, sizeof(timebuf));

if (!timercmp(&now, &s->free_after, >)) {
if (SCTIME_CMP_LTE(now, s->free_after)) {
SCLogDebug("not yet freeing store %p before %s", s, timebuf);
break;
}
Expand Down
Loading