Skip to content

ncnn2mem support new style array and string type #6440

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
nihui wants to merge 6 commits into
base: master
Choose a base branch
from
+154 −9
Open

ncnn2mem support new style array and string type #6440

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77167c3
ncnn2mem support new style array and string type
nihui Dec 5, 2025
8aae03a
Update tools/ncnn2mem.cpp
nihui Dec 5, 2025
53e2d79
Update tools/ncnn2mem.cpp
nihui Dec 5, 2025
c4d4591
Update tools/ncnn2mem.cpp
nihui Dec 5, 2025
b64ff83
Update tools/ncnn2mem.cpp
nihui Dec 5, 2025
d5777d9
Update tools/ncnn2mem.cpp
nihui Dec 5, 2025
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
163 changes: 154 additions & 9 deletions tools/ncnn2mem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ static bool vstr_is_float(const char vstr[16])
return false;
}

static bool vstr_is_string(const char vstr[16])
{
return isalpha(vstr[0]) || vstr[0] == '\"';
}

static float vstr_to_float(const char vstr[16])
{
double v = 0.0;
Expand Down Expand Up @@ -281,12 +286,13 @@ static int dump_param(const char* parampath, const char* parambinpath, const cha
int id = 0;
while (fscanf(fp, "%d=", &id) == 1)
{
fwrite(&id, sizeof(int), 1, mp);

bool is_array = id <= -23300;

if (is_array)
{
fwrite(&id, sizeof(int), 1, mp);

// old style array
int len = 0;
nscan = fscanf(fp, "%d", &len);
if (nscan != 1)
Expand Down Expand Up @@ -320,18 +326,157 @@ static int dump_param(const char* parampath, const char* parambinpath, const cha
fwrite(&v, sizeof(int), 1, mp);
}
}

continue;
}
else

char vstr[16];
char comma[4];
nscan = fscanf(fp, "%15[^,\n ]", vstr);
if (nscan != 1)
{
char vstr[16];
nscan = fscanf(fp, "%15s", vstr);
if (nscan != 1)
fprintf(stderr, "read value failed %d\n", nscan);
return -1;
}

bool is_string = vstr_is_string(vstr);
if (is_string)
{
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The transformation id = -id - 23400 is a magic number calculation. Consider adding a comment explaining that 23400 is the encoding offset for string types, similar to how 23300 is used for arrays. This would improve code readability and maintainability.

Suggested change
{
{
// 23400 is the encoding offset for string types (similar to 23300 for arrays)

Copilot uses AI. Check for mistakes.
id = -id - 23400;
fwrite(&id, sizeof(int), 1, mp);

// scan the remaining string
char vstr2[256];
vstr2[241] = '\0'; // max 255 = 15 + 240
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The magic number 241 and the comment explaining "max 255 = 15 + 240" suggest a constraint, but using 241 as a sentinel is confusing. Consider using a named constant to clarify the intent:

const int MAX_VSTR2_LEN = 240;
char vstr2[MAX_VSTR2_LEN + 1];
vstr2[MAX_VSTR2_LEN] = '\0';

Then check vstr2[MAX_VSTR2_LEN] != '\0' at line 373. This makes the relationship between the sizes explicit.

Copilot uses AI. Check for mistakes.

if (vstr[0] == '\"')
{
fprintf(stderr, "read value failed %d\n", nscan);
return -1;
int len = 0;
while (vstr[len] != '\0')
len++;
char end = vstr[len - 1];
if (end != '\"')
{
nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}
else
nscan = 0; // already ended with a quote, no need to scan more
Comment on lines +357 to +363
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The len calculation can cause undefined behavior when vstr is empty or only contains a null terminator. If vstr[0] == '\"' and the string has length 1 (only a quote), then len would be 1 and accessing vstr[len - 1] (i.e., vstr[0]) would incorrectly check the opening quote instead of detecting there's no closing quote.

Consider checking if len > 1 before accessing vstr[len - 1] to avoid edge cases:

int len = 0;
while (vstr[len] != '\0')
    len++;
if (len > 1)
{
    char end = vstr[len - 1];
    if (end != '\"')
    {
        nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
    }
    else
        nscan = 0;
}
else if (len == 1)
{
    // Only opening quote, need to read the rest
    nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}
Suggested change
char end = vstr[len - 1];
if (end != '\"')
{
nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}
else
nscan = 0; // already ended with a quote, no need to scan more
if (len > 1)
{
char end = vstr[len - 1];
if (end != '\"')
{
nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}
else
nscan = 0; // already ended with a quote, no need to scan more
}
else if (len == 1)
{
// Only opening quote, need to read the rest
nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}
else
{
// Empty string, nothing to scan
nscan = 0;
}

Copilot uses AI. Check for mistakes.
}
else
{
nscan = fscanf(fp, "%255[^\n ]", vstr2);
Comment on lines +360 to +367
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The format string "%255[^\n ]" can read up to 255 characters into vstr2 (size 256), which is correct. However, the subsequent validation at line 373 checks vstr2[241] instead of checking if the buffer was fully utilized. This inconsistency means strings between 241 and 255 characters would pass validation even though they exceed the intended limit (15 from vstr + 240 from vstr2 = 255 max).

Either adjust the format to "%240[^\n ]" to enforce the 255 total limit, or fix the validation to check the appropriate position.

Suggested change
nscan = fscanf(fp, "%255[^\"\n]\"", vstr2);
}
else
nscan = 0; // already ended with a quote, no need to scan more
}
else
{
nscan = fscanf(fp, "%255[^\n ]", vstr2);
nscan = fscanf(fp, "%240[^\"\n]\"", vstr2);
}
else
nscan = 0; // already ended with a quote, no need to scan more
}
else
{
nscan = fscanf(fp, "%240[^\n ]", vstr2);

Copilot uses AI. Check for mistakes.
}

std::string str;
if (nscan == 1)
{
if (vstr2[241] != '\0')
{
fprintf(stderr, "string too long (id=%d)\n", id);
return -1;
}

if (vstr[0] == '\"')
str = std::string(&vstr[1]) + vstr2;
else
str = std::string(vstr) + vstr2;
}
else
{
if (vstr[0] == '\"')
str = std::string(&vstr[1]);
else
str = std::string(vstr);
}

if (str[str.size() - 1] == '\"')
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Accessing str[str.size() - 1] without first checking if str is empty can cause undefined behavior. If the string is empty (which could happen if vstr[0] == '\"' and vstr[1] == '\0'), then str.size() would be 0, and str.size() - 1 would wrap around to a very large number due to unsigned arithmetic.

Add a check before accessing the last character:

if (!str.empty() && str[str.size() - 1] == '\"')
    str.resize(str.size() - 1);
Suggested change
if (str[str.size() - 1] == '\"')
if (!str.empty() && str[str.size() - 1] == '\"')

Copilot uses AI. Check for mistakes.
str.resize(str.size() - 1);

int len = (int)str.length();
fwrite(&len, sizeof(int), 1, mp);
fwrite(str.data(), sizeof(char), len, mp);

continue;
}

bool is_float = vstr_is_float(vstr);

nscan = fscanf(fp, "%1[,]", comma);
is_array = nscan == 1;

if (is_array)
{
id = -id - 23300;
fwrite(&id, sizeof(int), 1, mp);

std::vector<float> af;
std::vector<int> ai;

if (is_float)
{
af.push_back(vstr_to_float(vstr));
}
else
{
int v = 0;
nscan = sscanf(vstr, "%d", &v);
if (nscan != 1)
{
fprintf(stderr, "parse value failed %d\n", nscan);
return -1;
}

ai.push_back(v);
}

while (1)
{
nscan = fscanf(fp, "%15[^,\n ]", vstr);
if (nscan != 1)
{
break;
}

if (is_float)
{
af.push_back(vstr_to_float(vstr));
}
else
{
int v = 0;
nscan = sscanf(vstr, "%d", &v);
if (nscan != 1)
{
fprintf(stderr, "parse value failed %d\n", nscan);
return -1;
}

ai.push_back(v);
}

nscan = fscanf(fp, "%1[,]", comma);
if (nscan != 1)
{
break;
}
}

bool is_float = vstr_is_float(vstr);
if (is_float)
{
int len = (int)af.size();
fwrite(&len, sizeof(int), 1, mp);
fwrite(af.data(), sizeof(float), len, mp);
}
else
{
int len = (int)ai.size();
fwrite(&len, sizeof(int), 1, mp);
fwrite(ai.data(), sizeof(int), len, mp);
}
}
else
{
fwrite(&id, sizeof(int), 1, mp);

if (is_float)
{
Expand Down
Loading