From 047bbb91f3ad1489cc6b5ca00108ff9a3520fd7d Mon Sep 17 00:00:00 2001 From: Josh Stillerman Date: Thu, 6 Nov 2025 12:56:47 -0500 Subject: [PATCH 1/4] Fix: strict-aliasing violations in treeshr Replace pointer-int casts with memcpy Fixes https://github.com/MDSplus/mdsplus/issues/2982 ** Note there are strict aliasing violations in other parts of the codebase too.** treeshrp.h - add int_to_nid function - add nid_to_int function - fix RFA macros to be aliasing safe TreeAddNode.c - remove unnessary cast TreeDeleteNode.c - use nid_to_int function TreeGetNci.c - use int_to_nid and nid_to_int functions - fix other strict-aliasing violations TreeGetRecord.c - use int_to_nid function TreeOpen.c - remove volatile qualifier - use nid_to_int function TreeRenameNode.c - use nid_to_int functions --- treeshr/TreeAddNode.c | 2 +- treeshr/TreeDeleteNode.c | 4 +-- treeshr/TreeGetNci.c | 14 ++++---- treeshr/TreeGetRecord.c | 2 +- treeshr/TreeOpen.c | 8 ++--- treeshr/TreeRenameNode.c | 4 +-- treeshr/treeshrp.h | 72 +++++++++++++++++++++++++++++++++++----- 7 files changed, 80 insertions(+), 26 deletions(-) diff --git a/treeshr/TreeAddNode.c b/treeshr/TreeAddNode.c index e1794c484e..68cf46346e 100644 --- a/treeshr/TreeAddNode.c +++ b/treeshr/TreeAddNode.c @@ -196,7 +196,7 @@ int _TreeAddNode(void *dbid, char const *name, int *nid_out, char usage) &scratch_nci, &ncilocked); if (STATUS_OK) { - if (_TreeIsOn(dblist, *(int *)&parent_nid) & 1) + if (_TreeIsOn(dblist, parent_nid) & 1) new_nci.flags &= (unsigned)~NciM_PARENT_STATE; else new_nci.flags |= NciM_PARENT_STATE; diff --git a/treeshr/TreeDeleteNode.c b/treeshr/TreeDeleteNode.c index 2266864153..03f3bb8c98 100644 --- a/treeshr/TreeDeleteNode.c +++ b/treeshr/TreeDeleteNode.c @@ -223,8 +223,8 @@ extern void _TreeDeleteNodeExecute(void *dbid) while (_TreeDeleteNodeGetNid(dbid, (int *)&nid) & 1) { int found = 0; - _TreeRemoveNodesTags(dbid, *(int *)&nid); - _TreeSetNoSubtree(dbid, *(int *)&nid); + _TreeRemoveNodesTags(dbid, nid_to_int(&nid)); + _TreeSetNoSubtree(dbid, nid_to_int(&nid)); node = nid_to_node(dblist, &nid); parent = parent_of(0, node); if (child_of(0, parent) == node) diff --git a/treeshr/TreeGetNci.c b/treeshr/TreeGetNci.c index 9bc890389e..31d5dcfe9d 100644 --- a/treeshr/TreeGetNci.c +++ b/treeshr/TreeGetNci.c @@ -91,7 +91,7 @@ extern void **TreeCtx(); static char *Treename(PINO_DATABASE *dblist, int nid_in) { TREE_INFO *info; - NID nid = *(NID *)&nid_in; + NID nid = int_to_nid(nid_in); unsigned int treenum; for (info = dblist->tree_info, treenum = 0; info && treenum < nid.tree; info = info->next_info) @@ -220,7 +220,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) void *dbid = *TreeCtx(); INIT_STATUS_AS TreeSUCCESS; PINO_DATABASE *dblist = (PINO_DATABASE *)dbid; - NID nid = *(NID *)&nid_in; + NID nid = int_to_nid(nid_in); int node_number; TREE_INFO *info; NCI_ITM *itm; @@ -449,7 +449,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) for (node = child_of(dblist, node); node; count++, node = brother_of(dblist, node) ? brother_of(dblist, node) : 0) ; - *(int *)(itm->pointer) = count; + memcpy(itm->pointer, &count, sizeof count); break; case NciNUMBER_OF_MEMBERS: break_on_no_node; @@ -459,7 +459,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) for (node = member_of(node); node; count++, node = brother_of(dblist, node) ? brother_of(dblist, node) : 0) ; - *(int *)(itm->pointer) = count; + memcpy(itm->pointer, &count, sizeof count); break; case NciNUMBER_OF_ELTS: break_on_no_node; @@ -468,7 +468,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) for (count = 0; swapint16(&cng_node->conglomerate_elt) > count; count++, cng_node++) ; - *(int *)(itm->pointer) = count; + memcpy(itm->pointer, &count, sizeof count); break; case NciCHILDREN_NIDS: { @@ -665,7 +665,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) *(NID *)itm->pointer = out_nid; } else - *(int *)itm->pointer = 0; + memcpy(itm->pointer, &count, sizeof count); break; } case NciDTYPE_STR: @@ -766,7 +766,7 @@ static char *getPath(PINO_DATABASE *dblist, NODE *node, int remove_tree_refs) for (info = dblist->tree_info, i = 0; info && i < nid.tree; i++, info = info->next_info) ; - if ((tag = _TreeFindNodeTags((void *)dblist, *(int *)&nid, &ctx)) != NULL) + if ((tag = _TreeFindNodeTags((void *)dblist, nid_to_int(&nid), &ctx)) != NULL) { string[0] = '\\'; string[1] = '\0'; diff --git a/treeshr/TreeGetRecord.c b/treeshr/TreeGetRecord.c index a245eb28cd..4f442b92b2 100644 --- a/treeshr/TreeGetRecord.c +++ b/treeshr/TreeGetRecord.c @@ -257,7 +257,7 @@ typedef ARRAY(struct descriptor *) array_dsc; int TreeMakeNidsLocal(struct descriptor *dsc_ptr, int nid) { int status = 1; - unsigned char tree = ((NID *)&nid)->tree; + unsigned char tree = int_to_nid(nid).tree; if (dsc_ptr == NULL) status = 1; else diff --git a/treeshr/TreeOpen.c b/treeshr/TreeOpen.c index 07ae3bea5f..ef2d2c752d 100644 --- a/treeshr/TreeOpen.c +++ b/treeshr/TreeOpen.c @@ -1179,18 +1179,18 @@ static void SubtreeNodeConnect(PINO_DATABASE *dblist, NODE *parent, * make brother_nid volatile so that the optimizer does not * decide to optimize it out and replace it with a zero. */ - volatile NID brother_nid = {0,0}; + NID brother_nid = {0,0}; NODE *brother = brother_of(dblist, parent); parent->usage = TreeUSAGE_SUBTREE_REF; subtreetop->usage = TreeUSAGE_SUBTREE_TOP; node_to_nid(dblist, subtreetop, &child_nid); node_to_nid(dblist, parent_of(dblist, parent), &parent_nid); - parent->child = *(int *)&child_nid; + parent->child = nid_to_int(&child_nid); if (brother) { node_to_nid(dblist, brother_of(dblist, parent), &brother_nid); } - subtreetop->brother = *(int *)&brother_nid; - subtreetop->parent = *(int *)&parent_nid; + subtreetop->brother = nid_to_int(&brother_nid); + subtreetop->parent = nid_to_int(&parent_nid); memcpy(subtreetop->name, parent->name, sizeof(subtreetop->name)); return; } diff --git a/treeshr/TreeRenameNode.c b/treeshr/TreeRenameNode.c index 8064cf2094..d79d0d0b6c 100644 --- a/treeshr/TreeRenameNode.c +++ b/treeshr/TreeRenameNode.c @@ -256,8 +256,8 @@ static int FixParentState(PINO_DATABASE *dblist, NODE *parent_ptr, in the flag longword and the parent state argument to SET_PARENT_STATE are negative boolean logic. ****************************************************/ - parent_state = _TreeIsOn(dblist, *(int *)&parent_nid) & 1; - status = _TreeGetNci(dblist, *(int *)&child_nid, child_itm_list); + parent_state = _TreeIsOn(dblist, nid_to_int(&parent_nid)) & 1; + status = _TreeGetNci(dblist, nid_to_int(&child_nid), child_itm_list); if (STATUS_OK) { child_parent_state = ((child_flags & NciM_PARENT_STATE) == 0); diff --git a/treeshr/treeshrp.h b/treeshr/treeshrp.h index 44e278ac23..408297994a 100644 --- a/treeshr/treeshrp.h +++ b/treeshr/treeshrp.h @@ -397,14 +397,54 @@ typedef struct } RFA; #ifdef RFA_MACROS -#define RfaToSeek(rfa) \ - (((*(unsigned int *)rfa - 1) * 512) + \ - (*(unsigned short *)&((char *)rfa)[4] & 0x1ff)) -#define SeekToRfa(seek, rfa) \ - { \ - *(unsigned int *)rfa = (unsigned int)(seek / 512 + 1); \ - *(unsigned short *)&(((char *)rfa)[4]) = (unsigned short)(seek % 512); \ - } + +#include +/* + * RFA layout (byte-packed, little-endian): + * rfa[0..3] : uint32_t block_number_1based + * rfa[4..5] : uint16_t offset (low 9 bits used) + * + * seek = (block - 1) * 512 + (offset & 0x1FF) + */ + +static inline uint32_t rfa_to_seek(const RFA *r) +{ + /* load 32-bit block number (little-endian) */ + uint32_t blk = + ((uint32_t)r->rfa[0]) | + ((uint32_t)r->rfa[1] << 8) | + ((uint32_t)r->rfa[2] << 16) | + ((uint32_t)r->rfa[3] << 24); + + if (blk == 0) { + /* matches old behavior if uninitialized */ + return 0; + } + + /* load 16-bit offset (little-endian) */ + uint16_t off = + (uint16_t)r->rfa[4] | + (uint16_t)((uint16_t)r->rfa[5] << 8); + + return (blk - 1U) * 512U + (uint32_t)(off & 0x1FFU); +} + +static inline void seek_to_rfa(uint32_t seek, RFA *r) +{ + uint32_t blk = seek / 512U + 1U; + uint16_t off = (uint16_t)(seek % 512U); + + /* store block, little-endian */ + r->rfa[0] = (uint8_t)(blk & 0xFFU); + r->rfa[1] = (uint8_t)((blk >> 8) & 0xFFU); + r->rfa[2] = (uint8_t)((blk >> 16) & 0xFFU); + r->rfa[3] = (uint8_t)((blk >> 24) & 0xFFU); + + /* store offset, little-endian */ + r->rfa[4] = (uint8_t)(off & 0xFFU); + r->rfa[5] = (uint8_t)((off >> 8) & 0xFFU); +} + #endif /**************************************** @@ -642,6 +682,20 @@ typedef struct pino_database void *dispatch_table; /* pointer to dispatch table generated by dispatch/build */ } PINO_DATABASE; +static inline int nid_to_int(NID *nid) +{ + int result; + memcpy(&result, nid, sizeof(int)); + return result; +} + +static inline NID int_to_nid(int nid_int) +{ + NID nid; + memcpy(&nid, &nid_int, sizeof(int)); + return nid; +} + static inline NODE *nid_to_node(PINO_DATABASE *dbid, NID *nid) { TREE_INFO *info; @@ -793,7 +847,7 @@ static inline int node_to_nid(PINO_DATABASE *dbid, NODE *node, NID *nid_out) nid.tree = 0; if (nid_out) *nid_out = nid; - return *(int *)&nid; + return nid_to_int(&nid); } /****************************************** From 9b26d4ed78490f1919ac87ec35bddac75b68b741 Mon Sep 17 00:00:00 2001 From: Josh Stillerman Date: Thu, 6 Nov 2025 14:28:41 -0500 Subject: [PATCH 2/4] remove bogus comment about volatile remove bogus RFA macros in treeshrp.h --- treeshr/TreeOpen.c | 4 ---- treeshr/treeshrp.h | 51 ---------------------------------------------- 2 files changed, 55 deletions(-) diff --git a/treeshr/TreeOpen.c b/treeshr/TreeOpen.c index ef2d2c752d..e063935d6d 100644 --- a/treeshr/TreeOpen.c +++ b/treeshr/TreeOpen.c @@ -1175,10 +1175,6 @@ static void SubtreeNodeConnect(PINO_DATABASE *dblist, NODE *parent, NODE *subtreetop) { NID child_nid, parent_nid = {0, 0}; - /* - * make brother_nid volatile so that the optimizer does not - * decide to optimize it out and replace it with a zero. - */ NID brother_nid = {0,0}; NODE *brother = brother_of(dblist, parent); parent->usage = TreeUSAGE_SUBTREE_REF; diff --git a/treeshr/treeshrp.h b/treeshr/treeshrp.h index 408297994a..b02f8091de 100644 --- a/treeshr/treeshrp.h +++ b/treeshr/treeshrp.h @@ -396,57 +396,6 @@ typedef struct unsigned char rfa[6]; } RFA; -#ifdef RFA_MACROS - -#include -/* - * RFA layout (byte-packed, little-endian): - * rfa[0..3] : uint32_t block_number_1based - * rfa[4..5] : uint16_t offset (low 9 bits used) - * - * seek = (block - 1) * 512 + (offset & 0x1FF) - */ - -static inline uint32_t rfa_to_seek(const RFA *r) -{ - /* load 32-bit block number (little-endian) */ - uint32_t blk = - ((uint32_t)r->rfa[0]) | - ((uint32_t)r->rfa[1] << 8) | - ((uint32_t)r->rfa[2] << 16) | - ((uint32_t)r->rfa[3] << 24); - - if (blk == 0) { - /* matches old behavior if uninitialized */ - return 0; - } - - /* load 16-bit offset (little-endian) */ - uint16_t off = - (uint16_t)r->rfa[4] | - (uint16_t)((uint16_t)r->rfa[5] << 8); - - return (blk - 1U) * 512U + (uint32_t)(off & 0x1FFU); -} - -static inline void seek_to_rfa(uint32_t seek, RFA *r) -{ - uint32_t blk = seek / 512U + 1U; - uint16_t off = (uint16_t)(seek % 512U); - - /* store block, little-endian */ - r->rfa[0] = (uint8_t)(blk & 0xFFU); - r->rfa[1] = (uint8_t)((blk >> 8) & 0xFFU); - r->rfa[2] = (uint8_t)((blk >> 16) & 0xFFU); - r->rfa[3] = (uint8_t)((blk >> 24) & 0xFFU); - - /* store offset, little-endian */ - r->rfa[4] = (uint8_t)(off & 0xFFU); - r->rfa[5] = (uint8_t)((off >> 8) & 0xFFU); -} - -#endif - /**************************************** RECORD_HEADER VFC portion of file. From 6e4747c01a81f40c0ff08bd51b0630e13f0ab7e9 Mon Sep 17 00:00:00 2001 From: Josh Stillerman Date: Thu, 6 Nov 2025 14:34:36 -0500 Subject: [PATCH 3/4] Another co-pilot suggested fix --- treeshr/TreeGetNci.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/treeshr/TreeGetNci.c b/treeshr/TreeGetNci.c index 31d5dcfe9d..72862724a3 100644 --- a/treeshr/TreeGetNci.c +++ b/treeshr/TreeGetNci.c @@ -665,7 +665,10 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) *(NID *)itm->pointer = out_nid; } else - memcpy(itm->pointer, &count, sizeof count); + { + int zero = 0; + memcpy(itm->pointer, &zero, sizeof zero); + } break; } case NciDTYPE_STR: From fc7ce4331f495076464363c31d3d039fe9f3c394 Mon Sep 17 00:00:00 2001 From: Josh Stillerman Date: Thu, 6 Nov 2025 15:04:43 -0500 Subject: [PATCH 4/4] use sizeof(var) instead of sizeof var --- treeshr/TreeGetNci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/treeshr/TreeGetNci.c b/treeshr/TreeGetNci.c index 72862724a3..0297c51955 100644 --- a/treeshr/TreeGetNci.c +++ b/treeshr/TreeGetNci.c @@ -449,7 +449,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) for (node = child_of(dblist, node); node; count++, node = brother_of(dblist, node) ? brother_of(dblist, node) : 0) ; - memcpy(itm->pointer, &count, sizeof count); + memcpy(itm->pointer, &count, sizeof(count)); break; case NciNUMBER_OF_MEMBERS: break_on_no_node; @@ -459,7 +459,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) for (node = member_of(node); node; count++, node = brother_of(dblist, node) ? brother_of(dblist, node) : 0) ; - memcpy(itm->pointer, &count, sizeof count); + memcpy(itm->pointer, &count, sizeof(count)); break; case NciNUMBER_OF_ELTS: break_on_no_node; @@ -468,7 +468,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) for (count = 0; swapint16(&cng_node->conglomerate_elt) > count; count++, cng_node++) ; - memcpy(itm->pointer, &count, sizeof count); + memcpy(itm->pointer, &count, sizeof(count)); break; case NciCHILDREN_NIDS: { @@ -667,7 +667,7 @@ int TreeGetNci(int nid_in, struct nci_itm *nci_itm) else { int zero = 0; - memcpy(itm->pointer, &zero, sizeof zero); + memcpy(itm->pointer, &zero, sizeof(zero)); } break; }