Skip to content

Issue #3395: Fix tool table formatting error with random tool changer #3428

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
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #3395: Fix tool table formatting error with random tool changer #3428

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions src/emc/tooldata/tooldata_common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,11 @@ void tooldata_format_toolline (int idx,
#undef F_ITEM
#undef I_ITEM
if (tdata.comment[0]) {
snprintf(tmp,sizeof(tmp)," ;%s\n",tdata.comment); \
strncat(formatted_line,tmp,CANON_TOOL_ENTRY_LEN-1); \
}
snprintf(tmp,sizeof(tmp)," ;%s\n",tdata.comment);
} else {
snprintf(tmp,sizeof(tmp),"\n");
}
strncat(formatted_line,tmp,CANON_TOOL_ENTRY_LEN-1);
Copy link
Contributor

@smoe smoe May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

strncat "appends at most ssize non-null bytes from the array pointed to by src". It does not check how much is already written to the string or how large the buffer truly is. So, the length of formatted_line in my understanding needs to be substracted from CANON_TOOL_ENTRY_LEN-1 to be safe.

This is all a bit cumbersome. I suggest to not execute the strncat in line 241 and instead keep the value "tmp" intact by not overwriting it in line 273. Instead, in line 273 perform the snprintf into formatted_line and check the number of characters written. Like this:

new line 273:

int charsThatWouldBeWritten snprintf(formatted_line,CANON_TOOL_ENTRY_LEN-1,"%s ;%s\n", tmp, tdata.comment);
if (charsThatWouldBeWritten<0) {
    fprintf(stderr,"Could not be printed. No good. Maybe this computer should not control any machine.\n");-1
if (! charsThatWouldBeWritten<CANON_TOOL_ENTRY_LEN) {
    fprintf(stderr,"Ouch! May still be ok if the bits up the comment were copied. Not checked.\n");
}

and analogous for the part just attaching the "\n".

This would somewhat guarantee that all non-comment bits are truly read - This may avoid some weird situation to have some tool's length shortened by a couple of decimals positions and nobody notices that. Admittedly, after a quick check on http://wiki.linuxcnc.org/cgi-bin/wiki.pl?ToolTable it seems unlikely to create such a long line.

The code does not check if strlen(tdata.comment) < CANON_TOOL_COMMENT_SIZE . This would allow the routine to create formatted lines that do not follow the constraints set by the format.

Copy link
Collaborator Author

@andypugh andypugh May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This seems wrong?

char tmp[CANON_TOOL_ENTRY_LEN-1] = {0};

Shouldn't tmp be the full size, then we should use sizeof-1 to leave space for the null?

As for the rest of it, I wonder if this works?

char tmp[CANON_TOOL_ENTRY_LEN-1] = {0};
int space = sizeof(tmp);
int len;
    len= snprintf(tmp,sizeof(tmp),"T%-3d P%-3d"
            ,tdata.toolno
            ,is_random_toolchanger ? idx : tdata.pocketno);
    strncat(formatted_line,tmp,space);
    space -= len;
// format zero float values as %.0f for brevity
#define F_ITEM(item,letter) if (!ignore_zero_values || tdata.item) { \
                                if (tdata.item) { \
                                    len -= snprintf(tmp,sizeof(tmp)," " letter "%+f", tdata.item); \
                                } else { \
                                    len -= snprintf(tmp,sizeof(tmp)," " letter "%.0f",tdata.item); \
                                } \
                                strncat(formatted_line,tmp,space; \
                                space -= len;
                            }
#define I_ITEM(item,letter) if (!ignore_zero_values || tdata.item) { \
                                len = snprintf(tmp,sizeof(tmp)," " letter "%d",tdata.item); \
                                strncat(formatted_line,tmp,space); \
                                space -= len;
                            }

    F_ITEM(diameter,       "D");
    F_ITEM(offset.tran.x,  "X");
    F_ITEM(offset.tran.y,  "Y");
    F_ITEM(offset.tran.z,  "Z");
    F_ITEM(offset.a,       "A");
    F_ITEM(offset.b,       "B");
    F_ITEM(offset.c,       "C");
    F_ITEM(offset.u,       "U");
    F_ITEM(offset.v,       "V");
    F_ITEM(offset.w,       "W");
    F_ITEM(frontangle,     "I");
    F_ITEM(backangle,      "J");
    I_ITEM(orientation,    "Q");
#undef F_ITEM
#undef I_ITEM
    // Stop tracking len and space here for current tool table format
    if (tdata.comment[0]) {
        snprintf(tmp,space," ;%s\n",tdata.comment);
    } else {
        snprintf(tmp,space,"\n");
    }
    strncat(formatted_line,tmp,space);

return;
} // tooldata_format_toolline()

Expand Down
Loading