Fixed some possible bugs

[botbench]

Hi, I read through your buffer overflow patches, but I would prefer to see a fixed length (60) to be used for this and some code to ensure that the string is always 0 terminated. Did you test the code for the buttons? Are you sure that this is correct? It seems odd to me that this would've gone undetected for so long.

[aholler]

Am 17.09.2013 20:31, schrieb Xander Soldaat:

Hi, I read through your buffer overflow patches, but I would prefer to see a fixed length (60) to be used for this and some code to ensure that the string is always 0 terminated.

man snprintf, it always terminates strings with zero (in contrast to strncpy)

And not using sizeof but some numbers is exactly the failure the devs did. Having seen some of the rest of their code, I assume they didn't know C very well. It's a torture to read through the code of lms2012.

Did you test the code for the buttons? Are you sure that this is correct? It seems odd to me that this would've gone undetected for so long.

I don't know when that failure may occure, but my EV3 still works (sometimes, because of a ton of other bugs) with that patch. I've fixed it because of some compiler warnings, and the compiler is right. BACK_BUTTON is defined as 7 and ButtonState is only 6 long. So that code modfies ButtonTimer which surely wasn't their intention, otherwise they would use ButtonTimer and not ButtonState.

Regards,

Alexander Holler

Try this:

include

include

int main(void) { char foo[10]; int i;

     memset(foo, 1, sizeof(foo));
     snprintf(foo, sizeof(foo), "0123456790123456789");
     for(i = 0; i< sizeof(foo); ++i)
             printf("%d\n", foo[i]);
     return 0;

}

[botbench]

My mistake, I was under the impression that they were char *, in which case sizeof() is a little dangerous, hence the fixed buffer. It seem that both buffers in are already fixed length. The problem lies in the fact that the receiving buffer is only 60 and the source is 120 (vmFILENAMESIZE). As for the torture of reading the code, the use of system() to create folders, symlinks or mount devices makes my skin craw: snprintf(Buffer,250,"ln -s /media/card %s &> /dev/null",vmSDCARD_FOLDER); system(Buffer); sync(); I'll merge the pull request you made. Thanks for the explanations.

[aholler]

Am 18.09.2013 06:52, schrieb Xander Soldaat:

My mistake, I was under the impression that they were char *, in which case sizeof() is a little dangerous, hence the fixed buffer. It seem that both buffers in are already fixed length. The problem lies in the fact that the receiving buffer is only 60 and the source is 120 (vmFILENAMESIZE). As for the torture of reading the code, the use of system() to create folders, symlinks or mount devices makes my skin craw: snprintf(Buffer,250,"ln -s /media/card %s &> /dev/null",vmSDCARD_FOLDER); system(Buffer); sync();

It isn't only that, things like

memcpy(&(BtInstance.NonVol.DevList[Cnt].Name[0]), &(BtInstance.NonVol.DevList[Cnt + 1].Name[0]), sizeof(DEVICELIST));

or

(*IicStr).Type = Type;

and almost every snprintf() (400+) is used without sizeof() but with static numbers like in the sprintf line you've pasted above just show missing knowledge about C.

Regards,

Alexander Holler

[aholler]

Am 18.09.2013 13:06, schrieb Alexander Holler:

Am 18.09.2013 06:52, schrieb Xander Soldaat:

My mistake, I was under the impression that they were char *, in which case sizeof() is a little dangerous, hence the fixed buffer. It seem that both buffers in are already fixed length. The problem lies in the fact that the receiving buffer is only 60 and the source is 120 (vmFILENAMESIZE). As for the torture of reading the code, the use of system() to create folders, symlinks or mount devices makes my skin craw: snprintf(Buffer,250,"ln -s /media/card %s &> /dev/null",vmSDCARD_FOLDER); system(Buffer); sync();

It isn't only that, things like

memcpy(&(BtInstance.NonVol.DevList[Cnt].Name[0]), &(BtInstance.NonVol.DevList[Cnt + 1].Name[0]), sizeof(DEVICELIST));

or

(*IicStr).Type = Type;

and almost every snprintf() (400+) is used without sizeof() but with static numbers like in the sprintf line you've pasted above just show missing knowledge about C.

I wonder how we should deal with that.

Do you know if Lego is willing to review/modify the code itself?

Regards,

Alexander Holler

[botbench]

I have a call with them in the next week or so, I will ask them.