ricmoo / QRCode

QR code generation library in C, optimized for low-power devices, such as Arduino.
Other
647 stars 204 forks source link

memory out of range question #7

Closed fawdlstty closed 5 years ago

fawdlstty commented 5 years ago

Hi friend, in line:

https://github.com/ricmoo/QRCode/blob/master/src/qrcode.c#L709

after this code, some code access the variable but out of range: [0, data->capacityBytes) .if i fix the error, the code likes uint8_t result = new uint8_t [data->capacityBytes 100];

after fix, i have generate image, but it can't scan from any qrcode app.such as:

text

ide: visual studio 2017

ricmoo commented 5 years ago

Can you include your code you use to generate the at code? You shouldn’t need to touch any parts of the data structure directly...

fawdlstty commented 5 years ago

I just include qrcode.c and rename qrcode.cpp and modify that code uint8_t result [data->capacityBytes];

because Visual C++ does not support the grammar.

ricmoo commented 5 years ago

The syntax should certainly be supported, it is just creating a variable on the stack.

If you add a new to that line, you are allocating space on the heap, which will leak memory and could certainly cause corruption depending on how you use it, where free Is called and such.

You can leave it as a .c file, and wrap the header in extern C guards, which is a much safer way to modify the code, since all you are doing is changing the name mangling for the functions.

fawdlstty commented 5 years ago

20190411091356 this is source, file is .c, visual studio is not support

fawdlstty commented 5 years ago

only gcc and clang support

fawdlstty commented 5 years ago

20190411092139 compile output

ooxi commented 5 years ago

Variable length arrays are supported in C99, but not in C++. You can replace the expression with a call to alloca or std::vector.

While alloca might not be available on your platform, std::vector most likely will. It will however use the heap instead of the stack.

fawdlstty commented 5 years ago

While alloca might not be available on your platform

Using alloca may have an impact on the implementation because it changes the stack layout

std::vector most likely will. It will however use the heap instead of the stack.

I implemented it as a heap, and the compilation was fine, but the final output of the qr code was invalid

ricmoo commented 5 years ago

Right, I don’t really know how to solve this at this point. You are correct, that the Microsoft compiler cannot handle dynamic stack allocation. I’m curious is a .a file would suffice.

Using alloc is completely outside the purpose of this library though, which is intended to use stack only. The performance and more importantly, the memory safety of using alloc on embedded systems is greatly impacted, where any fragmentation can cause the system to non-deterministically cease to operate. The purpose of this library is to allow a heap-free implementation. There are plenty of existing heap-based implementations though. For example: https://github.com/nayuki/QR-Code-generator

This library is also meant to be C, while std::vector requires a C++ runtime, which adds 500-1400 bytes to the footprint, which would put this library outside of the capacity of many platforms it targets.

Does that make sense?

nayuki commented 5 years ago

There are plenty of existing heap-based implementations though. For example: https://github.com/nayuki/QR-Code-generator

Just for the record: My C++ library is heap-based because it uses std::vector. But my C library does not allocate heap memory; instead it requires the user to pass in appropriately sized buffers (which can be allocated on the stack or heap).

You wrote your C library inspired by my C++ library, but then I wrote my own C implementation some months later. This could explain why you might be unaware of my new developments.

ricmoo commented 5 years ago

Ah yes, sounds exactly the same then. It makes memory management much more obvious to pass in (and often reuse) a clump of memory, especially in the C world.

Does your library work using the MS compiler? I’ve had a few people have issues with it, and I personally always use GCC or LLVM, but MS is on record saying they have no intentions of ever supporting dynamic stack allocation. :(

For anyone interested in this, certainly check out nayuki’s libraries, which absolutely inspired this library in the first place, and also in general are just well written and cover a diverse set of problems. :)

fawdlstty commented 5 years ago

Both libraries are great. Thank you for advice

nayuki commented 5 years ago

Does your library work using the MS compiler?

Unsure, because I can't be bothered to reinstall the 4-gigabyte Visual Studio development environment again. People have reported C++ problems in the past (available in my issues / pull requests), but I think I resolved them by eschewing const fields because they mess up implicit constructor generation. As for C, I use C99 features, and I believe the first Microsoft compiler that fully supports C99 is MSVC++ 2013.

MS is on record saying they have no intentions of ever supporting dynamic stack allocation

I was unaware they stated this position. That said, I heard that stack allocation is not in the C standard, so it is a vendor extension no matter what. I'm not sure if stack allocation is the same as alloca().

Note that in my library, you declare and use buffers like this:

// 173 bytes, if we expect no greater than version 5.
uint8_t qrcode[qrcodegen_BUFFER_LEN_FOR_VERSION(5)];

// 3918 bytes, worst case that covers everything.
uint8_t tempBuffer[qrcodegen_BUFFER_LEN_MAX];

const char *text = "Hello, world!";

bool ok = qrcodegen_encodeText(
    text,
    tempBuffer,
    qrcode,
    qrcodegen_Ecc_LOW,
    qrcodegen_VERSION_MIN,
    5,  // maxVersion
    qrcodegen_Mask_AUTO,
    true);

if (ok)
    printQr(qrcode);