Page MenuHomePhabricator

ecore: here comes a command line object
ClosedPublic

Authored by bu5hm4n on Dec 26 2018, 9:10 AM.

Details

Summary

the mixin for now can carry a command, which can be setted as an string.
The string is then parsed again, this is done in order to make sure that
everything that needs escaping really is escaped or parsed correctly.

Depends on D7514

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
zmike requested changes to this revision.Dec 26 2018, 11:01 AM
zmike added a subscriber: billiob.

This should have a couple things prior to landing:

  1. successful travis build results
  2. (many) more tests for command line parsing; @billiob may have some interest/ideas here since terminology likely has done fuzz testing for things like this?
This revision now requires changes to proceed.Dec 26 2018, 11:01 AM

I completly agree with you, my problem here is, i have not a lot of shell knowledge, i don't really know what needs escapting etc. what is security sensitive, the code there is just copied from efl_task.c (which is untested.)

No autotools support? :(

You may want to escape every char < 0x20 (and write it as \x07). Same for 0x7f (del) and 0xc1, 0xc2 (unicode escape codes).

bu5hm4n planned changes to this revision.Dec 27 2018, 3:24 AM

I will check it out, thx billiob :)

TODO: more tests escape correct chars

bu5hm4n updated this revision to Diff 18221.Jan 4 2019, 3:25 AM
bu5hm4n edited the summary of this revision. (Show Details)

buildsystem update

I like the design of this object as it allow for both array and string to be set with their native type. Still a few nitpick, sorry.

src/lib/ecore/efl_core_command_line.eo
58

Aren't we using yoda style and so shouldn't it be array_fill? I would actually expect a array_set.

65

I was more expecting a string_set here to.

May the review be with you.

src/lib/ecore/efl_core_command_line.eo
65

I wanted to keep it the same as the array_fill method. So after appling your review comment it would be ´string_fill´ and ´array_fill´ is that ok ?

cedric added inline comments.Jan 24 2019, 9:51 AM
src/lib/ecore/efl_core_command_line.eo
65

Actually thinking about it, wouldn't it be better if both string and array where property actually. This would mean the syntax: cmd.string = "./run.sh"; to be a syntax in other language. Same for array, we would have cmd.array = [ "./run.sh" ];. Wouldn't that be better looking actually ?

bu5hm4n added inline comments.Jan 24 2019, 11:15 AM
src/lib/ecore/efl_core_command_line.eo
65

totally agreed.

raster requested changes to this revision.EditedJan 24 2019, 11:19 AM
raster added a subscriber: raster.

as cedric said. it should be properties.

i made the original code have an append arg func so in C it was simple to deal with ownership of the strings in the array.

so the question is... who owns the strings and the array when u set it? is it duplicated and the array is property of the caller, or it becomes owned by the object? your code here is the former, so anyone building args one at a time (and not snprintf()ing them to a buffer or strcat()ing them etc. via the command_set) needs to create an array, append strdup'd strings most likely, then set the array using this class, then iterate over the array to free each string when done, then free the array.

in C this is much more code than the methods already there today, so i would say you need to find a solution to this to not make it suck in C. if that means having the clear and append methods like i did, then so be it, but find a way to make it not suck in C. what you do have here though, compared to what is already there sucks in C if you use the array interface and/or want to append one arg at a time (e.g. from a list of file paths or snprintf to a tmp buffer some args etc.). you have to allocate an array, then per argument generate or find your arg (use a const char * or snprintf to buffer when generating an arg) then strdup it, then append to array, then set array, then iterate over array freeing the tmp strdup()'d strings, then free array. the task class simply is: for each arg, generate or find your arg (use a const char * or snprintf to buffer when generating an arg), call the arg append int he class. that's it. no need to create and free array and strdup on each arg, iterate over array and free each string and then free array. yes i know your test uses const char *'s so it doesn't have to strdup or free each string. this will not be the majority of more complex uses which need to generate args on the fly like:

append("command-name");
append("-opt");
if (value_num > 0) {
  append("-thing");
  snprintf(buf, sizeof(buf), "%s-%i", value_name, value_num);
  append(buf);
}
foreach list item {
  snprintf(buf, sizeof(buf), "%s/%s", base_dir, item->file);
  append(buf);
}
run();

with your setup above the above becomes:

a = array_new();
a.append(strdup("command-name"));
a.append(strdup("-opt"));
if (value_num > 0) {
  a.append(strdup("-thing"));
  snprintf(buf, sizeof(buf), "%s-%i", value_name, value_num);
  a.append(strdup(buf));
}
foreach list item {
  snprintf(buf, sizeof(buf), "%s/%s", base_dir, item->file);
  a.append(strdup(buf));
}
run();
foreach a item {
  free(item);
}
array_free(a);

so a lot more calls, strdups, more chance of leaking memory (forgetting to free) or crashing when u don't strdup an item into the array then try free it, and forgetting to free the array too... so please find a way to make this less error prone and less footwork.

now in addition to this, where do you think this mixin is going to be used? why should it be a mixin? why should it not be part of the task class? forget for now your argument of threads should not have command arguments. is there any use for this beyond the task class (exe's, main app obj)? what else would/could potentially need this that isn't inheriting e.g. from the exe class.

This revision now requires changes to proceed.Jan 24 2019, 11:19 AM
bu5hm4n updated this revision to Diff 18845.Jan 25 2019, 1:38 AM

general code update

Your code example lacks a little bit of realism, you *can* use strdup, but nothing forces you to do so, the string you pass in are noted as "string" in eolian, which means, its just a sequence of chars terminated with \0, no assertion over the heap vs. stack. Thus your usage of strings in your example is completly missleading and not respecting the reality, this could also be seen while reading the introduced tests. The array *was* not owned before, but is now, so the user does not need to handle it. So all in all, the examples are pretty much the same, beside from generating a array object, and passing this object on. Further more, you could also check what this means for bindings. The new API is a lot bindings friendlier, you can use the native array manipulation operator from your language, so an advantange over the previous solution.

cedric accepted this revision.Jan 25 2019, 3:35 PM

This is pretty neat and a clear improvement for bindings without making the C code harder. You can forget my past comment about the need for an eina_array_clone, as using eina_slstr would be a perfect fit here.

bu5hm4n updated this revision to Diff 19014.Jan 29 2019, 8:59 AM

rebase & fixup

zmike requested changes to this revision.Jan 31 2019, 6:13 AM
zmike added inline comments.
src/lib/ecore/efl_core_command_line.c
187

This could be eina_strdup.

213

Why not just use eina_array_count(array) for the size?

214

A bold choice to use the C99 feature of declaring a variable in a loop!

217

Using alloca here for a command line input seems potentially risky considering stack limit vs ARG_MAX?

239

This should probably be eina_strdup to avoid crashing on a null string.

244

This should null the pointer to avoid using a garbage pointer later.

src/lib/ecore/efl_core_command_line.eo
5

!DOC POLICE ALERT!
setted is not a word. Also this comma should be a semicolon for the given usage and the line should be wrapped.
!DOC POLICE ALERT!

src/tests/ecore/efl_app_test_cml.c
51

I didn't see where this function was defined but it should probably be string_fill based on efl naming.

This revision now requires changes to proceed.Jan 31 2019, 6:13 AM
segfaultxavi requested changes to this revision.Jan 31 2019, 7:19 AM
segfaultxavi added inline comments.
src/lib/ecore/efl_core_command_line.eo
52

Don't you ever dare to write setted again.

68

Since command_string and command_array are no longer methods but properties, the docs need to be updated to reflect that you can set and get these values.

Also, missing docs on the values.
Also, "an array", not "a array".

bu5hm4n added inline comments.Feb 5 2019, 2:30 AM
src/lib/ecore/efl_core_command_line.eo
68

its only a setter, not a getter.

src/tests/ecore/efl_app_test_cml.c
51

Apply this patch, phabricator does display something old.

segfaultxavi accepted this revision.Feb 5 2019, 3:18 AM
segfaultxavi added inline comments.
src/lib/ecore/efl_core_command_line.eo
70

It's split, not splitted. And there is also an incorrect "A array".
Mark the inline comments as done as you fix them so you don't forget :P
But don't worry, I'll do a general orthographic review at some point...

bu5hm4n updated this revision to Diff 19193.Feb 5 2019, 5:16 AM

Make xavi happy

You even corrected mistakes I had not spotted! Well done! I am happy now 😁

bu5hm4n marked 18 inline comments as done.Feb 7 2019, 7:16 AM
zmike requested changes to this revision.Feb 11 2019, 9:08 AM
zmike added inline comments.
src/lib/ecore/efl_core_command_line.c
251

This is now a garbage pointer.

This revision now requires changes to proceed.Feb 11 2019, 9:08 AM
bu5hm4n updated this revision to Diff 19283.Feb 11 2019, 9:26 AM

setting ptr to NULL

zmike requested changes to this revision.Feb 11 2019, 9:30 AM
zmike added inline comments.
src/lib/ecore/efl_core_command_line.c
220

This leaks command.

This revision now requires changes to proceed.Feb 11 2019, 9:30 AM
zmike requested changes to this revision.Feb 11 2019, 9:44 AM
zmike added inline comments.
src/lib/ecore/efl_core_command_line.c
223

nit: space after (.

225

Now this is a garbage pointer.

This revision now requires changes to proceed.Feb 11 2019, 9:44 AM
bu5hm4n updated this revision to Diff 19286.Feb 11 2019, 9:50 AM

more fixes

zmike accepted this revision.Feb 11 2019, 9:51 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 12 2019, 2:20 AM
Closed by commit rEFL48e5684b3c37: ecore: here comes a command line object (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.

this makes the c definitely worse exactly as i described and it is not niche usage. the ownership is even mixed up between strings and the array.