Skip to content
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

Update FileAttributes size to work across all platforms #88

Merged
merged 2 commits into from
Aug 6, 2018

Conversation

jiria
Copy link
Member

@jiria jiria commented Aug 2, 2018

FileAttributes is expected to be 64 bit wide when calling into Windows APIs, no matter the platform. This fix is ensuring that.

@lowenna
Copy link
Contributor

lowenna commented Aug 2, 2018

LGTM. @jterry75 Could you also take a look while @jstarks is out of town.

@jterry75
Copy link
Contributor

jterry75 commented Aug 2, 2018

@jiria - Are you sure? _FILE_BASIC_INFO. Seems to be that its a DWORD in the typedef.

@jiria
Copy link
Member Author

jiria commented Aug 2, 2018

@jterry75 Good catch, made me question my sanity. The pre-existing comment in the file helped. Structures are padded at the end so that the size of the structure is divisible by the size of the largest element. So in the case of 64 bit platform, sizeof(uintptr) was 8, so no extra padding was needed. On 32 bit platform, sizeof(uintptr) or sizeof(DWORD) are 4, so there is invisible 4 bytes of alignment. Since go does not observe these, the buffer size was only 36, not 40.

Having said that, I do not think this change is the best way to go about fixing this. We could add 4 bytes of dummy padding into the GO structure, the Windows code seems to be testing only the minimum size of the buffer, so it would not break 64 bit platforms. Or do you have some other thoughts on how to amend this?

@jterry75
Copy link
Contributor

jterry75 commented Aug 2, 2018

The cgo standard would be to add a uint32 pad variable after the DWORD I believe. I'm actually surprised that this works at all I would of thought it would write the attribute data into the high 4 bytes and your comparisons to the actual values would be wrong. Either way seems right to just make it say

FileAttributes uint32;
pad uint32

@jiria
Copy link
Member Author

jiria commented Aug 3, 2018

This is little endian, so it should go into the low 4 bytes; why do you believe otherwise?

Sounds good on the proposal, will update, retest and push a new iteration.

@jterry75
Copy link
Contributor

jterry75 commented Aug 6, 2018

LGTM. Did your tests with this work as expected?

@jiria
Copy link
Member Author

jiria commented Aug 6, 2018

Yes, I had mirrored these changes on top of fork of moby/moby and I can build dockerd and run it successfully (tested docker import, docker run). Please let me know if there is more tests that you would want me to run. Other than that, I think we can complete this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants