classification
Title: msilib.SummaryInfo.GetProperty() truncates the string by one character
Type: behavior Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.8, Python 3.7, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: amaury.forgeotdarc, atuining, berker.peksag, eric.smith, ezio.melotti, loewis, markm, miss-islington, paul.moore, steve.dower, tim.golden, uranusjr, zach.ware
Priority: normal Keywords: patch

Created on 2007-09-04 22:03 by atuining, last changed 2019-02-19 03:14 by steve.dower. This issue is now closed.

Files
File name Uploaded Description Edit
_msi.patch2.txt atuining, 2007-09-04 22:03
issue1104_msi_2.patch markm, 2011-04-04 01:58 Updated patch to fix and test off by one error in _msi.c review
issue1104_msi_3.patch markm, 2011-04-04 02:38 fix typo of Ptyhon -> Python review
Pull Requests
URL Status Linked Edit
PR 4517 merged python-dev, 2017-11-23 09:53
PR 11738 merged miss-islington, 2019-02-02 17:13
PR 11738 merged miss-islington, 2019-02-02 17:13
PR 11738 merged miss-islington, 2019-02-02 17:13
PR 11738 merged miss-islington, 2019-02-02 17:13
PR 11749 merged uranusjr, 2019-02-03 06:09
PR 11749 merged uranusjr, 2019-02-03 06:09
PR 11749 merged uranusjr, 2019-02-03 06:09
Messages (15)
msg55649 - (view) Author: Anthony Tuininga (atuining) * Date: 2007-09-04 22:03
Attached is a patch that fixes the truncation of the property values
returned by msilib.SummaryInfo.GetProperty(). Unfortunately Microsoft
has deemed it necessary to return the size of the string without the
null termination character but insists upon the size including it when
passing it in. Arggh!
msg111079 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-07-21 15:58
The patch looks correct, it's now a matter of unit tests.
for example, I'd test the case where the length is around 1000..
msg132912 - (view) Author: Mark Mc Mahon (markm) * Date: 2011-04-04 01:58
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.
msg132913 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-04-04 02:02
+        path, msilib.schema, "Ptyhon Tests", "product_code", "1.0", "PSF")
s/Ptyhon/Python/
msg132914 - (view) Author: Mark Mc Mahon (markm) * Date: 2011-04-04 02:38
And fix the typo... (thanks Ezio)
msg134976 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2011-05-02 14:31
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.

1. It's not clear to me that the malloc() call only occurs if the type is VT_LPSTR. But that's the only case where free() is called. I think it would be better to move the call to free() to a cleanup section at the end of the function.

2. The status is never checked for success, just for one specific failure. I think both calls to MsiSummaryInfoGetProperty should be looking for ERROR_SUCCESS.

3. I don't think VT_FILETIME is special enough for its own error message. It should just be caught with all other unknown types.

Maybe these last 3 should get their own new issue.
msg137133 - (view) Author: Mark Mc Mahon (markm) * Date: 2011-05-28 14:00
Responding to Eric's comments

1. There are only three valid property types returned by MsiInteger, String & FILETIME. (http://msdn.microsoft.com/en-us/library/aa372045%28v=VS.85%29.aspx)

2. That comment makes sense - I have entered a new issue (issue12202) for that.

3. Per 1. - there shouldn't be any other unhandled types. I have entered issue issue12201 to track adding support for FILETIMEs.
msg306502 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2017-11-19 15:06
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.
msg306796 - (view) Author: Tzu-ping Chung (uranusjr) * Date: 2017-11-23 09:59
I have created a PR #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.
msg306798 - (view) Author: Tzu-ping Chung (uranusjr) * Date: 2017-11-23 10:19
I made a shot to address the free() call. Hopefully this makes sense.
msg334752 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-02 16:33
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.
msg334755 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-02 17:13
New changeset 2de576e16d42ce43698d384d0dd46ba6cf165424 by Steve Dower (Tzu-ping Chung) in branch 'master':
bpo-1104: msilib.SummaryInfo.GetProperty() truncates the string by one character (GH-4517)
https://github.com/python/cpython/commit/2de576e16d42ce43698d384d0dd46ba6cf165424
msg334756 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-02 17:14
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.
msg334758 - (view) Author: miss-islington (miss-islington) Date: 2019-02-02 17:36
New changeset 56f84117a766d21045349f0217ce740831aef0dc by Miss Islington (bot) in branch '3.7':
bpo-1104: msilib.SummaryInfo.GetProperty() truncates the string by one character (GH-4517)
https://github.com/python/cpython/commit/56f84117a766d21045349f0217ce740831aef0dc
msg335880 - (view) Author: miss-islington (miss-islington) Date: 2019-02-19 03:06
New changeset d5409eb6c26c6bca2686762ce0fd5223bb845e8a by Miss Islington (bot) (Tzu-ping Chung) in branch '2.7':
[2.7] bpo-1104: msilib.SummaryInfo.GetProperty() truncates the string by one character (GH-4517) (GH-11749)
https://github.com/python/cpython/commit/d5409eb6c26c6bca2686762ce0fd5223bb845e8a
History
Date User Action Args
2019-02-19 03:14:43steve.dowersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-02-19 03:06:16miss-islingtonsetmessages: + msg335880
2019-02-03 06:10:15uranusjrsetstage: backport needed -> patch review
pull_requests: + pull_request11680
2019-02-03 06:09:56uranusjrsetstage: backport needed -> backport needed
pull_requests: + pull_request11679
2019-02-03 06:09:37uranusjrsetstage: backport needed -> backport needed
pull_requests: + pull_request11678
2019-02-02 17:36:52miss-islingtonsetnosy: + miss-islington
messages: + msg334758
2019-02-02 17:14:56steve.dowersetmessages: + msg334756
stage: patch review -> backport needed
2019-02-02 17:13:49miss-islingtonsetpull_requests: + pull_request11648
2019-02-02 17:13:47miss-islingtonsetpull_requests: + pull_request11647
2019-02-02 17:13:45miss-islingtonsetpull_requests: + pull_request11646
2019-02-02 17:13:44miss-islingtonsetpull_requests: + pull_request11645
2019-02-02 17:13:25steve.dowersetmessages: + msg334755
2019-02-02 16:33:43steve.dowersetassignee: loewis -> steve.dower
messages: + msg334752
versions: + Python 3.7, Python 3.8, - Python 3.4, Python 3.5
2017-11-23 10:19:11uranusjrsetmessages: + msg306798
2017-11-23 09:59:53uranusjrsetmessages: + msg306796
2017-11-23 09:53:46python-devsetpull_requests: + pull_request4454
2017-11-19 15:06:38eric.smithsetmessages: + msg306502
2017-11-19 07:05:24berker.peksagsetnosy: + berker.peksag
2017-10-07 08:41:11uranusjrsetnosy: + uranusjr
2015-05-06 07:11:33BreamoreBoysetnosy: + paul.moore, tim.golden, zach.ware, steve.dower

components: + Windows
versions: + Python 3.4, Python 3.5, - Python 3.1, Python 3.2
2011-05-28 14:00:42markmsetmessages: + msg137133
2011-05-02 14:31:37eric.smithsetnosy: + eric.smith
messages: + msg134976
2011-04-04 02:38:43markmsetfiles: + issue1104_msi_3.patch

messages: + msg132914
2011-04-04 02:02:46ezio.melottisetnosy: + ezio.melotti

messages: + msg132913
stage: test needed -> patch review
2011-04-04 01:58:42markmsetfiles: + issue1104_msi_2.patch

nosy: + markm
messages: + msg132912

keywords: + patch
2010-07-21 15:58:38amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg111079
2010-07-10 15:50:09BreamoreBoysetversions: + Python 3.1, Python 2.7, Python 3.2, - Python 2.6, Python 3.0
2009-04-07 04:04:42ajaksu2setstage: test needed
type: behavior
versions: + Python 2.6, Python 3.0, - Python 2.5
2007-09-17 08:24:13jafosetpriority: normal
2007-09-06 07:45:49loewissetassignee: loewis
nosy: + loewis
2007-09-04 22:03:41atuiningcreate