#52 Integer truncation bug in handling of EMR_POLYLINE

[Derek Foster]

After experiencing a symptom such that passing numbers larger than about 32767 to the CDC::Polyline function caused the corresponding lines to not be drawn, I have tracked the problem down to the following two lines of code, in meta2pdf.cpp:

case EMR_POLYLINE: {
const EMRPOLYLINE16 dt = (const EMRPOLYLINE16 ) lpEMFR;

I am unable to test changes to this code (because there is no included way to build the library from source :-( ), but this appears to be the wrong type for EMR_POLYLINE. I think it should be using an EMRPOLYLINE struct, not EMRPOLYLINE16. (Note that the corresponding code that generates debug output uses EMRPOLYLINE, not EMRPOLYLINE16, and that in the other corresponding pairs of commands such as EMR_POLYDRAW vs. EMR_POLYDRAW16, EMR_POLYBEZIERTO vs. EMR_POLYBEZIERTO16, etc., the 16-bit version of the commands all use the 16-bit versions of the structs, and the non-16-bit versions of the commands use the non-16-bit versions of the structs. I am guessing this bug started out as a copy-paste bug of some kind.

Please fix, ASAP. This is triggering an active customer bug report.

• Also, if you generate a new release fixing this, please consider also making the four ...ToUnitEx functions be declared 'static', and the four '..ToUnit' functions be declared 'const'. I am currently having to code workarounds for the fact that these functions are missing these attributes.

Thanks!

[zyx]

Thanks for a bug report. You are right, it's a copy&paste error.

because there is no included way to build the library from source

That's not true at all. There is that CMake project file, with which you can build the code yourself. You need the dependencies, like with everything, of course.

consider also making the four ...ToUnitEx functions be declared 'static', and the four '..ToUnit' functions be declared 'const'. I am currently having to code workarounds for the fact that these functions are missing these attributes.

What precise problem is that supposed to fix, please? The static is not necessary and the ToUnit/UnitTo, well, the const will make the API cleaner, that's true, though I miss a real problem behind that. You rarely have a const TLitePDF object, do you not? I'm not against the change, I'm only wondering about the background of this change.

[Derek Foster]

Hi, zyx. Thanks for the quick response.

With regard to building litePDF from source, perhaps I am operating on old information or am misinterpreting what you said, but in ticket #47, on 2022-09-07, you said "The litePDF is built on Windows, but its build files are very special. I cannot share it." My company has a policy in which we are not allowed to use DLLs which we did not build from source. (This is to avoid supply-chain attacks where an attacker might post innocent source code, then put a trojan horse into a DLL that was supposedly built from that source code.) As such, another employee of my company was tasked with building a version of the litePDF DLL which we can use. She found it quite difficult (probably mostly due to its dependencies), although she eventually did figure out how to create a basic DLL that worked for our initial needs. (Some features are still missing in our rebuilt DLL, such as direct access to the underlying PoDoFo document, which we are likely to need later.) I don't know if your CMake script was part of how she finally succeeded in building it. I also don't know what the specific stumbling blocks are, or why our DLL is not equivalent to the one that you shipped. I will try to research this.

With regards to making the ...Ex functions 'static': I am trying to declare some global constants which include measurements and layout parameters which will eventually be used to create PDF documents. At the time that these global constants are initialized, there is no TLitePDF object in existence, and therefore I have no object on which to call functions like TLitePDF::MMToUnitEx. I shouldn't need to have such an object: since the functions wouldn't use it anyway. There is no need for these functions to be nonstatic. Also, at one point, I was trying to use a multilevel inheritance hierarchy of classes inheriting the TLitePDF class. I wanted to pass in some measurements from my child class to its parent class constructor (the parent class's constructor took a parameter to specify the PDF units, and the child class took parameters specifying width/height), but couldn't do it because the parent class's own parent TLitePDF superclass had not been initialized yet, and it would therefore be invalid (per C++ rules) to be calling its nonstatic member functions such as MMToUnitEx before it had been fully constructed. (Note: making the parent class contain a TLitePDF object instead of inheriting from one, aka "prefer composition to inheritance", doesn't avoid this problem.) In general, I would say that a good guideline is that if a function doesn't use any members of the class it is defined on, it should be declared 'static' unless such use is anticipated in the future. To not declare it as static limits its potential uses, but provides no benefit: It can't be called without an instance, and it can't be called before construction. (Not that performance is much of an issue in this particular case, but non-inlined nonstatic functions are also often slightly slower, due to the caller having to pass the extra 'this' parameter, which in a function that could have been static, would just be ignored, taking up CPU time for no benefit.)

With regards to making functions 'const': I am subclassing the TLitePDF class, and trying to define member functions that perform various kinds of measurement and layout tasks. (For instance, one such function takes a parameter of a "rectangle" object, specified in our company's coordinates, and returns a corresponding rectangle specified in PDF coordinates.) Our company coding standard specifies that functions which don't change the object they are members of should be declared 'const', in accordance with various standard and widely-agreed-upon C++ guidelines. However, these functions need to call functions like MMToUnit, which don't change anything in the object, and yet aren't currently declared 'const', which means that in order to be const-correct, my functions need to call these functions using code like const_cast<LitePDF*>(this)->MMToUnit(blah) rather than just calling them normally. As far as I can see, there is no reason to need a workaround like this. Nearly every set of widely adopted C++ guidelines or coding standards recommends that functions that don't change the observable state of their object should be declared as 'const' unless there is some compelling reason not to do so. Having low-level functions which are not const-correct complicates the implementation of higher-level functions which are const-correct and must call those low-level functions.

Declaring functions as 'const' and/or 'static' is not just a case of making the API 'cleaner'. It has real effects on who can call the functions and when (at least without workarounds such as const_cast).

[zyx]

"The litePDF is built on Windows, but its build files are very special. I cannot share it."

I meant the files in the SDK, because they bundle the dependencies into a single .dll file. The CMake files in here expect the dependencies to be prebuilt and available as runtime dependencies (and as other DLL files). Honestly, I doubt anybody else tries to build the library, people might just use the SDK, because it's the simplest thing, thus if there are any issues with the CMake files in here, then nobody will find it. Your company policy in this regard makes perfect sense.

With regards to making the ...Ex functions 'static'

I'm not against it, your explanation makes perfect sense, I'm only not used to it, which is the reason why it is as it is.

functions which don't change the object they are members of should be declared 'const', in accordance with various standard and widely-agreed-upon C++ guidelines.

I agree to that and also follow that practice. This case, about these functions, it's a plain overlook from my side. My fault.

There is one thing I do not understand, when you build from the source, and you know what to change (your analyse was correct, it's a copy&paste typo), why did you write the following?

Please fix, ASAP. This is triggering an active customer bug report.

When you know all of this, would it not be simpler to propose a patch here, then it could be included in the sources? That's how the Open Source works, after all. I welcome patches, there's no need to be shy to send/propose them.

[Derek Foster]

Hi, zyx.

To answer your question: "simpler" depends somewhat on what someone is used to doing and is set up to do. I hoped my description of the necessary changes would be sufficient. I have not personally actually built litePDF and its dependencies from source: Another employee does that (this is a big company!) and her setup for doing so is highly individual and quite complicated and not easily transported to my machine. We are looking at ways to simplify that, but for the time being, I found the bug I am reporting here by looking at litePDF source code via a web browser pointed at sourceforge.net, not by using source code on my local machine.

My company is not normally set up to work with SourceForge and Subversion (we use GitHub/Git, etc.), so there was a bit of a learning curve involved in creating a patch file for this. Nonetheless, I have now installed Subversion, downloaded litePDF source, modified the source files, and created a patch file. Hopefully, I did it correctly, and this file will be enough for you to make the changes. In the interest of time (and avoidance of possible security issues involved in using dependency DLLs which we did not build ourselves), I did not try compiling these changes, but hopefully these changes are simple enough that I made them correctly without errors, or if I introduced errors, that they will be simple ones that you can fix easily. Please let me know if you have trouble with this patch file. (It was taken from 'svn diff > ticket52.patch' in the 'trunk' folder.)

Thanks for your help.

[zyx]

I'm sorry for the misunderstanding. Your description for the suggested changes were all fine. I thought, and expected, that when you build on your own you also have it fixed locally (on the other machine), without a need to wait for me when I get some free time to do the changes. As if you'd have the changes deployed, it made more sense to just use them too, to avoid any overlook on my side (aka to provide a patch).

I do not know how it works in your company, maybe you need to wait until the upstream accepts any suggested change. The POLYLINE part was a clear bug, there was no doubt the change will be done, thus you could just go ahead and build a fixed version in your environment. That's why "please fix ASAP" did not make much sense to me, because you build from the source code, you can do any changes in the code, it's all up to you whether or what modifications you do. Bug fixes are good to be proposed to the upstream, just as you did here, thus others can benefit too. Thank you for that.

[Derek Foster]

Hi, zyx. Thanks for the clarification.

Part of the reason that I didn't just request that my coworker make the change to our own local copy was that I wanted to be sure that you confirmed that it was a real bug (and that the fix I proposed was the correct fix for it) and not a misunderstanding on my part.

Also, in general, if we change our own local copy of an external library to incorporate changes of our own, there can be negative consequences, such as:

1. If we later encounter any additional bugs, we have to verify that it's not due to our internal changes, and that it's possible for the library author to reproduce it even though the library author might not have our internal changes,

2. If someone else reports bugs against the library, we likewise have to wonder whether that bug might be affected by any local changes we make,

3. We have to retest code that depends on the library multiple times: each time we make our own local changes, and then again when we get those same changes (and maybe others) in an official release from the library author, and

4. When we eventually do receive a new updated version of the library, we will then have to merge it with any local changes we have made, which creates the potential for merge conflicts and errors due to merging.

In general, making these kinds of local changes to external libraries can easily become a maintenance problem. For these and other reasons, we (and also other companies I have worked for in the past) generally prefer to avoid making local changes to external libraries we use, unless there is no other practical option. Also, as you mentioned, we would like to contribute bugfixes back to the wider community using the library, as a "thank you" contribution to a project that we have benefited from, and a gift to its other users, in the spirit of open source.

Clearly, if it's going to be a long time before you can issue a new release, we'll have no choice but to modify our local copy of litePDF, but we'd prefer to avoid these kinds of potential maintenance problems if possible and just rely on official releases. That way, we'd be confident that what we are using is what everybody else is using, and all bug reports, etc. that affect other people would affect us, and vice versa.

Do you have an estimate of how long it will take you to issue a new release containing a fix for this issue? If it's going to be more than a week or so, I'll probably ask my coworker to apply my patch file to our local copy in the meantime.

Thanks for any information!

[zyx]

Yes, doing local changes has its consequences. It the light of your "fix ASAP", I believe the easiest is just to fix locally for the time being, then you do not need to wait for slow people like me. As you said, ideally after at least a confirmation from the author about the proposed changes.

For the time frame, I wanted to look on it earlier, but I did not manage it for the past two weekends, which is a shame. I'm sorry. I may have some free time at the end of this week. I'll update this bug once it's done.

[Derek Foster]

OK, thanks for the update. If I don't see an update by Monday (Feb 24), I'll recommend to my coworker that we should make the change locally.

[zyx]

I just figuerd (the compiler told me) why the functions cannot be const. They call GetUnit(), which calls a library (dll) function, which can modify internal state of the class (like by loading the dll and filling the function pointers). I will keep the static part of your patch.

[zyx]

I applied your change (that possible part of it) in [r40] and corrected the typo in the EMR_POLYLINE handling in [r41]. I also checked other commands for similar typos, but it seems this was the only broken.

To be released as 2.0.5.2.

[zyx]

I just made the release. You can find it in the Files section.

[Derek Foster]

We'll update to 2.0.5.2 ASAP. Your help is very much appreciated! Thanks!