Skip to content
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.

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

Bug in LIST_OPEN_HANDLES reply #2

Closed
JakubVanek opened this issue Jul 1, 2016 · 3 comments
Closed

Bug in LIST_OPEN_HANDLES reply #2

JakubVanek opened this issue Jul 1, 2016 · 3 comments

Comments

@JakubVanek
Copy link

Hi,
I'm implementing the EV3 communication protocol in my program and I think there's a bug in c_com.c on line 2263:

for (HCnt1 = 0; HCnt1 < ((MAX_FILE_HANDLES/8) + 1); HCnt1++)
{

  pReplyListHandles->PayLoad[HCnt1 + 2] = 0;

  for(HCnt2 = 0; HCnt2 < 8; HCnt2++)
  {

    if (0 != ComInstance.Files[HCnt2 * HCnt1].State)
    { // Filehandle is in use

      pReplyListHandles->PayLoad[HCnt1 + 2] |= (0x01 << HCnt2) ;
    }
  }
}

That can be simplified to this pseudocode:

FOR i: 0...8
  ARR[i+2] = 0
  FOR j: 0...7
    IF IS_OPEN(i*j)
      ARR[i+2] |= 1 << j
    END IF
  END FOR
END FOR

I think the i*j (HCnt2 * HCnt1) handle index is a nonsense. Simple bash script illustrates that:

$ for((i=0;$i<=8;i++)) do for((j=0;$j<8;j++)) do echo "i = $i; j = $j; i*j = $(expr $i \* $j)"; done; done
i = 0; j = 0; i*j = 0
i = 0; j = 1; i*j = 0
i = 0; j = 2; i*j = 0
i = 0; j = 3; i*j = 0
i = 0; j = 4; i*j = 0
i = 0; j = 5; i*j = 0
i = 0; j = 6; i*j = 0
i = 0; j = 7; i*j = 0
i = 1; j = 0; i*j = 0
i = 1; j = 1; i*j = 1
i = 1; j = 2; i*j = 2
i = 1; j = 3; i*j = 3
i = 1; j = 4; i*j = 4
i = 1; j = 5; i*j = 5
i = 1; j = 6; i*j = 6
i = 1; j = 7; i*j = 7
i = 2; j = 0; i*j = 0
i = 2; j = 1; i*j = 2
i = 2; j = 2; i*j = 4
i = 2; j = 3; i*j = 6
i = 2; j = 4; i*j = 8
i = 2; j = 5; i*j = 10
i = 2; j = 6; i*j = 12
i = 2; j = 7; i*j = 14
i = 3; j = 0; i*j = 0
i = 3; j = 1; i*j = 3
i = 3; j = 2; i*j = 6
i = 3; j = 3; i*j = 9
i = 3; j = 4; i*j = 12
i = 3; j = 5; i*j = 15
i = 3; j = 6; i*j = 18
i = 3; j = 7; i*j = 21
i = 4; j = 0; i*j = 0
i = 4; j = 1; i*j = 4
i = 4; j = 2; i*j = 8
i = 4; j = 3; i*j = 12
i = 4; j = 4; i*j = 16
i = 4; j = 5; i*j = 20
i = 4; j = 6; i*j = 24
i = 4; j = 7; i*j = 28
i = 5; j = 0; i*j = 0
i = 5; j = 1; i*j = 5
i = 5; j = 2; i*j = 10
i = 5; j = 3; i*j = 15
i = 5; j = 4; i*j = 20
i = 5; j = 5; i*j = 25
i = 5; j = 6; i*j = 30
i = 5; j = 7; i*j = 35
i = 6; j = 0; i*j = 0
i = 6; j = 1; i*j = 6
i = 6; j = 2; i*j = 12
i = 6; j = 3; i*j = 18
i = 6; j = 4; i*j = 24
i = 6; j = 5; i*j = 30
i = 6; j = 6; i*j = 36
i = 6; j = 7; i*j = 42
i = 7; j = 0; i*j = 0
i = 7; j = 1; i*j = 7
i = 7; j = 2; i*j = 14
i = 7; j = 3; i*j = 21
i = 7; j = 4; i*j = 28
i = 7; j = 5; i*j = 35
i = 7; j = 6; i*j = 42
i = 7; j = 7; i*j = 49
i = 8; j = 0; i*j = 0
i = 8; j = 1; i*j = 8
i = 8; j = 2; i*j = 16
i = 8; j = 3; i*j = 24
i = 8; j = 4; i*j = 32
i = 8; j = 5; i*j = 40
i = 8; j = 6; i*j = 48
i = 8; j = 7; i*j = 56

Then there's a write outside the packet CmdSize caused by index i+2 (HCnt1 + 2). Claimed size is sizeof(RPLY_LIST_HANDLES) - sizeof(CMDSIZE) + 9, but real size is sizeof(RPLY_LIST_HANDLES) - sizeof(CMDSIZE) + 11.
I think the whole switch case should be:

    case LIST_OPEN_HANDLES:
    {
      UBYTE NByte, NBit;

      LIST_HANDLES       *pListHandles;
      RPLY_LIST_HANDLES  *pReplyListHandles;

      //Setup pointers
      pListHandles        =  (LIST_HANDLES*)pRxBuf->Buf;
      pReplyListHandles   =  (RPLY_LIST_HANDLES*)pTxBuf->Buf;

      pReplyListHandles->CmdSize  =  SIZEOF_RPLYLISTHANDLES - sizeof(CMDSIZE);
      pReplyListHandles->MsgCount =  pListHandles->MsgCount;
      pReplyListHandles->CmdType  =  SYSTEM_REPLY;
      pReplyListHandles->Cmd      =  LIST_OPEN_HANDLES;
      pReplyListHandles->Status   =  SUCCESS;

      for (NByte = 0; NByte < (MAX_FILE_HANDLES / 8 + 1); NByte++)
      {
        pReplyListHandles->PayLoad[NByte] = 0;

        for(NBit = 0; NBit < 8 && (NByte * 8 + NBit) < MAX_OPEN_HANDLES; NBit++)
        {
          if (0 != ComInstance.Files[NByte * 8 + NBit].State)
          { // Filehandle is in use
            pReplyListHandles->PayLoad[Byte] |= (0x01 << Bit);
          }
        }
      }
      pReplyListHandles->CmdSize  +=  Byte;
      pTxBuf->BlockLen             =  SIZEOF_RPLYLISTHANDLES + Byte;
    }
    break;

Have a nice day!

@dlech
Copy link
Member

dlech commented Jul 2, 2016

I'm just curious... What are you using this command for in your program? I have a feeling that the official LEGO software does not use this command for anything.

@JakubVanek
Copy link
Author

It's just for fun. I'm writing my program because I want to be able to control the brick with a command line utility. After I finish it, I'm probably going to publish it on GitHub.

By the way, I like your firmware flasher. Is there any documentation for the boot EEPROM apart from your code and the boot code itself?

@dlech
Copy link
Member

dlech commented Jul 2, 2016

Is there any documentation for the boot EEPROM apart from your code and the boot code itself?

No. LEGO has not released the source code used in the EEPROM and it is not mentioned in any of the SDKs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants