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

Possible buffer overrun in ssd.c? #11

Open
tmmagee opened this issue Nov 20, 2011 · 5 comments
Open

Possible buffer overrun in ssd.c? #11

tmmagee opened this issue Nov 20, 2011 · 5 comments

Comments

@tmmagee
Copy link

tmmagee commented Nov 20, 2011

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
Copy link
Contributor

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
Copy link

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:
#11

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

@tmmagee
Copy link
Author

tmmagee commented Nov 21, 2011

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
Copy link

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:
#11 (comment)

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

@tmmagee
Copy link
Author

tmmagee commented Jan 16, 2012

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

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

3 participants