doRqnp in FLiM.c overflows CAN frame

[kkimber]

doRqnp () copies in the first 7 module params into the 7 remaining bytes of the 8 byte CAN frame after the OPC_PARAMS opcode.

It then attempts to update the 9th byte of the CAN frame with the dynamic module flags:

// update the dynamic flags
cbusMsg[d1+PAR_FLAGS-1] = getParFlags();

Parameter flags cannot be returned in an OPC_PARAMS, as a standard CAN frame isn't big enough!

[spikyian]

I agree. The line which attempts to copy Parameter 8 should be removed.

[pnbgit]

Agreed.
I also checked dorqnpn()

if ((idx == PAR_FLAGS) && (flimState == fsFLiM)) 
            cbusMsg[d4] |= PF_FLiM;  

I think that should be calling getParFlags() otherwise it doesn't pick up the learn mode flag, or anything else added in the future to the flags that is dynamic.

Then looking at getParFlags(), I think there is a problem with the effective else clause which returns 0 if neither FLiM or FLiMLearn is set:

''''
if (flimState == fsFLiMLearn) {
return(PF_LRN | PF_FLiM) | (paramptr->params.module_flags);
}
if (flimState == fsFLiM) {
return(PF_FLiM)|(paramptr->params.module_flags);
}
return 0;
''''

As it will return 0 if it is neither in FLiM or learn mode.

Ok, so that means it is in SLiM, so generally wouldn't be interrogated anyway by RQNPN, but to be correct and just in case, it should be something like:

''''
if (flimState == fsFLiMLearn) {
return(PF_LRN | PF_FLiM) | (paramptr->params.module_flags);
}
if (flimState == fsFLiM) {
return(PF_FLiM)|(paramptr->params.module_flags);
}
return paramptr->params.module_flags;
''''

Possibly could code that more efficiently, although compiler will probably optimise it a bit.

[spikyian]

Would you be able to submit a pull request with the fixes?

[pnbgit]

I've put those changes into my copy of the code, and I can test it out here. I'm currently working in a branch towards CBUSLIB version 2q, which supports CANMIO version 2e. I will publish those branches with my latest commits once I have checked these changes havn't broken anything. These versions have conditional compilation to support my CANEMIO (or whatever I end up calling it) as well as building for standard CANMIO. My feeling is that this particular fix will be fine in the next beta release we do, which will probably be quite soon with the CANEMIO support. I don't feel that we need to rush out a release incorporating this fix in the next few minutes, as for most users the current release builds are working just fine. Are you ok with that?

[spikyian]

I checked what was located at the memory address after cbusMsg and found for the latest build on the 26K80 that it was bounceState so I don't think that there is a need to rush a fix out. Ian

…

On Thu, 18 Jan 2024, 18:57 Pete Brownlow, ***@***.***> wrote: I've put those changes into my code, and I can test it out here. I'm currently working in a branch towards CBUSLIB version 2q, which supports CANMIO version 2e. I will publish those branches with my latest commits once I have checked these changes havn't broken anything. These versions have conditional compilation to support my CANEMIO (or whatever I end up calling it) as well as building for standard CANMIO. My feeling is that this particular fix will be fine in the next beta release we do, which will probably be quite soon with the CANEMIO support. I don't feel that we need to rush out a release incorporating this fix in the next few minutes, as for most users the current release builds are working just fine. Are you ok with that? — Reply to this email directly, view it on GitHub <#14 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE6VG5VDFYSVKCRPHZ6ZWETYPFWDFAVCNFSM6AAAAABB7JXJ5OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJZGA2DAOJQGI> . You are receiving this because you commented.Message ID: ***@***.***>

[kkimber]

Sounds like you both have this covered. I'm not using or compiling the code for PIC, so this isn't a problem for me, I just spotted the issue while reviewing the code.