Skip to content

ENT-6511: Increase cf-monitord measurement slots from 100 to 300 #6239

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

Open
nickanderson wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ENT-6511: Increase cf-monitord measurement slots from 100 to 300 #6239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
469f094
ENT-6511: Increase cf-monitord measurement slots from 100 to 300
nickanderson Jul 20, 2026
d278e7e
Added a DB migration for the enlarged Averages struct
nickanderson Jul 21, 2026
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
2 changes: 1 addition & 1 deletion cf-check/db_structs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ static const char *const observable_strings[] =
};

// Not the actual count, just the room we set aside in struct (and LMDB):
#define CF_OBSERVABLES 100
#define CF_OBSERVABLES 300

typedef struct Averages
{
Expand Down
13 changes: 12 additions & 1 deletion cf-check/dump.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,18 @@ static void print_struct_or_string(
{
if (structs)
{
if (StringContains(file, "cf_lastseen.lmdb")
if ((StringContains(file, "cf_observations.lmdb")

@larsewi larsewi Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pre-existing. But, should probably be StringEndsWith()? Could be a follow-up commit.

@nickanderson nickanderson Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

in this PR or completely separately?

@larsewi larsewi Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Up to you. I'm OK with leaving it as is as well

@nickanderson nickanderson Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think I will leave it for now. Seems to have been intentionally changed from StringsEndsWith to StringContains 3e98366c3

|| StringContains(file, "history.lmdb"))
&& StringEqual(key.mv_data, "version"))
{
// After the CF_OBSERVABLES migration (dbm_migration_observations.c)
// these DBs hold a "version" bookkeeping key whose value is a short
// version string, not an Averages struct. Print it as a string;
// otherwise it would reach print_struct_averages() and trip its
// struct-size assertion.
print_json_string(value.mv_data, value.mv_size, strip_strings);
}
else if (StringContains(file, "cf_lastseen.lmdb")
&& StringStartsWith(key.mv_data, "q"))
{
print_struct_lastseen_quality(value, strip_strings);
Expand Down
21 changes: 17 additions & 4 deletions cf-check/observables.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,23 @@ char **GetObservableNames(const char *ts_key_path)

if (fgets(line, CF_MAXVARSIZE, f) == NULL)
{
Log(LOG_LEVEL_ERR,
"Error trying to read ts_key from file '%s'. (fgets: %s)",
filename,
GetErrorStr());
if (ferror(f))
{
Log(LOG_LEVEL_ERR,
"Error trying to read ts_key from file '%s'. (fgets: %s)",
filename,
GetErrorStr());
}
/* The ts_key has fewer entries than CF_OBSERVABLES -- e.g. it
* was written by an older agent with a smaller CF_OBSERVABLES.
* Fill the remaining names so the returned array keeps its
* documented non-NULL guarantee (callers index all
* CF_OBSERVABLES entries). */
for (int j = i; j < CF_OBSERVABLES; ++j)
{
snprintf(buf, CF_MAXVARSIZE, "spare[%d]", j);
temp[j] = xstrdup(buf);
}
Comment on lines +77 to +86

@larsewi larsewi Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this still needed post migration fix ?

@nickanderson nickanderson Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes — this is about the ts_key text file, which the DB migration does not touch. GetObservableNames() is documented to return a CF_OBSERVABLES-sized array of non-NULL strings. A ts_key written by an older agent has only 100 lines, so after the bump fgets hits EOF at slot 100 and, without this fill, temp[100..299] stay uninitialized (xmalloc, not zeroed); the sole caller (dump.c:179) then indexes and free()s all 300 → uninitialized read + invalid free. So it is an independent correctness fix.

@nickanderson nickanderson Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

And, just FYI, I have a follow-up PR waiting to changes the storage to only require what is used

break;
}

Expand Down
2 changes: 1 addition & 1 deletion cf-check/observables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#include <sequence.h>

// copy of libpromises/cf3.defs.h, TODO refactor
#define CF_OBSERVABLES 100
#define CF_OBSERVABLES 300

char **GetObservableNames(const char *ts_key_path);

Expand Down
1 change: 1 addition & 0 deletions libpromises/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ libpromises_la_SOURCES = \
dbm_api.c dbm_api.h dbm_api_types.h dbm_priv.h \
dbm_migration.c dbm_migration.h \
dbm_migration_lastseen.c \
dbm_migration_observations.c \
dbm_lmdb.c \
dbm_quick.c \
dbm_tokyocab.c \
Expand Down
2 changes: 1 addition & 1 deletion libpromises/cf3.defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ typedef enum
#define CF_MEASURE_INTERVAL (5.0*60.0)
#define CF_SHIFT_INTERVAL (6*3600)

#define CF_OBSERVABLES 100
#define CF_OBSERVABLES 300

/* Special exit codes */
#define EC_EVAL_ABORTED 6 /* like SIGABRT, but signal exit codes are 120+SIG */
Expand Down
204 changes: 204 additions & 0 deletions libpromises/constants.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,208 @@ const char *const OBSERVABLES[CF_OBSERVABLES][2] =
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
/* ENT-6511: slots 100-299, added when CF_OBSERVABLES was raised
100->300. Every slot must have a name entry: cf-monitord's
GetObservable() falls back to OBSERVABLES[i] for unregistered
slots, so a table shorter than CF_OBSERVABLES => NULL deref. */
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
{"spare", "unused"},
};
7 changes: 6 additions & 1 deletion libpromises/dbm_migration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <backup.h>

extern const DBMigrationFunction dbm_migration_plan_lastseen[];
extern const DBMigrationFunction dbm_migration_plan_observations[];


#ifndef LMDB
Expand All @@ -51,7 +52,11 @@ static size_t DBVersion(DBHandle *db)
}

static const DBMigrationFunction *const dbm_migration_plans[dbid_max] = {
[dbid_lastseen] = dbm_migration_plan_lastseen
[dbid_lastseen] = dbm_migration_plan_lastseen,
/* Both DBs store the same fixed-size Averages records, so they share the
* plan that expands them when CF_OBSERVABLES grows. */
[dbid_observations] = dbm_migration_plan_observations,
[dbid_history] = dbm_migration_plan_observations
};

bool DBMigrate(DBHandle *db, dbid id)
Expand Down
Loading
Loading