msi:RECORD_StreamFromFile bug, help needed
Hi all, I think I found a msi.dll, record.c, RECORD_StreamFromFile. This function calls GlobalAlloc, with a file size as an argument. However, I believe that this can easily fail (which it does in my case) when the file size is too large. In my case, my msi builder (WiX) calls MsiRecordSetStreamW, which calls static function RECORD_StreamFromFile. WiX calls MsiRecordSetStreamW on some file that seems to be 0x11b2acc5 bytes (296 megs) large. Trying to allocate the block of 296 megs causes the error: 036:Call msi.MsiRecordSetStreamW(00000003,00000002,0cc8078c L"C:\\users\\robert\\Temp\\3c5c43d5\\1bfe39fa\\#product.cab") ret=009a4c3a 0036:Call KERNEL32.CreateFileW(0cc8078c L"C:\\users\\robert\\Temp\\3c5c43d5\\1bfe39fa\\#product.cab",80000000,00000001,00000000,00000003,00000000,00000000) ret=202868cf 0036:Ret KERNEL32.CreateFileW() retval=00000100 ret=202868cf 0036:Call KERNEL32.GetFileSize(00000100,0033ef58) ret=202868ef 0036:Ret KERNEL32.GetFileSize() retval=11b2acc5 ret=202868ef 0036:Call KERNEL32.GlobalAlloc(00000000,11b2acc5) ret=202869fb 0036:Ret KERNEL32.GlobalAlloc() retval=00000000 ret=202869fb <--- fail I believe the call to GlobalAlloc stems from RECORD_StreamFromFile. This tries to copy the full contents of the file in memory, and then create an IStream from that memory using CreateStreamOnHGlobal: /* read the data in a file into an IStream */ static UINT RECORD_StreamFromFile(LPCWSTR szFile, IStream **pstm) { ... handle = CreateFileW(szFile, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); ... sz = GetFileSize(handle, &szHighWord); if( sz != INVALID_FILE_SIZE && szHig sz = GetFileSize(handle, &szHighWord); if( sz != INVALID_FILE_SIZE && szHighWord == 0 ) { hGlob = GlobalAlloc(GMEM_FIXED, sz); // <-- Fails in my case, because sz == 296 megs if( hGlob ) { BOOL r = ReadFile(handle, hGlob, sz, &read, NULL); if( !r ) { GlobalFree(hGlob); hGlob = 0; } } CloseHandle(handle); if( !hGlob ) return ERROR_FUNCTION_FAILED; // <-- hence, this error is raised. hr = CreateStreamOnHGlobal(hGlob, TRUE, pstm); ... } I want to fix this. I guess that apart from that the allocation fails, I have the feeling that copying this entire 296 megs into main memory is anyway a bit heavy, just for creating a stream. So I thought a better solution would be just to create a stream from the file. My first guess would be to use the SHCreateStreamOnFile function: HRESULT SHCreateStreamOnFile( __in LPCTSTR pszFile, __in DWORD grfMode, __out IStream **ppstm ); But then I was thinking: this seems like such an obvious thing to do, that it is almost suspicious. Was there any reason for copying the file contents to main memory and then create a memory stream? For instance, was the intention to have a non-mutable copy in memory, in case the backing file would be altered later on? I guess if that's the case, I could just create a copy of the original file in a temp dir, and mark it as STGM_DELETEONRELEASE. Before I wildly attempt a fix though, I would appreciate it if someone had an idea here... Kind regards, Robert van Herk
On Mon, 2012-06-18 at 14:14 +0200, robert.van.herk(a)serioustoys.com wrote:
But then I was thinking: this seems like such an obvious thing to do, that it is almost suspicious. Was there any reason for copying the file contents to main memory and then create a memory stream?
For instance, was the intention to have a non-mutable copy in memory, in case the backing file would be altered later on?
Yes, the concern is that the file could be changed or removed. We should test what native does here but it probably means that we need to create a stream on the file with a sharing mode that denies writing.
I guess if that's the case, I could just create a copy of the original file in a temp dir, and mark it as STGM_DELETEONRELEASE.
It would take a long time to copy such a large file.
Dear Hans and dear group, Thanks for your input! I altered my local source tree, such that it opens the file exclusively as a stream. That seems to work. Having that fixed, at least provisionally, I am now facing another issue: my MSIs are most of the time very small, only about 5 megs. This is wrong. In some runs, it goes fine though, and they reach 300 megs, which is the right size. I find no error messages in the +relay trace... The process seems to be completely unpredictable. I can see in my Windows temp dir that the temporary files created there do seem to add up to about 300 megs, but then when the final MSI is baked, something seems to go wrong... (?) Would you have any idea of how this could be? Maybe some race-condition? It seems to be unrelated to the other bug (the one described before: GlobalAllocing a big block to copy file contents into), because before this fix this current behaviour also happened in the instances where the GlobalAlloc didn't fail. Any help would be appreciated! Kind regards, Robert ________________________________________ Van: Hans Leidekker [hans(a)codeweavers.com] Verzonden: dinsdag 19 juni 2012 9:29 Aan: robert.van.herk(a)serioustoys.com CC: wine-devel(a)winehq.org Onderwerp: Re: msi:RECORD_StreamFromFile bug, help needed On Mon, 2012-06-18 at 14:14 +0200, robert.van.herk(a)serioustoys.com wrote:
But then I was thinking: this seems like such an obvious thing to do, that it is almost suspicious. Was there any reason for copying the file contents to main memory and then create a memory stream?
For instance, was the intention to have a non-mutable copy in memory, in case the backing file would be altered later on?
Yes, the concern is that the file could be changed or removed. We should test what native does here but it probably means that we need to create a stream on the file with a sharing mode that denies writing.
I guess if that's the case, I could just create a copy of the original file in a temp dir, and mark it as STGM_DELETEONRELEASE.
It would take a long time to copy such a large file.
On Tue, 2012-06-19 at 12:36 +0200, robert.van.herk(a)serioustoys.com wrote:
I altered my local source tree, such that it opens the file exclusively as a stream. That seems to work.
Having that fixed, at least provisionally, I am now facing another issue: my MSIs are most of the time very small, only about 5 megs. This is wrong. In some runs, it goes fine though, and they reach 300 megs, which is the right size. I find no error messages in the +relay trace... The process seems to be completely unpredictable.
I can see in my Windows temp dir that the temporary files created there do seem to add up to about 300 megs, but then when the final MSI is baked, something seems to go wrong... (?)
Would you have any idea of how this could be? Maybe some race-condition?
It seems to be unrelated to the other bug (the one described before: GlobalAllocing a big block to copy file contents into), because before this fix this current behaviour also happened in the instances where the GlobalAlloc didn't fail.
Can you please open a bug report for this and attach a +msi,+msidb trace?
I am not able to reproduce the bug anymore :-S. I hope it'll pop up in the coming days again, if so, I will send you the trace :-). I'll also give the sharing thing / SHCreateFileStream a try and see if I can come up with a unit test... Regards, Robert ________________________________________ Van: Hans Leidekker [hans(a)codeweavers.com] Verzonden: dinsdag 19 juni 2012 13:04 Aan: robert.van.herk(a)serioustoys.com CC: wine-devel(a)winehq.org Onderwerp: RE: msi:RECORD_StreamFromFile bug, help needed On Tue, 2012-06-19 at 12:36 +0200, robert.van.herk(a)serioustoys.com wrote:
I altered my local source tree, such that it opens the file exclusively as a stream. That seems to work.
Having that fixed, at least provisionally, I am now facing another issue: my MSIs are most of the time very small, only about 5 megs. This is wrong. In some runs, it goes fine though, and they reach 300 megs, which is the right size. I find no error messages in the +relay trace... The process seems to be completely unpredictable.
I can see in my Windows temp dir that the temporary files created there do seem to add up to about 300 megs, but then when the final MSI is baked, something seems to go wrong... (?)
Would you have any idea of how this could be? Maybe some race-condition?
It seems to be unrelated to the other bug (the one described before: GlobalAllocing a big block to copy file contents into), because before this fix this current behaviour also happened in the instances where the GlobalAlloc didn't fail.
Can you please open a bug report for this and attach a +msi,+msidb trace?
Yes, the concern is that the file could be changed or removed. We should test what native does here but it probably means that we need to create a stream on the file with a sharing mode that denies writing.
So... I tried this out. I do not understand what I found though! On Windows, after MSI_RecordSetStreamFromFileW, you CAN call DeleteFile on that file. But you cannot open the file for writing; that will give ERROR_ACCESS_DENIED. I found in the specs of DeleteFile that this behaviour is understandable, because " The DeleteFile function marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED. " On the other hand, it says: " The DeleteFile function fails if an application attempts to delete a file that is open for normal I/O or as a memory-mapped file. " So... I figured that somehow in the MS implementation of RecordSetStreamFromFile they called SHCreateStreamOnFileEx with parameters that allowed it to be scheduled for deletion through DeleteFile, and didn't allow the file to be opened again for writing. So far so good. So I set out to find this combination of parameters and I cannot find it. I saw that for CreateFile, you can pass FILE_SHARE_DELETE, and I guess that that allows for DeleteFile to be called. However, you cannot pass flags to SHCreateStreamOnFileEx that will cause this flag to be set... I am a bit out of ideas now... Maybe SHCreateStreamOnFileEx does NOT actually create the stream, but just opens the file and stores the filename in order to create the stream later on, when it is needed? Thus, some sort of lazy initialization? Any other ideas? Regards, Robert
On Tue, 2012-06-19 at 20:55 +0200, robert.van.herk(a)serioustoys.com wrote:
Yes, the concern is that the file could be changed or removed. We should test what native does here but it probably means that we need to create a stream on the file with a sharing mode that denies writing.
So... I tried this out. I do not understand what I found though!
On Windows, after MSI_RecordSetStreamFromFileW, you CAN call DeleteFile on that file. But you cannot open the file for writing; that will give ERROR_ACCESS_DENIED.
I found in the specs of DeleteFile that this behaviour is understandable, because " The DeleteFile function marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED. "
On the other hand, it says: " The DeleteFile function fails if an application attempts to delete a file that is open for normal I/O or as a memory-mapped file. "
So... I figured that somehow in the MS implementation of RecordSetStreamFromFile they called SHCreateStreamOnFileEx with parameters that allowed it to be scheduled for deletion through DeleteFile, and didn't allow the file to be opened again for writing.
So far so good.
So I set out to find this combination of parameters and I cannot find it. I saw that for CreateFile, you can pass FILE_SHARE_DELETE, and I guess that that allows for DeleteFile to be called. However, you cannot pass flags to SHCreateStreamOnFileEx that will cause this flag to be set...
We don't have to call SHCreateStreamOnFileEx, we can add a suitable implementation to msi.
We don't have to call SHCreateStreamOnFileEx, we can add a suitable implementation to msi.
Ah, I see. I guess this stream cannot escape from MSI. The only way it can be accessed is through MsiRecordReadStream and MsiRecordSetStream, but it can never be accessed directly? In that case, we don't have to use an IStream... I could alter msipriv.h such that MSIFIELD just holds a HANDLE in case the field is a stream. The handle can then be created through CreateFile, and I guess then I could alter the permissions such that it matches the MS implementation... Do you think that that is the way to go? Or is there a deep reason why we should use IStream, and create our own IStream implementation? Regards, Robert
On Tue, 2012-06-19 at 21:45 +0200, robert.van.herk(a)serioustoys.com wrote:
We don't have to call SHCreateStreamOnFileEx, we can add a suitable implementation to msi.
Ah, I see.
I guess this stream cannot escape from MSI. The only way it can be accessed is through MsiRecordReadStream and MsiRecordSetStream, but it can never be accessed directly?
In that case, we don't have to use an IStream... I could alter msipriv.h such that MSIFIELD just holds a HANDLE in case the field is a stream. The handle can then be created through CreateFile, and I guess then I could alter the permissions such that it matches the MS implementation...
Do you think that that is the way to go?
Or is there a deep reason why we should use IStream, and create our own IStream implementation?
IStream is not exposed to the user, but an internal interface would probably end up looking a lot like IStream because we need read/write/seek operations, reference counting and the ability to create clones. So we might as well use IStream.
participants (2)
-
Hans Leidekker -
robert.van.herk＠serioustoys.com