assimp 🚀 - Huge set of warnings in M3D importer

Ok, I will start to add a techdep-issue for that. Thanks for the analysis.

RichardTea notifications@github.com schrieb am Mo., 2. Dez. 2019, 12:58:

This new format is producing almost all the compiler warnings I now see.
There are over a hundred new compiler warnings, primarily from m3d.h.

The general code quality is rather low, with a lot of unnecessary
'optimisations' many of which are pessimisations on modern
hardware/compilers.

I am a little concerned about the memory allocation too.

Additionally, the M3DImporter and M3DExporter headers unnecessarily
pollute the global namespace (and extend compilation time) with the
entirety of the "m3d.h" header.
This causes issues in projects that compile Assimp statically and is
likely to cause future issues with other importers/exporters.
As this one is trivial fix, I shall be attaching a pull request
momentarily.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/assimp/assimp/issues/2803?email_source=notifications&email_token=AARXFVESIRZGDPLYZ75A2L3QWTZ5RA5CNFSM4JTUV7CKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4H5HVDEA,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AARXFVFYTXSTSETTFDPT72LQWTZ5RANCNFSM4JTUV7CA
.

kimkulling on 2 Dec 2019

Upon further investigation, this importer should be marked as experimental and disabled by default.

It is very unsafe and is absolutely certain to read past the end of the provided buffer when given malformed data.

This is because the C SDK load function m3d_load(char *, m3dread_t, m3dfree_t, m3d_t*) function is only given a raw start pointer and cannot even be _informed_ of the size of the buffer.
Thus it cannot possibly know when it reads past the end of the buffer and will definitely read past the end if given malformed data.
This is a clear security hazard.

@bztsrc There is a fair amount of work needed on m3d.h before it's ready for prime time.
The easy wins:

Add a buffer size parameter, then check that the calculated size is equal or smaller than the provided buffer and ensure it cannot read past the end.
Add "const" modifier to all data buffers passed into m3d.h functions that it is not intended to modify. This allows the compiler to further optimise and prevents a lot of mistakes in both usage and implementation.
Defined constants - don't use "-1U" and "-2U", instead define some (macro'd) constants for these magic values.
functions starting with underscore are reserved for the C/C++ implementation, it's best not to use them: Eg _m3d_safestr et al.

RichardTea on 3 Dec 2019

👍1

Add a buffer size parameter, then check that the calculated size is equal or smaller than the provided buffer and ensure it cannot read past the end.

Agreed.

Add "const" modifier to all data buffers passed into m3d.h functions that it is not intended to modify. This allows the compiler to further optimise and prevents a lot of mistakes in both usage and implementation.

The compiler can't actually use this since const only means "can't be modified via this pointer" but because of aliasing _someone else_ might change it. And also the constness can be cast away... It does eliminate some usage errors though.

functions starting with underscore are reserved for the C/C++ implementation, it's best not to use them: Eg _m3d_safestr et al.

Also agreed. I noted this in my initial review.

In general assimp is not secure and should not be used with untrusted data. Fuzzing it will crash within seconds. I made a bug report at one point with several hundred cases. We simply don't have enough people/time to fix it all...

turol on 3 Dec 2019

👍1

And M3DExporter leaks the entire model every time.
At least that one is an easy fix.

RichardTea on 3 Dec 2019

Add "const" modifier to all data buffers passed into m3d.h functions that it is not intended to modify. This allows the compiler to further optimise and prevents a lot of mistakes in both usage and implementation.

The compiler can't actually use this since const only means "can't be modified via this pointer" but because of aliasing _someone else_ might change it. And also the constness can be cast away... It does eliminate some usage errors though.

Depends on context. Casting away const is known to the compiler, and it can often prove a variable is not aliased by the function (not always, of course). If another thread (or interrupt) is fiddling then that's Undefined Behaviour and nasal demons apply.

The real win is having the compiler tell you about silly mistakes!

In general assimp is not secure and should not be used with untrusted data. Fuzzing it will crash within seconds. I made a bug report at one point with several hundred cases. We simply don't have enough people/time to fix it all...

True, but that's no reason to make it worse!

RichardTea on 3 Dec 2019

@bztsrc Another easy win:
m3d_load() et al should be passed a void* cookie for an implementation-specific object, that it gives to all callbacks, particularly m3dimporter_readfile().
In C this can be an instance ident, while in C++ it can be a pointer to a specific object.

Most C APIs provide something similar - for example, gstreamer and Win32 both do this.

RichardTea on 3 Dec 2019

Hi @RichardTea

There are over a hundred new compiler warnings, primarily from m3d.h.

For example what? I've checked all the outputs, there was no warnings at all. Please be specific, "hundred new" helps nothing to solve this issue.

I am a little concerned about the memory allocation too

Again, be specific! I've ran the code through valgrind about thousand times with different inputs, there were no memory leaks. Valgrind always reported "No leaks possible".

This causes issues in projects that compile Assimp statically

Like what? This issue has been already resolved.

This is because the C SDK load function m3d_load(char , m3dread_t, m3dfree_t, m3d_t) function is only given a raw start pointer and cannot even be informed of the size of the buffer.

You are completely wrong about this, check the code again. The m3d_load() DOES receive the buffer size and it DOES check the buffer bounds.

Defined constants - don't use "-1U" and "-2U", instead define some (macro'd) constants for these magic values.

Are these causing your alleged warning messages?

functions starting with underscore are reserved for the C/C++ implementation, it's best not to use them: Eg _m3d_safestr et al.

It's best if you don't care about the private functions. That's why they are called "private", and that's why they are not part of the public API. I would have set the hidden attribute for them if I could, but unfortunately there's no cross-platform solution for that.

m3d_load() et al should be passed a void* cookie for an implementation-specific object, that it gives to all callbacks, particularly m3dimporter_readfile().

True, but it is not m3d that uses this, but Assimp. Why pollute the M3D API with something that's not needed elsewhere? The correct and OOP approved solution to this would be to add a static IOSSystem getter that returns the current instance.

In short, thanks for the report, but please be more specific, copy'n'paste warning messages for example, because you have just said big words but nothing that could actually help locating or fixing your alleged issues.

Thanks!
bzt

bztsrc on 8 Dec 2019

Hi @bztsrc

For example what? I've checked all the outputs, there was no warnings at all. Please be specific, "hundred new" helps nothing to solve this issue.

Erm, I had hoped you would take a look at the AppVeyor output. Almost every warning is from M3D.
I assume you're only using one compiler, take a look at the output from MSVC or mingw compilers. For example:
https://ci.appveyor.com/project/kimkulling/assimp/builds/29385310/job/h7km8fb4qkexf2wx?fullLog=true

They are mostly casting issues. I presume your specific compiler doesn't care, this is why AppVeyor and Travis are great tools!

I am a little concerned about the memory allocation too

Again, be specific! I've ran the code through valgrind about thousand times with different inputs, there were no memory leaks. Valgrind always reported "No leaks possible".

It would definitely leak the entire model on some code paths, and the exporter would leak the whole thing every time so I assume you never looked at it.
Assimp uses exceptions for error handling, so there many 'hidden' returns.
Remember that valgrind only looks at the paths taken, it does not have any way of checking what happens on error paths, eg if fed malformed data.

I'm assuming you have only used valid data. Try passing it data that claims to be ten times bigger than it actually is.

This causes issues in projects that compile Assimp statically

Like what? This issue has been already resolved.

This is because the C SDK load function m3d_load(char _, m3dread_t, m3dfree_t, m3d_t_) function is only given a raw start pointer and cannot even be informed of the size of the buffer.

You are completely wrong about this, check the code again. The m3d_load() DOES receive the buffer size and it DOES check the buffer bounds.

Can you please explain where the size of the data buffer is passed?
In void M3DImporter::InternReadFile() the value of the variable fileSize is never passed to the m3d functions.

m3d = m3d_load(&data[0], m3dimporter_readfile, free, nullptr);
Declaration: m3d_t *m3d_load(unsigned char *data, m3dread_t readfilecb, m3dfree_t freecb, m3d_t *mtllib);

This function seems to be given a raw pointer, two function pointers, and a second raw pointer.

Defined constants - don't use "-1U" and "-2U", instead define some (macro'd) constants for these magic values.

Are these causing your alleged warning messages?

Yes, they are causing some of them. Some other do too. Even if they were not, magic numbers are BAD.

functions starting with underscore are reserved for the C/C++ implementation, it's best not to use them: Eg _m3d_safestr et al.

It's best if you don't care about the private functions. That's why they are called "private", and that's why they are not part of the public API. I would have set the hidden attribute for them if I could, but unfortunately there's no cross-platform solution for that.

It's best if you keep within the C or C++ standard.

'Hidden' functions are _supposed_ to be created by the use of translation units, however the single-file-with-magic-macro approach makes it impossible to put the implementation into its own translation unit.
By choosing the STB layout, the main thing you sacrifice is the ability to have private functions. Whether that is a worthwhile tradeoff is up to you.

C/C++ standards require that you do not use identifiers that are reserved for the implementation, unless you _are_ the C/C++ implementation.

M3D is not part of the toolchain, runtime or OS, and therefore must not use such identifiers.
Future versions of MSVC, clang et al may start using it. Yes, it is unlikely, no that doesn't mean you should just hope they do not.

PS: I do note that _m3d_safestr() is called from M3DExporter.cpp, which implies it isn't actually private anyway and so should probably just be public?

m3d_load() et al should be passed a void* cookie for an implementation-specific object, that it gives to all callbacks, particularly m3dimporter_readfile().

True, but it is not m3d that uses this, but Assimp. Why pollute the M3D API with something that's not needed elsewhere? The correct and OOP approved solution to this would be to add a static IOSSystem getter that returns the current instance.

No, that is invalid. This is not about OOP, it is about re-entrancy.
There isn't a single current instance! There are _multiple_ instances in the process.

How does the callback know _which_ path to read when there are 2 or more model loads 'in-flight'?

In 2019, 100% of systems are multicore, so all library code must be written assuming that it will be used in a multithreaded (multicore) situation.
Eg Vulkan was explicitly designed to allow multiple threads to be preparing model data for submission to the GPU.

M3D will misbehave and/or crash if two threads in a process run two Assimp::Importer instances, as the static callback function pointer will be null'ed and the static data path will be changed out from under it.

All the other Assimp loaders are reentrant (with the possible exception of C4D which is proprietary and thus hidden from view).

It is very easy to make a C library re-entrant - simply avoid all static variables and pass in a 'cookie'.
The C library doesn't need to do anything with the cookie, just hand it out to any callbacks.
The runtime will deal with thread-safe malloc/free and the host application deals with wider thread-safety by making sure each cookie is appropriately handled.
Yes, there are other ways for a host application to try to handle it, but they are not standard until C++11 and Assimp is currently trying to remain compatible with C++03.

In the pull request I put a very simple C++11 mutex wrapper around it, but that's a very poor 'solution' as it throws away performance and doesn't work on C++03.

If it's not feasible for your SDK to be re-entrant, then it requires an explicit health warning saying this so users can handle it accordingly.

RichardTea on 8 Dec 2019

BTW - In Assimp only _embedded_ texture images must be loaded. The open-file callback might not even be necessary at all.

The other Assimp importers just return the (relative) paths for external image assets, in line with the Unix philosophy of doing one thing.
It also means textures can be encoded in more ways, and there's fewer bugs as it's much easier to fix a single bad PNG importer than to fix five or more different ones.

That said, it's not wrong for it to load them.

RichardTea on 8 Dec 2019

Hi @RichardTea,

I assume you're only using one compiler, take a look at the output from MSVC or mingw compilers.

You're assuming it wrong, but MSVC is definitely not on my primary list. You are saying that all warnings are limited to this one compiler and to the exporter code only? Thanks for the link, now with that list I can work with.

It would definitely leak the entire model on some code paths

Care to elaborate on this? I haven't tested the exporter as throughfully as the importer, I admit. But I'll. (Just for the records, as described in the documentation with all the reasons fully explained, the Assimp exporter is limited to static meshes. You should use the m3dconv utility or the Blender plugin for exporting M3D files in the first place.)

I'm assuming you have only used valid data.

You're assuming it wrong.

Try passing it data that claims to be ten times bigger than it actually is.

Already done, that's not an issue for several reasons. Not only the boundaries are checked (separately for the file, for the uncompressed buffer, and for each chunk), but there's also a sentinel to avoid such cases. And before you ask, yes, tested with smaller ones too.

Can you please explain where the size of the data buffer is passed?

Can you please read the code and the spec more carefully? I'm sure you'll figure it out, you're smart. However having an extra check on fileSize matching the passed file's size wouldn't hurt, I'll add that.

Even if they were not, magic numbers are BAD.

You have clearly no experience with the embedded world or low-level programming. They are NOT BAD, they are extremely useful. Think about this: why are magic numbers used in ALL binary file formats? (Just pick any format: PE, ELF, JPEG, PNG, 3DS, etc. all are full of magic numbers. Have you ever wondered why if they were bad as you claim?)

And how would you define a sentinel without a magic value? I'm listening.

Regardless, please tell me, if not by -1, then how would you indicate that an index is not set WITHOUT using extra memory?

C/C++ standards require that you do not use identifiers that are reserved for the implementation, unless you are the C/C++ implementation.

True, but starting a private label with an underscore is not a reserved identifier (it is by definition not an identifier, just a prefix, anything can came after the underscore). Instead it is, and always has been a common practice in C to prefix private functions / variables / fieldnames by an underscore.

If you don't believe me, here are some random examples, all using this private-starts-with-underscore convention:

There are much much more examples, I've just picked a few widely used and well-known ones. All of them are multi-platform, and most of them are multi-compiler projects.

Future versions of MSVC, clang et al may start using it.

I really really doubt that. I seriously think that the chance of them using anything that even starts with "_m3d_" is absolutely zero, let alone defining one of the identifiers in it's entire length.

I do note that _m3d_safestr() is called from M3DExporter.cpp

You are right about that, I was lazy. M3DExporter should have it's own string sanitizer method probably.

M3D will misbehave and/or crash if two threads in a process run two Assimp::Importer instances

Wrong. M3D won't misbehave. Assimp will.

It is very easy to make a C library re-entrant - simply avoid all static variables and pass in a 'cookie'.

This is Assimp specific. M3D SDK on it's own is perfectly thread safe, it does not use any global static variables (except for read-only tables), and only operates on the passed context exclusively. This problem has to be handled on Assimp side, sorry. Mutex you introduced is a working solution. Another (mutex-free) solution would be for Assimp to use thread-specific variables in such cases, like libc does with errno.

doesn't work on C++03.

C++11 is the minimum standard level for the M3D SDK. But guess what, the mutex you introduced is on the Assimp side! Which expects even later C++ standard if I recall correctly?

If it's not feasible for your SDK to be re-entrant

The M3D SDK is perfectly fine with re-entrancy, it is Assimp that has issues with the badly designed IOSystem instance (there should be one IOSystem per system, and not one per model which is a huge waste of resources anyway and causes problems like this). Be specific, please. As I said before, this is an Assimp specific issue, no other libraries are affected, so this has to be solved on the Assimp side. You already provided a working solution, and I have offered another (proper, performance-friendly) solution too.

Cheers,
bzt

bztsrc on 10 Dec 2019

Oh, and by Assimp's IOSystem misbehaving I mean: the texture won't be loaded, only it's file path will be returned. Which is the expected behavior anyway according to you.

Please note that the M3D callback checks if it were given a nullptr IOSystem, if there's a crash, that must be happen inside IOSsystem => not M3D problem.

Cheers,
bzt

bztsrc on 10 Dec 2019

The ad-hominem attacks were very much unnecessary.
If you cannot be civil, then leave this place and do not return until you can accept criticism of your code.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-magic
Yes, this applies to C as well. In C it's usually a macro.

What does "-2U" mean, and when should that magic number be used as opposed to "-1U" or 0?

Buffer:
m3d.h reads straight past the end of the buffer if the buffer does not contain well-formed data.
It will _always_ read at least one byte past the end, and _may_ read much futher.

This is undefined behaviour.

The only reason it does not immediately crash with an out-of-bounds exception is because some malloc implementations have a guard band.

Relying on the guard band to save your code is an incredibly bad idea.

RichardTea on 10 Dec 2019

Can you please read the code and the spec more carefully? I'm sure you'll figure it out, you're smart.

This kind of language is uncalled for. Please be polite in your interactions with others.
As for the code, m3d_load currently doesn't take the buffer length and must be modified to do so.

For the IOSystem and mutex issue, using a mutex is not really a solution but rather a hack. All other assimp importers can function in "shared nothing" mode where two entirely separate importers with separate IOSystems can be used in multiple threads without any synchronization and therefore without any chance of deadlock or other misbehavior. By using a mutex we bring in all their problems. If the users IOSystem internally takes some lock then we've introduced the possibility of incorrect locking order and deadlocks. Worse, there are other architectures which use threads very differently, see for example Parallelizing the Naughty Dog Engine Using Fibers. There a task can migrate to other threads relatively freely. This can easily break if assimp internally uses locks. I've written my own prototype using a similar architecture to prove it works with current assimp.

Wrong. M3D won't misbehave. Assimp will.

And since this is the assimp project we must not misbehave no matter which loaders are in use.

For the leading underscores, they really should be avoided. The C/C++ rules for when exactly they're allowed and when they're reserved for the implementation are not trivial. It's safer to just never use a leading underscore. And several of the projects you mention (MUSL/glic/libgcc and maybe jemalloc) are in fact "part of the implementation" so the rules for them are different than application code.

turol on 10 Dec 2019

👍1

And for the crash, here's one example:
https://drive.google.com/open?id=1A87g5UK8FvKgL2U5McEAYstYut6yuQOg

This reads past the end of buffer since m3d_load doesn't know its size. It was generated with AFL, which found several crashes within seconds.

turol on 10 Dec 2019

👍1

Please calm down. We are all trying to fix issues together, not against each other. The beast is the compiler, not the guy telling you that other optinions must be taken into account as well.

kimkulling on 10 Dec 2019

And some words about compiler warnings and code quality:
It is important to fix all the compiler warnings we can see in the output of the CI-builds! Maybe I should add a harder rule for the windows platforms as well.

And even private functions can cause hard crashes, so code quality is important here as well.

I will add the experimental flag to this importer. Thanks a lot to all for the great work to be able to make this code better :-)!

kimkulling on 10 Dec 2019

Hi All,

@RichardTea: what attack are you referring to? I don't understand. I'm civil and polite, and I take well-form criticism about my code. What I can't do anything with is when someone saying nonsense things without even reading the code. For example, "magic is BAD", what am I supposed to do with this? What was your intention, what response did you expect from this? Or, why did you say, "reads past the end" without telling where?

What does "-2U" mean, and when should that magic number be used as opposed to "-1U" or 0?

This is a perfect example. First, should you have read the spec (or the source at least), then you'd know. Also if you'd have enough experience with low-level programming, you'd know what the binary value of full 1s is conventionally used for. M3D only uses old and known-to-work conventions. Second, you asking this now is completely irrelevant as no -1U neither -2U are in the code at all any more.

m3d.h reads straight past the end of the buffer if the buffer does not contain well-formed data.

Where exactly? Please provide an example of this. And at least reference the line that supposedly reads past the end. It is not enough to say things like this, you have to PoC it, otherwise if it's not reproduceable, no-one can fix it.

This kind of language is uncalled for.

I don't understand what you mean by this kind of language? I meant to be polite, I haven't used any bad words, I've even used the word "please". I apologize if you misunderstood.

currently doesn't take the buffer length

You are keep saying that, but that's simply not true. It does take the buffer length.

Let me read the code for you (absolutely politely, no offense intended):
line 2934: here the buffer end pointer "end" is set using the buffer's length (!)
line 2991: here the condition does not allow the "buff" pointer to go beyond "end", and a santinel chunk is also checked.

while(buff < end && !M3D_CHUNKMAGIC(buff, 'O','M','D','3')) {

line 3021: iterating on chunks, again, the condition does not allow the "chunk" pointer to go beyond "end" and santinel chunk also checked, just as before.

while(chunk < end && !M3D_CHUNKMAGIC(buff, 'O','M','D','3')) {

And several of the projects you mention (MUSL/glic/libgcc and maybe jemalloc) are in fact "part of the implementation"

No, not these ones I linked. I've carefully chosen examples that are not exposed to the C/C++ level. And what about qemu, SDL, Linux, git? Are those "part of the implementation" too? Please, respect this SDK as much as I respect Assimp. One is C, the other is C++, there'll be differences for sure you must understand that. Your way is not the only way.

@kimkulling: I'm perfectly calm, but I'm confused. I don't know how to fix something that's not broken. Or what to do with "magic is BAD"? Seriously what kind of modification is supposed to be done for this? You simply cannot remove all the magic values from a binary format, that just makes no sense at all.

It is important to fix all the compiler warnings we can see in the output of the CI-builds!

I agree! That's why I've already fixed them long before these comments. I'll check if there's more and I'll fix those too.

And even private functions can cause hard crashes, so code quality is important here as well.

Again, agreed! That's why I said, "Please note that the M3D callback checks if it were given a nullptr IOSystem", meaning I already did everything to avoid possible crash. If the crash occurs inside IOSystem, that surely can't be fixed from the M3D wrapper.

I will add the experimental flag to this importer.

May I ask why? @RichardTea was talking about the _exporter_ all along. What is needed to take the experimental flag away?

Cheers,
bzt

bztsrc on 10 Dec 2019

@bztsrc Great, thanks a lot for your patience! I guess we need more testing on that. Hopefully someone will find some more thime during the chrismas vacation!

kimkulling on 10 Dec 2019

There's only one warning left with MSVC: "'=' : conversion from 'M3D_FLOAT' to 'int', possible loss of data"

It did not help to cast explicitly. I'm open to suggestions how to tell MSVC to accept this conversion without a warning.

Cheers,
bzt

bztsrc on 10 Dec 2019

Have I ever mentiond that face-to-face conversations are much better than posts? It is much easier without seeing the mimic and gestic of a person to get something wrong :-).

kimkulling on 10 Dec 2019

Warnings from my VS2019-build:

1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2126,23): warning C4267: "=": Konvertierung von "size_t" nach "unsigned int", Datenverlust möglich (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2129,77): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2138,36): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2145,30): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2158,31): warning C4244: "=": Konvertierung von "unsigned int" in "uint16_t", möglicher Datenverlust (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2159,31): warning C4244: "=": Konvertierung von "unsigned int" in "uint16_t", möglicher Datenverlust (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2180,102): warning C4100: "fn": Unreferenzierter formaler Parameter (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2180,80): warning C4100: "freecb": Unreferenzierter formaler Parameter (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2180,49): warning C4100: "readfilecb": Unreferenzierter formaler Parameter (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2437,61): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2467,23): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2471,32): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2476,36): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2489,39): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2508,61): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2570,45): warning C4244: "=": Konvertierung von "unsigned int" in "uint8_t", möglicher Datenverlust (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2582,80): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2608,36): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2613,44): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2619,31): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2677,42): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2685,27): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2688,54): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2694,60): warning C4267: "=": Konvertierung von "size_t" nach "unsigned int", Datenverlust möglich (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2702,42): warning C4244: "=": Konvertierung von "unsigned int" in "uint16_t", möglicher Datenverlust (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2716,56): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2722,43): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2748,92): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2847,65): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2970,31): warning C4127: Bedingter Ausdruck ist konstant (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2971,101): message : Verwenden Sie stattdessen ggf. die Anweisung "if constexpr". (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3119,57): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3153,66): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3247,76): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3278,32): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3286,40): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3294,48): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3336,38): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3338,15): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3341,42): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3366,51): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3374,59): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3586,7): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)
1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(3600,7): warning C4146: Einem vorzeichenlosen Typ wurde ein unärer Minus-Operator zugewiesen. Das Ergebnis ist weiterhin vorzeichenlos. (Quelldatei wird kompiliert D:\projects\osre\3dparty\assimp\code\M3D\M3DImporter.cpp)

kimkulling on 10 Dec 2019

@kimkulling: you have been distracted by these trolls, you're checking an OLD source! Check the latest pull request, posted hours before these comments: https://github.com/assimp/assimp/pull/2825
And please provide English messages if possible, not all readers understand German.

1>D:\projects\osre\3dparty\assimp\code\M3D\m3d.h(2180,49): warning C4100: "readfilecb": Unreferenzierter formaler Parameter

Would you please be kind to explain this? The readfilecb argument is defined at line 2103. It is referenced at line 2128 inside the gettx function, and there's no ifdef to avoid that. Are you trying to compile a modified source? Or are you usig some VERY old source in which line 2180 does not belong the gettx function?

Please use only the latest, unmodified version of m3d.h, otherwise I can guarantee nothing!

@RichardTea, @turol: please answer my questions, so that we can take you seriously and we can address the issues at hand.

how should be an undefined index indicated without using extra memory if not by a magic?
what code changes do you expect for "magic is BAD"?
are qemu, SDL, Linux and git "part of the implementation" too?
at which line does M3D read beyond the buffer?
if reading external assets really do crash, then when, under which circumstances and at which line does the wrapper crash?

Thanks,
bzt

bztsrc on 11 Dec 2019

That last float to int warning is cleaned up too, now it compiles with MSVC without any warnings (no M3D related warnings that is, and there were no MinGW warnings in the first place):
https://ci.appveyor.com/project/kimkulling/assimp/builds/29458596/job/cc3xf1e1tjp965p3?fullLog=true
https://ci.appveyor.com/project/kimkulling/assimp/builds/29458596/job/y7pis81ubatn3vgg?fullLog=true

I've also added an extra additional check for the file size.

Cheers,
bzt

bztsrc on 11 Dec 2019

Sorry for the german warnings. I just switched my VS to english.

The warnings are from the current master, so this is the current situation on master.

And these people are core-members, which I appreciate. So please avoid blaming them!

Their comments are useful and correct, when you calm down.

kimkulling on 11 Dec 2019

you have been distracted by these trolls

Do not accuse people of trolling just because they disagree with you. The issue with your previous comments is this statement:

I'm sure you'll figure it out, you're smart.

This sounds very condescending and disrespectful. Criticize code, not people.

posted hours before these comments

Posted on your branch but not on master yet. It takes extra effort to test code when you have to pull from someone else's fork. I did test your latest code just now.

how should be an undefined index indicated without using extra memory if not by a magic?

It's ok to use some specific number to indicate unused index but it should not be just a number. Replacing it with M3D_NOTDEFINED as you did is good because it signals intent and is easy to change in one place if necessary.

what code changes do you expect for "magic is BAD"?

When there's some number without any explanation it should be replaced with a symbolic name.

are qemu, SDL, Linux and git "part of the implementation" too?

No, but just because some other project does something doesn't mean we should too. Yes, in some cases it's ok to start an identifier with an underscore but not in all cases. The rules about it are not entirely trivial and so my preference is to avoid leading underscores entirely.

at which line does M3D read beyond the buffer?

Looks like you fixed my initial test case so here's another:
https://drive.google.com/file/d/1Hio8z42YPzK4hcjpSOIB9Kaso3cfB5vm/view?usp=sharing

When trying to load it with assimp info it crashes. Valgrind gives me this:

Invalid read of size 4
   at 0x6A820E: _m3d_getidx (m3d.h:2220)
   by 0x6ADE4E: m3d_load (m3d.h:3315)
   by 0x6B63EF: Assimp::M3DWrapper::M3DWrapper(Assimp::IOSystem*, std::vector<unsigned char, std::allocator<unsigned char> > const&) (M3DWrapper.cpp:110)
   by 0x6B0AB0: Assimp::M3DImporter::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, aiScene*, Assimp::IOSystem*) (M3DImporter.cpp:181)
   by 0x53F8EC: Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Assimp::IOSystem*) (BaseImporter.cpp:130)
   by 0x3BA3F0: Assimp::Importer::ReadFile(char const*, unsigned int) (Importer.cpp:656)
   by 0x3AE935: Assimp::Importer::ReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (Importer.hpp:648)
   by 0x3ADAD7: ImportModel(ImportData const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (Main.cpp:289)
   by 0x3B5C5F: Assimp_Info(char const* const*, unsigned int) (Info.cpp:332)
   by 0x3AD83C: main (Main.cpp:207)
 Address 0x4ff164f is 175 bytes inside a block of size 176 alloc'd
   at 0x4837D7B: realloc (vg_replace_malloc.c:836)
   by 0x6AB99D: m3d_load (m3d.h:2921)
   by 0x6B63EF: Assimp::M3DWrapper::M3DWrapper(Assimp::IOSystem*, std::vector<unsigned char, std::allocator<unsigned char> > const&) (M3DWrapper.cpp:110)
   by 0x6B0AB0: Assimp::M3DImporter::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, aiScene*, Assimp::IOSystem*) (M3DImporter.cpp:181)
   by 0x53F8EC: Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Assimp::IOSystem*) (BaseImporter.cpp:130)
   by 0x3BA3F0: Assimp::Importer::ReadFile(char const*, unsigned int) (Importer.cpp:656)
   by 0x3AE935: Assimp::Importer::ReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (Importer.hpp:648)
   by 0x3ADAD7: ImportModel(ImportData const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (Main.cpp:289)
   by 0x3B5C5F: Assimp_Info(char const* const*, unsigned int) (Info.cpp:332)
   by 0x3AD83C: main (Main.cpp:207)

Invalid read of size 4
   at 0x6A820E: _m3d_getidx (m3d.h:2220)
   by 0x6ADF18: m3d_load (m3d.h:3321)
   by 0x6B63EF: Assimp::M3DWrapper::M3DWrapper(Assimp::IOSystem*, std::vector<unsigned char, std::allocator<unsigned char> > const&) (M3DWrapper.cpp:110)
   by 0x6B0AB0: Assimp::M3DImporter::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, aiScene*, Assimp::IOSystem*) (M3DImporter.cpp:181)
   by 0x53F8EC: Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Assimp::IOSystem*) (BaseImporter.cpp:130)
   by 0x3BA3F0: Assimp::Importer::ReadFile(char const*, unsigned int) (Importer.cpp:656)
   by 0x3AE935: Assimp::Importer::ReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (Importer.hpp:648)
   by 0x3ADAD7: ImportModel(ImportData const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (Main.cpp:289)
   by 0x3B5C5F: Assimp_Info(char const* const*, unsigned int) (Info.cpp:332)
   by 0x3AD83C: main (Main.cpp:207)
 Address 0x4ff1653 is 3 bytes after a block of size 176 alloc'd
   at 0x4837D7B: realloc (vg_replace_malloc.c:836)
   by 0x6AB99D: m3d_load (m3d.h:2921)
   by 0x6B63EF: Assimp::M3DWrapper::M3DWrapper(Assimp::IOSystem*, std::vector<unsigned char, std::allocator<unsigned char> > const&) (M3DWrapper.cpp:110)
   by 0x6B0AB0: Assimp::M3DImporter::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, aiScene*, Assimp::IOSystem*) (M3DImporter.cpp:181)
   by 0x53F8EC: Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Assimp::IOSystem*) (BaseImporter.cpp:130)
   by 0x3BA3F0: Assimp::Importer::ReadFile(char const*, unsigned int) (Importer.cpp:656)
   by 0x3AE935: Assimp::Importer::ReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (Importer.hpp:648)
   by 0x3ADAD7: ImportModel(ImportData const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (Main.cpp:289)
   by 0x3B5C5F: Assimp_Info(char const* const*, unsigned int) (Info.cpp:332)
   by 0x3AD83C: main (Main.cpp:207)

Invalid read of size 4
   at 0x6B2665: Assimp::M3DImporter::importMeshes(Assimp::M3DWrapper const&) (M3DImporter.cpp:399)
   by 0x6B0D56: Assimp::M3DImporter::InternReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, aiScene*, Assimp::IOSystem*) (M3DImporter.cpp:201)
   by 0x53F8EC: Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Assimp::IOSystem*) (BaseImporter.cpp:130)
   by 0x3BA3F0: Assimp::Importer::ReadFile(char const*, unsigned int) (Importer.cpp:656)
   by 0x3AE935: Assimp::Importer::ReadFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) (Importer.hpp:648)
   by 0x3ADAD7: ImportModel(ImportData const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (Main.cpp:289)
   by 0x3B5C5F: Assimp_Info(char const* const*, unsigned int) (Info.cpp:332)
   by 0x3AD83C: main (Main.cpp:207)
 Address 0x1d2f2e90 is not stack'd, malloc'd or (recently) free'd

if reading external assets really do crash, then when, under which circumstances and at which line does the wrapper crash?

It reads past the end of the buffer allocated on line 2921 and then later crashes in M3DImporter.cpp line 399 because the vertex indices read from uninitialized memory are invalid.

turol on 11 Dec 2019

The crash turol indicates is because m3d_load() does not know the size of the passed buffer.

line 2934: here the buffer end pointer "end" is set using the buffer's length (!)

No, in this line 4 bytes are read from the buffer and the (little-endian) value is assumed to be the size. It is never compared to the size of the buffer that was _actually_ passed.

For an example of a correct C-buffer API, take a look at the STB functions that take a buffer, eg _m3dstbi_zlib_decode_malloc_guesssize_headerflag or stbi_load_from_memory().

m3d_load() will _always_ read some distance past the end of the buffer if the data in the buffer is not well-formed.

There are many places where the buffer is _assumed_ to be of a particular size or to end with specific byte(s), and the binary format reads much further past the end.

Some notes on thread-safety:

setlocale() is not thread safe and should never be used in library code as it modifies the global state of the entire process.
This will thus cause intermittent data corruption bugs in users of the m3d SDK and Assimp library.
As Raymond Chen puts it - A library is a guest in someone else's house; don't go changing the carpet.

C-language callbacks cannot be _used_ in a thread-safe manner without a user cookie.
For an example of this, see stbi_load_from_callbacks().
Again, this pattern is repeated throughout STB.

RichardTea on 11 Dec 2019

👍1

Do not accuse people of trolling just because they disagree with you.

I do not. I have no problems with people disagreeing with me, but let's face it, you are keep saying invalid things, demanding arrogantly modifications without even realizing that a) those are either already done, b) go against the C specification, c) would result in incompatibility with non-Assimp environments or even worse, would cause feature-loss.

but just because some other project

Yeah, but those "some other project"s are the one of most prominent C projects in the history of computers. If Linus and Fabrice uses this underscore convention all over and over again, then I'm sure we are safe to use it too. Don't get me wrong, but I trust them more on this than you.

m3d_load() does not know the size of the passed buffer.

I tell you one more time. This is NOT TRUE, m3d_load() does know the size, what's more, I've even showed the line in which the size is used to calculate the buffer's end pointer. You can't see what's in front of you.

It is never compared to the size of the buffer that was actually passed.

Again, NOT TRUE. This is exactly why I can't take you seriously and why I think you're just a troll. What do you think this code does in M3DImporter.cpp line 162?

    // extra check for binary format's first 8 bytes. Not done for the ASCII variant
    if(!memcmp(buffer.data(), "3DMO", 4) && memcmp(buffer.data() + 4, &fileSize, 4)) {
        throw DeadlyImportError("Bad binary header in file " + file + ".");
    }

It is not possible to fix something that's not broken.

Besides, Assimp in general can't handle malformed model files, it keeps crashing even with valid obj, blend, b3d and gltf2 files just to name a few. It is non-sense to expect something from my importer that other importers can't do either (and just for the records, M3D is already more tolerant to malformed files than the other importers).

setlocale() is not thread safe and should never be used in library code as it modifies the global state of the entire process.

And again, you see? It is just an option in the SDK, which is not used neither by the M3DImporter nor the M3DExporter!

Only A3D exporter uses it and only for the number format, and even more, the original setlocale() LC_NUMERIC value is __saved and restored__. I'm not happy about setlocale() either, but the C standard and POSIX specifies that sprintf()'s formats must depend on it. We need consistent number formats in the ascii file, so it has to be set until the C specification of libc is redefined otherwise (don't count on it).

Please address your concerns about setlocale() to the C or POSIX comitees, not to me, I can't change the C standard.

C-language callbacks cannot be used in a thread-safe manner

Again, NOT TRUE. You've implemented a mutex, but you should have just simply use "thread_local" keyword on the IOSystem instance, because it is the C++ side that's not thread-safe as I've already pointed out to you. Thread local storage is a native C++ keyword, part of the C++11 spec: http://eel.is/c++draft/dcl.stc#1 This has nothing to do with C nor with C-language callbacks. You have to declare the IOSystem instance to be thread-local if you want it to be thread-safe, simple as that.

Cheers,
bzt

bztsrc on 13 Dec 2019

@kimkulling I've changed my mind.
We should simply remove this format, it's not good enough and upstream appears unwilling to accept any criticism.

They can maintain their own fork if partial, unsafe support is required.

RichardTea on 13 Dec 2019

Dear @kimkulling,

Could you please ellaborate?

All warnings are removed.
It does check the buffer's size (it even does compare it to the file's size), I can't add that when it's already there.
It does not and did not crash on valid input, and with the latest patch it does not read past the buffer with invalid input either.
I've fixed RichardTea's code, replaced the mutex with a thread-local variable.

What unresolved issue remained? Let me know.

Cheers,
bzt

bztsrc on 13 Dec 2019

And about your change requests, it is not that I don't accept criticism, it is about your change requests would break the code. No offense meant in any way, let me explain why:

to make "unused variable" messages disappear, you must tell the compiler if an argument is unused. It is only unused if interpreter is not configured (which is the case for Assimp, but not for the other environments). Using nameless arguments only works in C++, but not in C.
there must be two max defines, because one is always 32 bit wide, but the other depends on the configuration, as you can compile the SDK with 16 bit mesh indices too. Assimp does not do that, it only uses 32 bit indices, but other environments might. With uint16_t indices, the INDEXMAX 0xfffffffe define would generate incorrect code for "if(index < INDEXMAX)". Likewise, with uint32_t indices INDEXMAX 0xfffe would be incorrect. On the other hand, shape indices are always 32 bit wide (unused by Assimp, but other environment might use CAD models), which requires a define independent to the mesh index max.

Therefore the two defines are necessary (one is constant, the other is configuration dependent). With negative unsigned constants it was enough to use one single constant, as the compiler knew the size of the variable and compiled it correctly regardless if it was uint32_t shape index or configured for uint16_t or uint32_t mesh index.

Does this make sense to you now?

Cheers,
bzt

bztsrc on 13 Dec 2019

Regarding point 1.:
I proposed a way how to deal with the unused stuff. Please check if this would work for you.

Regarding 2.:
Got it, thanks for the detailed explanation. It was not so easy to understand ths concept in your code.

And I really appreciate your work and explanations on this. Let' continue working this way :-).

kimkulling on 13 Dec 2019

@kimkulling I've changed my mind.
We should simply remove this format, it's not good enough and upstream appears unwilling to accept any criticism.

They can maintain their own fork if partial, unsafe support is required.

I guess it is not always easy for a new developer to understand why a code review does not mean that we are criticize the developer personally but the issues we found in his / her code.

During my career I had a lot of fights with excellent developers because of my findings during a code review I made with them. This can be annouying for both parts, the developer himself and the reviewer who just wants to help. Doing this without seeing the person makes things not easier in comon.

So from my point of view we should try to restart the collaboration, calm down a little bit and continue to help bztsrc to reach your level of quality @RichardTea .

I can try to help here!

kimkulling on 13 Dec 2019

Dear @kimkulling,

Thank you, I appreciate that very much!

It looks like MSVC2013 is not fully C++11 compatible, I've provided an alternative to thread_local, testing it now.

Cheers,
bzt

bztsrc on 13 Dec 2019

It is never compared to the size of the buffer that was actually passed.
Again, NOT TRUE. This is exactly why I can't take you seriously and why I think you're just a troll. What do you think this code does in M3DImporter.cpp line 162?

This is the first comment where you mention this line. We are not psychic, it's your job to explain if we don't understand some part of your code. Or better yet, clarify the code or comments. You didn't explain that the length has already been checked by M3DImporter before calling m3d_load. In addition it was buried in a complicated if-clause and the comment only says "check for binary format's first 8 bytes" with no mention about file size. It is not obvious to other people that this checks the buffer size, especially since the standard convention in C is to always pass both the pointer and the size.

Also the check appears to be insufficient in the ASCII case. The test model cube_with_vertexcolors.a3d generates reads past the end of the buffer on m3d_load.h lines 2410 and 2411.

Besides, Assimp in general can't handle malformed model files, it keeps crashing even with valid obj, blend, b3d and gltf2 files just to name a few.

No it can't but we should try to make it handle them. Most of the obj etc code is old code that no-one's had time to refactor yet. We have limited people and limited resources. But if you're adding a new importer that's been written from scratch you should do it right from the beginning.

M3D is already more tolerant to malformed files than the other importers

No it isn't, you didn't fix the second crashing testcase I provided. Also AFL will still generate more crashing testcases within seconds. You should use AFL yourself to test your code.

setlocale() is not thread safe and should never be used in library code as it modifies the global state of the entire process.
Only A3D exporter uses it and only for the number format, and even more, the original setlocale() LC_NUMERIC value is saved and restored. I'm not happy about setlocale() either, but the C standard and POSIX specifies that sprintf()'s formats must depend on it. We need consistent number formats in the ascii file, so it has to be set until the C specification of libc is redefined otherwise (don't count on it).

No, your importer also calls setlocale. Since this is process-wide state it also affects the host program which might also be relying on locale and therefore your code can break it in unpredictable ways. Even if you restore the value it can still break things when multi-threading is used and bugs like that are really hard to debug. If you need specific formatting of numbers then you must write (or copy/include from somewhere else) functions which do it without affecting global state.

C-language callbacks cannot be used in a thread-safe manner

Again, NOT TRUE. You've implemented a mutex, but you should have just simply use "thread_local" keyword on the IOSystem instance, because it is the C++ side that's not thread-safe as I've already pointed out to you. Thread local storage is a native C++ keyword, part of the C++11 spec: http://eel.is/c++draft/dcl.stc#1 This has nothing to do with C nor with C-language callbacks. You have to declare the IOSystem instance to be thread-local if you want it to be thread-safe, simple as that.

This is not any better. It still breaks in "shared nothing" use case or if the program calling assimp uses context-switching. Again, it's a standard C idiom to pass a user-defined "context" pointer to callbacks so they can do the right thing in all cases.

turol on 13 Dec 2019

For M3D_INDEXMAX having two separate definitions of the macro is in fact the correct thing if there are also separate definitions of M3D_INDEX. I can't remember if casting negative signed integers to unsigned is UB or not but it's certainly not good form. With separate defines it's immediately clear what the code was meant to do.

turol on 13 Dec 2019

We should simply remove this format, it's not good enough and upstream appears unwilling to accept any criticism.

I don't think it warrants removal just yet, if @bztsrc is willing to fix the issues we pointed out and refrain from personal insults.

turol on 13 Dec 2019

Hi @turol,

Yes, I'm willing to fix the issues if your request are reasonable and does not break compatibility with other systems. You surely understand how important compatibility is, you would reject if somebody would ask to add a user-defined context pointer to aiImportFile(), now wouldn't you? Or just a path to look up the external assets in, which by the way, would be much needed? Bugs are different, I'll fix them right away, but with new features and API changes compatibility is always an issue.

That being said, about the ASCII reads past the buffer I'll check it, and fix it.

About the second test case you mentioned, I cannot download it from google drive, it gives me error. Could you please upload it somewhere else?

About setlocale, I understand your concern, but with respect I disagree. I'd like to point out that locale is not changed in general, only one aspect, the number format is saved and changed temporarily and it is restored in all possible code paths (if not let me know), therefore the risk is zero.

I see no point in reimplementing the libc in every single library, that's absurd. Besides, there's no way one could rewrite strtod to be as effective and fast as the ones musl and glibc provides. Assimp's fast_atof is very slow and buggy compared to libc's, and it is not as bullet-proof by far. And your ai_strtof is just as much dependent on setlocale. Otherwise I would have suggested to use one of those. It took several years for musl developers to get strtod right. Also the sprintf code is extremely complex, together with strtod more than 1200 SLoC! Assimp hasn't implmented sprintf either, it calls libc's version, so it is just as much setlocale dependent https://github.com/assimp/assimp/blob/master/include/assimp/StringUtils.h

Duplicating that much code by the way in every single library that wants formatted numbers is just not reasonable by any means. Let me remind you, unlike libpng and zlib, the libc is part of the C implementation and MUST be available on all platforms, and it automatically is, no additional linkage needed.

If setlocale is so cardinal to you, then I propose to avoid the ASCII variant altogether. The binary variant does not need nor use string to number conversions, and the ASCII variant is just an extra feature for easy debugging the models anyway. Would be a shame, but I would understand and I'm willing to remove the debug format from Assimp.

About the thread-local, it guarantees that if the application calling Assimp uses threading (pthread, lwp etc.) it will work correctly. If there's a context-switch, then again, that involves context-backup and context-restore by definition which includes the thread-local-storage pointer too (which is a dedicated general-purpose-register on x86), so no problem. Adding a user-defined pointer to the callback might be a C idiom, but that would break compatibility with the other environments, which is something that is not allowed. And it is not needed either with the current thread-local IOSystem. The last patch compiles with MSVC2013 too, and the other compilers had no problems in the first place.

Cheers,
bzt

bztsrc on 13 Dec 2019

How to deal with setlocale: get the older settings, store it, set your new one and after the import / export just restore the old state and everyone here is happy at all :-) ...

kimkulling on 13 Dec 2019

you would reject if somebody would ask to add a user-defined context pointer to aiImportFile(), now wouldn't you?

It's only needed for when the code has user-provided callbacks and C++ generally handles those with user-defined subclasses. If someone wanted to add another such callback (for example memory allocations) to the importer I would not reject it out of hand.

Or just a path to look up the external assets in, which by the way, would be much needed?

This can already be achieved by overriding the IOSystem.

That being said, about the ASCII reads past the buffer I'll check it, and fix it.

That's good.

About the second test case you mentioned, I cannot download it from google drive, it gives me error. Could you please upload it somewhere else?

It worked for me when I tested the download from another computer in private mode. What error are you getting? I don't really have another place to host files right now.

About setlocale, I understand your concern, but with respect I disagree. I'd like to point out that locale is not changed in general, only one aspect, the number format is saved and changed temporarily and it is restored in all possible code paths (if not let me know), therefore the risk is zero.

This is not good enough. None of the other importers/exporters of assimp touch locale so it's implicitly part of our API. If you're unwilling to change your code then I'm afraid I will also have to recommend that it be removed from assimp.

Assimp's fast_atof is very slow and buggy compared to libc's, and it is not as bullet-proof by far.

Can you provide specific cases where it fails?

And your ai_strtof is just as much dependent on setlocale.

That is unfortunate but like I said, old code. At least C++ is getting to_chars / from_chars. We will switch to that eventually.

About the thread-local, it guarantees that if the application calling Assimp uses threading (pthread, lwp etc.) it will work correctly. If there's a context-switch, then again, that involves context-backup and context-restore by definition which includes the thread-local-storage pointer too (which is a dedicated general-purpose-register on x86), so no problem. Adding a user-defined pointer to the callback might be a C idiom, but that would break compatibility with the other environments, which is something that is not allowed. And it is not needed either with the current thread-local IOSystem. The last patch compiles with MSVC2013 too, and the other compilers had no problems in the first place.

No, this only works if you actually switch threads. If you use user-space context switching like Boost.Context, windows fiber functions or other platform-specific things it will not do the right thing. Passing a context pointer (and all other assimp importers) will work in that case. Remember, this is a very popular library and is used in all kinds of weird cases we can't even think of.

turol on 13 Dec 2019

How to deal with setlocale: get the older settings, store it, set your new one and after the import / export just restore the old state and everyone here is happy at all :-) ...

The code currently does that and it is not sufficient. Consider the following scenario:

There are two threads, "A" and "B".

A changes locale, saves the old one and starts doing it's thing.
B is running at the same time, saves the current locale and sets another one.
Oops! A is now using locale it was not expecting, gets wrong results.
A finishes it's thing and restores the original locale.
Oops! B is now using locale it was not expecting, also gets the wrong result.
B finishes its work and resets to the locale it originally started with, which was set by A and is not the locale the main program is expecting. Now every future operation will use the wrong locale.

The locale API is really unfortunately designed and can't be used correctly in a multithreaded scenario. No other importer in assimp changes it and we really should get rid of all calls which depend on it. C++ upcoming from_chars / to_chars is the correct API we should be moving to.

turol on 13 Dec 2019

Hi,

How to deal with setlocale: get the older settings, store it, set your new one and after the import / export just restore the old state and everyone here is happy at all :-) ...

That's the problem: that is exactly what the code does, but people here are not happy about it.

It's only needed for when the code has user-provided callbacks

Nope. I want a list of paths to be added to aiImportFile() in which to look the external assets up. See, there's nothing callback related about this request.

This can already be achieved by overriding the IOSystem.

No, you cannot override IOSystem from the application's side by calling aiImportFile(). But you should be able, I agree! :-)

I don't really have another place to host files right now.

Erhm, here on github maybe?

None of the other importers/exporters of assimp touch locale

No, they don't, but they rely on it regardless! That's causing problems in many importers / exporters right now, and renders Assimp unusable in many cases. For example, you can't use assjson, assxml or obj (or any other text based format that's using stream operator or sprintf for export) importer on files which were exported on a machine with different locale, and that is a big problem.

Consider the following scenario

This scenario is pretty far fetched and very far from everyday practice. It is highly unlikely that an application wants to mess with the number format locale during model loading, no matter how many threads it's using. Someone exporting on a machine with one locale and sending it to someone with a machine with another locale is much more likely scenario, specially between model designers and game engine developers around the globe.

Now let's talk about the solutions:

about the ASCII reads past the buffer, the problem was not in the SDK, that's why my test cases didn't showed it. Fixed.
no underscore names used in wrappers no more
I've moved two defines into M3DWrapper.h, these are:
if you remove M3D_ASCII, then only the binary format will be supported -> no need to worry about setlocale, won't be used at all
if you remove ASSIMP_USE_M3D_READFILECB, then textures won't be loaded, only their names will be returned unchecked (that is, texture names for non-existing files will be returned too) -> don't worry about thread_local, won't be used at all

Cheers,
bzt

bztsrc on 14 Dec 2019

This can already be achieved by overriding the IOSystem.

No, you cannot override IOSystem from the application's side by calling aiImportFile(). But you should be able, I agree! :-)

There's another function aiImportFileEx which takes a pointer to aiFileIO struct. Which by the way does have a UserData member. Your use case of adding more search paths can be accommodated with this API. The use case of safe multithreading in all situations can't be accommodated with your API and so you must add a function which allows similar userdata to be passed.

I don't really have another place to host files right now.

Erhm, here on github maybe?

Last I checked gist didn't support binary data directly so I'd have to create a repository for it. It's much simpler to just use gdrive. I've reuploaded with hopefully more permissions, try this:
https://drive.google.com/file/d/18FCd9INbSN0wPoBeyCCUAfbCpZS4lcM8/view?usp=sharing

None of the other importers/exporters of assimp touch locale

No, they don't, but they rely on it regardless! That's causing problems in many importers / exporters right now, and renders Assimp unusable in many cases. For example, you can't use assjson, assxml or obj (or any other text based format that's using stream operator or sprintf for export) importer on files which were exported on a machine with different locale, and that is a big problem.

This is a problem and should be fixed but changing locale is not the right fix, it will break applications. C++20 from_chars is the right solution. Since we can't go to C++20 yet we need to find a library which does the same. I couldn't quickly find such a library, does anyone know of one?

This scenario is pretty far fetched and very far from everyday practice. It is highly unlikely that an application wants to mess with the number format locale during model loading, no matter how many threads it's using.

No it's not far-fetched, it will happen whenever two threads call setlocale without synchronization. And since the other thread might be in some other library we can't do that. Even if the threads get the right result they will still mess up the state for the rest of the program.

about the ASCII reads past the buffer, the problem was not in the SDK, that's why my test cases didn't showed it. Fixed.

You appear to have worked around it instead of a full fix. If I understand the code correctly you preprocess the string to make sure there's a zero terminator in there somewhere. This is not sufficient, AFL can still crash it easily. Here's some more testcases:
https://drive.google.com/file/d/1Y29HTdwWV-dGZTL7-l0YRbWy6HeDYERC/view?usp=sharing

if you remove M3D_ASCII, then only the binary format will be supported -> no need to worry about setlocale, won't be used at all

It should default to disabled so users don't accidentally have setlocale called when they're not expecting it. No other assimp importer calls it so there's an implicit API contract that we don't change the locale.

if you remove ASSIMP_USE_M3D_READFILECB, then textures won't be loaded, only their names will be returned unchecked (that is, texture names for non-existing files will be returned too) -> don't worry about thread_local, won't be used at all

This should be the default. All other assimp importers can be used in thread-safe manner and that should be the default for this one too.

turol on 14 Dec 2019

I couldn't quickly find such a library, does anyone know of one?

yes, I do, it's called libc. (Don't get this the wrong way, but that's what libc is for, you just would have to accept the fact that setlocale is part of the solution.)

You appear to have worked around it instead of a full fix.

I'm not sure what you mean. The API is perfectly clear that ASCII variant input must be a zero terminated string with optional utf-8 characters in it. True, that the code has some checks and tries to validate the input, but by default providing an invalid input means undefined-behavior in general. This is no different for the M3D SDK, with invalid input you get an UB.

About the defines, I knew that providing an option for you would be the right thing to do!

It should default to disabled so users don't accidentally have setlocale called

I don't agree, but so be it. The chance is extremely slim that they "accidentally" use setlocale, and I seriously and most sincerely doubt that any Assimp user who messes with the number locale would be surprised if that also has affect on the loading and saving of the text-based models, but okay, I'll remove it.

This should be the default. All other assimp importers can be used in thread-safe manner and that should be the default for this one too.

About this one we should go another round. I agree that it needs to be thread-safe, no question about that. But you don't seem to understand that with the C++ native thread_local keyword this solution is already 100% thread-safe. I can accept your decidion to remove this define, but these are invalid reasons, therefore your argument is invalid.

So the question is, knowing that this solution is already thread-safe (guaranteed by the C++ specification), do you still want to remove this feature?

Cheers,
bzt

bztsrc on 15 Dec 2019

yes, I do, it's called libc. (Don't get this the wrong way, but that's what libc is for, you just would have to accept the fact that setlocale is part of the solution.)

No, it's a different, non-thread-safe API. setlocale is not and cannot be part of the solution because it will break things and you need to accept that.

You appear to have worked around it instead of a full fix.

I'm not sure what you mean

You didn't actually touch the line which reads past the end of buffer so I'm not sure _why_ your changes fixed that test case. It wasn't a full fix either, see the zip of crashing testcases I provided.

But you don't seem to understand that with the C++ native thread_local keyword this solution is already 100% thread-safe.

It's only thread-safe if the user callback doesn't do user-space context switches. I provided several APIs and a video describing an architecture which does this. Also calling this code via FFI from other languages might cause similar issues but I'm not entirely sure about that.

I can accept your decidion to remove this define, but these are invalid reasons, therefore your argument is invalid.

Just because you can't see how someone else's use-case works doesn't make their argument invalid.

So the question is, knowing that this solution is already thread-safe (guaranteed by the C++ specification), do you still want to remove this feature?

I want you to make it truly thread-safe but if you won't then removing it will suffice.

turol on 15 Dec 2019

SetLocale is already in use for the Collada-Exporter, so we already have this issue.

And @turol thanks for the scenario: yes this is an issue which can happen. So I did some research and at stackoverflow they recommend this solution for a threadsafe setlocale-call:

https://stackoverflow.com/questions/4057319/is-setlocale-thread-safe-function

kimkulling on 16 Dec 2019

As far as I can tell the other exporters don't use setlocale but instead some version of ::imbue which the documentation indicates only changes that particular stream, not global locale. This should be safe.

That link suggests either uselocale or _configthreadlocale. As I understand it they only fix the problem of setlocale itself being non-thread safe but not the problem of possibly messing up the state for the rest of the program. In particular doing user-space context switching will still probably break. Also I'm not sure if they exist universally, people do use assimp on platforms like iOs and Android.

turol on 16 Dec 2019

SetLocale is already in use for the Collada-Exporter, so we already have this issue.

No, it is not. At current master, the only file that calls setlocale() in any configuration is m3d.h

Collada, like most of the existing text-based importers/exporters use a C++ std::stringstream and set the locale _of that stream_ appropriately:

// make sure that all formatting happens using the standard, C locale and not the user's current locale
  mOutput.imbue( std::locale("C") );
  mOutput.precision(ASSIMP_AI_REAL_TEXT_PRECISION);

Unfortunately std::stringstream is not available to a C file like m3d.h.

There may be some exporters or importers that currently rely on the process locale, if so that's a bug that also needs fixing. A quick search implies that the assxml exporter _may_ be relying on locale but I have not checked (I don't use that exporter so my users will not encounter it).

I doubt any of the importers rely on locale because the fast_atof locale is hardcoded.

And @turol thanks for the scenario: yes this is an issue which can happen. So I did some research and at stackoverflow they recommend this solution for a threadsafe setlocale-call:

https://stackoverflow.com/questions/4057319/is-setlocale-thread-safe-function

The C function setlocale() simply _cannot_ be safely used in any library or SDK, because it is global. It _cannot_ be protected by any means whatsoever, because the library/SDK has no knowledge of what the rest of the application is doing at the time - a library only knows what _it_ is doing.

The Linux manpages explicitly mark it as "thread-unsafe": http://man7.org/linux/man-pages/man3/setlocale.3.html#ATTRIBUTES

setlocale() is only suited for direct use by an application.

There are many standard functions that are only suited to specific situations. That doesn't make them necessarily _bad_, only that you must not use them without careful thought as to the consequences.

RichardTea on 16 Dec 2019

👍1

Hi,

You didn't actually touch the line which reads past the end of buffer so I'm not sure why your changes fixed that test case.

I haven't touched that line because it wasn't broken. I made sure of that invalid input that caused the issue will never make that far (as it shouldn't), that's what fixed the case.

But now this is irrelevant, just as the setlocale thing because I've removed ASCII support entirely.

It wasn't a full fix either, see the zip of crashing testcases I provided.

I would, but google drive still not working. Please use github.

It's only thread-safe if the user callback doesn't do user-space context switches.

How? If the user-space context switch brakes a "thread_local" variable (or any other variable as a matter of fact), then that context-switch code is broken and shouldn't be used in the first place.

But again, irrelevant, as the code only returns texture filenames now, it does not load them, so IOSystem is not used at all, and therefore it can't do any user-space context-switch.

(Huh I'm sorry, but even the idea of user-space context-switch is fundamentally BAD to me. Context-switches should be only and exclusively done in supervisor-mode by the kernel (cooperative yields and preemptive alike). Process should only allowed to do thread or LWP switching, and there the "thread_local" keyword must be respected per the C++ specification.)

Cheers,
bzt

bztsrc on 17 Dec 2019

Try this then: https://github.com/turol/assimp/commit/4ebb8d0ff0cf5fc0e2d918d0b28873b3e16c187d

It's not meant to be merged.

(Huh I'm sorry, but even the idea of user-space context-switch is fundamentally BAD to me. Context-switches should be only and exclusively done in supervisor-mode by the kernel (cooperative yields and preemptive alike). Process should only allowed to do thread or LWP switching, and there the "thread_local" keyword must be respected per the C++ specification.)

You're wrong here, there's many use cases and APIs for this. Win32 has CreateFiber / SwitchToFiber. Posix has (deprecated) makecontext / swapcontext. Many platforms are supported by Boost.Context. Golang and node.js use something similar but they're obviously not C++. Even C++ is standardizing these things in some form. These are all much faster than kernel-space context switch. See above for the Naughty Dog video for a concrete use case.

turol on 18 Dec 2019

I've checked your samples with the M3D validator. As it turned out, neither of them are valid M3D files. Also ASCII format was entirely removed.

Both thread_local and setlocale() has been removed; and with that your concerns. The patch has been merged, and there was no further issues. Could someone please close this issue?

Thanks,
bzt

bztsrc on 23 Jan 2020

Even if they are not valid files the importer should not crash. I'll try the importer again to see if it's still possisble to crash it.

turol on 23 Jan 2020

From the M3D SDK manual: "There are some minimal checks, but in general you are expected to pass only valid input to the loader, otherwise you'll get undefined behaviour."

There's a very good reason why the M3D SDK provides a separate validator.

Okay try the importer again, but keep in mind that ASCII variant are not handled any more. Also I've tried your first case which not even made it through the m3d layer, Assimp has thrown an Exception before it could call m3d_load.

bztsrc on 23 Jan 2020

From the M3D SDK manual: "There are some minimal checks, but in general you are expected to pass only valid input to the loader, otherwise you'll get undefined behaviour."

That statement should terrify you. Undefined behaviour is always a serious bug and must be avoided at all costs.

The input data may have been corrupted on disk or in transit, actually a different format that _just so happens_ to match the M3D header, or even be a deliberate attack by a malicious actor. The first two are certain to occur.

If I pass invalid HTML to your browser, while it will attempt to interpret the garbage I sent and might manage to render a partial version, if what I send is sufficiently invalid then it will reject the data and report that it couldn't understand it so it threw it all away.

The loader _must_ make a good-faith attempt to _properly deal_ with all forms of invalid input. In some cases it may be able to correct for minor issues (eg missing material data), in most cases it will simply reject the data, perhaps without much explanation.

A validator is a developer tool that attempts to offer guidance to someone writing an _exporter_ so they can try to ensure their output is strictly compliant with the specification. It should be incredibly strict, and give as much detailed information about non-compliance as possible. It's not something that anyone else should ever need to run.

You'll notice that many of the other assimp formats have 'invalid' model data within the automated test cases. This is to test whether loaders for those formats are still properly rejecting the known invalid data edge cases that have caused problems in the past.

RichardTea on 24 Jan 2020

👍1

It still crashes when fuzzed. I've force-pushed https://github.com/turol/assimp/tree/m3d-crash with new test cases.

Also note that assimp will be getting fuzzed with OSS-Fuzz in the future so eventually all of these will have to be fixed.

turol on 24 Jan 2020

The issue was "Huge set of warnings in M3D importer". Has that been resolved? Then close this issue.
Do you have other problems? Open a new issue, specifying the details.

Undefined behaviour is always a serious bug and must be avoided at all costs.

Tell that to the POSIX comitee and to the glib developers too. For example strlen(NULL) WILL crash, and it is supposed to crash.

Passing invalid input is always a serious bug and must be avoided at all costs.

The input data may have been corrupted on disk or in transit

M3D is protected for that, there are checksums in the zlib payload. What turol did, actually was:

remove the 8 bytes header
uncompress the payload
toggle some bits randomly
compress the payload
recalculate checksums and add a new header
There's no way that a transit error or disk corruption can cause this.

new test cases.

I've checked, neither of those are valid M3D files. Not one of them passes validation.

M3D works perfectly for all valid inputs, and it does not misbehave on a great deal on invalid inputs, but it can't take into account all invalid possibilities. Expecting that is irrational.

There are almost 400 open issues for Assimp, and approx one third of them about Assimp CRASHING ON VALID INPUT (dear Lord, nearly 100 cases!).
Don't take this the wrong way, but you should focus your energy on fixing those cases instead of generating invalid inputs. That would be something that could Assimp actually benefit from.

And again, close this case as it has been resolved. If you have other problems than "Huge set of warnings", then open a new case.

Cheers,
bzt

bztsrc on 29 Jan 2020

The issue was "Huge set of warnings in M3D importer". Has that been resolved? Then close this issue.
Do you have other problems? Open a new issue, specifying the details.

Agreed, this issue is becoming too big and unwieldy. @RichardTea please close it. Also could you fix all the remaining MSVC warnings and enable warnings-as-errors on Appveyor?

M3D is protected for that, there are checksums in the zlib payload. What turol did, actually was:
1. remove the 8 bytes header
2. uncompress the payload
3. toggle some bits randomly
4. compress the payload
5. recalculate checksums and add a new header

I did not. I just ran AFL on it. If you look at the filenames they will tell you what AFL did. op:flip1 means one bit was flipped from the previous (non-crashing) file to generate this one. flip:2 means two sequential bits were flipped.

There's no way that a transit error or disk corruption can cause this.

These kinds of errors can easily be caused by corruption in transit or storage. Also no checksums were altered so if there was one it's not very good since it didn't catch the flips.

I've checked, neither of those are valid M3D files. Not one of them passes validation.

No but the library must gracefully handle corrupt files, as already explained above.

M3D works perfectly for all valid inputs, and it does not misbehave on a great deal on invalid inputs, but it can't take into account all invalid possibilities. Expecting that is irrational.

I don't expect perfection, I expect _reasonable effort_ to fix issues especially when they've been pointed out to you and we have provided test cases. And since assimp is now being fuzzed by OSS-Fuzz, eventually we _will_ have to fix these.

Don't take this the wrong way, but you should focus your energy on fixing those cases instead of generating invalid inputs. That would be something that could Assimp actually benefit from.

I checked this because of your unwillingness to believe the issue. The classic case of "someone is wrong on the Internet" and "you can't argue with a crashing test case". And it only took some minutes and almost no effort to set this up. You should try AFL yourself, it's instructive.

I do work on other parts of assimp (like the testsuite lately) but I have limited time and it's even more limited if I have to repeatedly argue about issues in someone else's code. It's much easier to _prevent_ the introduction of issues than fix them afterwards.

turol on 30 Jan 2020

Thanks all, I will close this issue as the warnings it was originally about have been corrected.

I'm sorry I left this open so long, I removed this importer from my product and CI builds due to the quality issues raised above so had to do manual check to confirm.

@bztsrc I hope in time you will come to understand why the other code quality issues raised in this discussion matter.

RichardTea on 18 Feb 2020

I hope in time you will come to understand why the other code quality issues raised in this discussion matter.

I don't want to disappoint you, I understood all along. I hope in time you'll get enough experience to be able to estimate true risks and prioritize between solutions; maybe some day you'll be wise enough to realize what _real_ code quality is about, and you won't make the same mistakes you made here.

Best wishes,
bzt

bztsrc on 21 Feb 2020

Assimp: Huge set of warnings in M3D importer

All 58 comments

Related issues