New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
msilib.SummaryInfo.GetProperty() truncates the string by one character #45445
Comments
Attached is a patch that fixes the truncation of the property values |
The patch looks correct, it's now a matter of unit tests. |
I have updated the patch for current trunk (though no real changes required). I also include a testcase. One thing to review is that I added functionality to the tests to create the MSI to be tested. (felt this was safer than touching one of the ones under %systemroot%\installer. |
+ path, msilib.schema, "Ptyhon Tests", "product_code", "1.0", "PSF") |
And fix the typo... (thanks Ezio) |
This patch seems okay to me, as far as it goes. I'd like to hear Martin's feedback, but I think it should be committed. And I realize the rest of this message doesn't apply to the patch, but it does address other problems in summary_getproperty(). At least one of these led to the original problem with the truncated character.
Maybe these last 3 should get their own new issue. |
Responding to Eric's comments
|
Despite the fact that as of now (or 6+ years ago!) the only way to trigger the malloc() is via VT_LPSTR, I still think the way the free() call is written is bad. What if another type is added? If that were fixed, I still think the basic idea of this patch in isolation of the other raised issues is a good idea. If it was converted to a PR, I'd support merging it (although I'd like to hear from the Windows folks). But it is curious that this bug is so old and it hasn't caused more problems. |
I have created a PR bpo-4517 from the patch. Would it be better to track the malloc problem in a new issue? As for why this never caused any problems… msilib is pretty standalone, and not one of the most used modules. It is also pretty trivial to roll your own solution with ctypes (or any FFI library), which is what I did when I hit this bug. |
I made a shot to address the free() call. Hopefully this makes sense. |
I resolved some conflicts and will merge this once CI completes. If the backport to 2.7 isn't automatic then it may wait until someone else comes in to do it. |
As expected, the 2.7 backport needs more work. Leaving this open for anyone who wants to handle it. I'll happily click merge for you if CI passes. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: