Page MenuHomePhabricator

ecore-file: fix ecore_file_can_exec() on Windows
ClosedPublic

Authored by vtorri on Aug 27 2020, 12:12 AM.

Details

Summary

on Windows access() has no support of X_OK (see https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/access-waccess?view=vs-2019), and SHGetFileInfo() only test the extension. So :

  1. we parse the PE file (EXE or DLL) and we verify that's only a EXE
  2. for a .bat file (technically speaking, it's an executable, equivalent of a shell script), it's a text file so we can only test if the extension is ".bat"
Test Plan

test program

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vtorri created this revision.Aug 27 2020, 12:12 AM
vtorri requested review of this revision.Aug 27 2020, 12:12 AM
jptiz added a reviewer: lucas.Aug 27 2020, 4:18 AM
jptiz accepted this revision.Aug 27 2020, 5:40 AM

This patch makes sense for me. Also, since you looked into this part of EFL, I would like to point out: apparently Windows considers only .exe files to be executables (neither .bat or .ps1 or any other file is), so even with SHGetFileInfo, tests like this will fail (as it does even with this patch). I'll open a phab task for this later.

This revision is now accepted and ready to land.Aug 27 2020, 5:40 AM
lucas added a comment.Aug 27 2020, 5:54 AM

I may be wrong, but SHGetFileInfo will only say that a file is executable if it ends with .exe, it won't consider .bat, .msi, for example (don't know if it is a problem, anyway).

But by using SHGetFileInfo (or even GetBinaryTypeA) it would be good to change ecore_test_ecore_file to create files with .exe when testing for ecore_file_can_exec == EINA_TRUE.

strange because i've tested on a .bat explicitely. Here the problem is a .sh which is not supported outside a unix shell

also see https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shgetfileinfoa

the 'return value' section.

I test only if the result is 0 or not.

jptiz added a comment.Aug 27 2020, 5:58 AM

strange because i've tested on a .bat explicitely. Here the problem is a .sh which is not supported outside a unix shell

Tested renaming test.sh to test.bat (in both exe and exe_cmd variables) and still have the same issue.

jptiz added a comment.Aug 27 2020, 6:00 AM

also see https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shgetfileinfoa

the 'return value' section.

I test only if the result is 0 or not.

Ooooh right. So in that case, it would be necessary to split Hi and Lo bits and compare them separately?

also see https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shgetfileinfoa

the 'return value' section.

I test only if the result is 0 or not.

Ooooh right. So in that case, it would be necessary to split Hi and Lo bits and compare them separately?

why ?

if it's != 0, then I consider (according to the doc) Windows application, MS-DOS .exe or .com files and Console application or .bat file

so all of them

jptiz added a comment.Aug 27 2020, 6:08 AM

also see https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shgetfileinfoa

the 'return value' section.

I test only if the result is 0 or not.

Ooooh right. So in that case, it would be necessary to split Hi and Lo bits and compare them separately?

why ?

if it's != 0, then I consider (according to the doc) Windows application, MS-DOS .exe or .com files and Console application or .bat file

so all of them

Oh wow, sorry, my head just flipped the 0-return meaning.

Hm, well, just renaming the file to .bat didn't solve the issue. Maybe it is something related to being created as a binary file (dunno if Windows expects it to have some kind of text file flag?).

another solution : consider only PE binaries and parse the file (i've alredy done that), and check if it's a binary or not

lucas added a comment.EditedAug 27 2020, 6:35 AM

another solution : consider only PE binaries and parse the file (i've alredy done that), and check if it's a binary or not

Seems good to me.

But test will need changes anyway, right?

i renamed a .exe into a .isexe and SHGetFileInfo() fails, so it's really the extension which matters here

i'll try to write something for PE files only. For .bat, i don't know what to do.

But test will need changes anyway, right?

for .sh ?

lucas added a comment.Aug 27 2020, 7:14 AM

But test will need changes anyway, right?

for .sh ?

I mean to generate files with .exe for the tests of ecore_file_can_exec should return EINA_TRUE, but if you are going to use PE it won't be needed.

vtorri updated this revision to Diff 31069.Aug 27 2020, 10:51 PM

SHGetFileInfo() look at the file extension only. Parse file to see if it is an executable or not instead

jptiz accepted this revision.Aug 30 2020, 5:13 AM
vtorri updated this revision to Diff 31072.Aug 30 2020, 5:40 AM

add .bat files support

stefan_schmidt added inline comments.
src/lib/ecore_file/ecore_file.c
657

What does 0x5a4d mean here? Same for 0x3c a few lines later and 0x00004550 and the 22 offset?

This all looks really like magic and as if nobody would be able to understand and debug this later one?

Is there no better way to do this?

vtorri updated this revision to Diff 31076.Aug 31 2020, 4:55 AM

add comments decribing the parsing of a PE file

vtorri edited the summary of this revision. (Show Details)Aug 31 2020, 5:08 AM
stefan_schmidt accepted this revision.Aug 31 2020, 5:08 AM
This revision was automatically updated to reflect the committed changes.