WedgeCoop / IS4C

Integrated Systems for Co-ops
http://is4c.coop
32 stars 17 forks source link

Possible buffer overrun in ssd.c? #11

Open tmmagee opened 12 years ago

tmmagee commented 12 years ago

In ssd.c the scannerInput array is defined as follows:

 char scannerInput[12];

But later in the code when the scanner input is read:

  for (i = 0; i < 17; i++) {
       scannerInput[i] = serialBuffer[i+4];
   }

Is this buffer overrun intentional? I have seen this code in multiple is4c forks on github and it has not been corrected. I changed the size of scannerInput to 17 in my own code to correct the issue, and the daemon still appears to run fine. Is there something I am missing?

maxolasersquad commented 12 years ago

That's a good find. I've ran through this code numerous times and never noticed it. I wonder if anyone else has any thoughts on this.

Bottlecap commented 12 years ago

You are right. That doesn't seem correct.

I think the intention is to get the barcode, without the 4 character prefix, from the serialBuffer, and put it into scannerInput and therefore it should be

for (i = 0; i < 12; i++) {

      scannerInput[i] = serialBuffer[i+4];

}

I suppose it doesn't hurt to have scannerInput bigger. but scannerInput[12] is meant to be the eof character

Tak

On 11-11-20 10:42 AM, tmmagee wrote:

In ssd.c the scannerInput array is defined as follows:

  char scannerInput[12];

But later in the code when the scanner input is read:

   for (i = 0; i<  17; i++) {
        scannerInput[i] = serialBuffer[i+4];
    }

Is this buffer overrun intentional? I have seen this code in multiple is4c forks on github and it has not been corrected. I changed the size of scannerInput to 17 in my own code to correct the issue, and the daemon still appears to run fine. Is there something I am missing?


Reply to this email directly or view it on GitHub: https://github.com/WedgeCoop/IS4C/issues/11

I.T. Manager Wedge Community Co-op 2105 Lyndale Avenue S. Minneapolis, MN 55405 612 874 7275 www.wedge.coop

tmmagee commented 12 years ago

I tried that fix and it did not work for me. Items did not scan correctly. I did not debug the issue, but I can only assume that by making the change you suggest the scanner was no longer scanning in the entire barcode.

Changing the size of the scannerInput to 17, however, did work just fine.

One note: the store I am working for (Mariposa Food Co-op) reads the check digit in UPC barcodes, so perhaps that was my problem for the fix you have just suggested.

Bottlecap commented 12 years ago

The fix I just suggested was actually incorrect

should have been

for (i = 0; i< 13; i++) { scannerInput[i] = serialBuffer[i+4]; }

and not < 12 because there are 13 characters altogether.

If you have a check digit then your scanned input would have 14 characters (12 digit barcode + check digit + eof) in which case scannedInput will have to be scannedInput[13]. I suppose anything over [13] would work because of the presence of the eof character

Tak

On 11-11-21 11:00 AM, tmmagee wrote:

I tried that fix and it did not work for me. Items did not scan correctly. I did not debug the issue, but I can only assume that by making the change you suggest the scanner was no longer scanning in the entire barcode.

Changing the size of the scannerInput to 17, however, did work just fine.

One note: the store I am working for (Mariposa Food Co-op) reads the check digit in UPC barcodes, so perhaps that was my problem for the fix you have just suggested.


Reply to this email directly or view it on GitHub: https://github.com/WedgeCoop/IS4C/issues/11#issuecomment-2817712

I.T. Manager Wedge Community Co-op 2105 Lyndale Avenue S. Minneapolis, MN 55405 612 874 7275 www.wedge.coop

tmmagee commented 12 years ago

Your fix worked for us, Bottlecap. Thanks for the help.