Page MenuHomePhabricator

evil: Fix fcntl for F_SETLK and F_SETLKW wrong length calculation
ClosedPublic

Authored by felipealmeida on Dec 9 2020, 8:04 AM.

Details

Summary

If length and start are both 0, size is wrongfully negative. Besides,
using length as a delimitator in a range means that [0, length) is a
half-closed interval, so we don't need to subtract by 1.

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.
felipealmeida created this revision.Dec 9 2020, 8:04 AM
felipealmeida requested review of this revision.Dec 9 2020, 8:04 AM

no problem for me, but i think that this function should not exist in evil at all.

$ git grep F_SETLKW src/
// strip evil code
src/modules/ecore_evas/engines/extn/ecore_evas_extn_buf.c: if (fcntl(b->lockfd, F_SETLKW, &filelock) == -1)

extn engine is not used (though compiled) on Windows, so a #ifdef F_SETLKW should be sufficient

$ git grep F_SETLK src/
// strip evil code
src/bin/efreet/efreet_desktop_cache_create.c: if (fcntl(lockfd, F_SETLK, &fl) < 0)
src/bin/efreet/efreet_icon_cache_create.c: if (fcntl(lockfd, F_SETLK, &fl) < 0)
src/bin/efreet/efreet_mime_cache_create.c: if (fcntl(lockfd, F_SETLK, &fl) < 0)
src/modules/ecore_evas/engines/extn/ecore_evas_extn_buf.c: if (fcntl(b->lockfd, F_SETLK, &filelock) == -1)
src/modules/ecore_evas/engines/extn/ecore_evas_extn_buf.c: if (fcntl(b->lockfd, F_SETLKW, &filelock) == -1)

extn engine : same as above
for efreet in bin/ I have to look

but i am really wanting to remove all or parts of fcntl in Evil

I actually hit this bug. I think it was in efreet. But I'm seeing it in ecore/efl_exe, ecore/efl_thread, efl_io_closer, ecore_signal as well this call.

Or do you mean remove the else if ((cmd == F_SETLK) || (cmd == F_SETLKW)) block but leave the rest?

what i meant is, in src/modules/ecore_evas/engines/extn/ecore_evas_extn_buf.c, line 156 :

#ifdef F_SETLKW
        if (fcntl(b->lockfd, F_SETLKW, &filelock) == -1)
          {
             ERR("lock release fail");
             return;
          }
#endif

what i meant is, in src/modules/ecore_evas/engines/extn/ecore_evas_extn_buf.c, line 156 :

#ifdef F_SETLKW
        if (fcntl(b->lockfd, F_SETLKW, &filelock) == -1)
          {
             ERR("lock release fail");
             return;
          }
#endif

OK, but effectively removing the else block from fcntl implementation, right? I don't know, I'm not sure about efreet's use. Besides, people may use fcntl down the road, breaking EFL again on Windows. Do you know if efreet's use could be removed?

i don't know about efreet yet, i've not looked at the code

in efreet, it's for locking a file, so I think the code in evil is useful

lucas accepted this revision.Dec 10 2020, 5:05 AM
This revision is now accepted and ready to land.Dec 10 2020, 5:05 AM
vtorri accepted this revision.Dec 12 2020, 11:31 PM
This revision was automatically updated to reflect the committed changes.