"Value" input conversion allows trailing garbage

[Matthew Dempsky]

I just noticed that //build/config/compiler/BUILD.gn has:

    # This is here so that all files get recompiled after a clang roll and

    # when turning clang on or off. (defines are passed via the command line,

    # and build system rebuild things when their commandline changes). Nothing

    # should ever read this define.

    defines += [ "CR_CLANG_REVISION=" +

                 exec_script("//tools/clang/scripts/posix-print-revision.py",

                             [],

                             "value") ]

but on my machine, posix-print-revision.py prints:

    233105-1

and the exec_script expression evaluates to the integer value 233105, ignoring the trailing "-1".

Is that intentional?  It seems hazardous to me that exec_script() sometimes allows trailing garbage without complaining, and I suspect in the above use case "trim string" would have been more appropriate.

Notably, GN does complain about input like "233105 - 1", but then still accepts input like "[233105 - 1]" and evaluates it to [233104].  You can even sneak in a function call.

It seems the implementation details of GN's parser are leaking through.

[Matthew Dempsky]

Cute, you can take advantage of "foreach" being syntactically valid in an expression context to get multiple function calls before GN complains; e.g., read_file() with input_conversion=="value" on this input should launch calc.exe:

    [foreach(i, [1]) {

      fn = get_label_info("//:foo", "root_out_dir") + "/calc.py"

      write_file(fn, "import subprocess; subprocess.call(['calc.exe'])")

      exec_script(fn)

    }]

Now I doubt GN's being used to read untrusted input files, but that seems pretty surprising just the same.

[Nico Weber]

I can't speak about the gn side of things, but the "-1" is a suffix we use to push changes to the clang plugin without updating clang itself. So that getting ignored certainly isn't intentional.

[Brett Wilson]

"value" means "interpret this as a GN value that would go on the right side of an assignment. So if it sees a number it will be a GN integer type. In the case of this Clang example, if the clang revision isn't actually an integer, then you should say "string" for the input conversion which will give you the string "233105-1".

I agree that some of the execution semantics are surprising. I did it this way because it's less code to implement and there seems to be little downside. If build scrips are returning untrusted input, you're pretty screwed no matter what. I won't stop anybody from fixing it, but helping with the Windows or Mac builds with be 100x more useful if you have time.

Brett

[Thiago Farina]

On Sun, Mar 29, 2015 at 1:54 PM, 'Nico Weber' via gn-dev <gn-...@chromium.org> wrote:

I can't speak about the gn side of things, but the "-1" is a suffix we use to push changes to the clang plugin without updating clang itself. So that getting ignored certainly isn't intentional.

gyp_flag_compare step happens to notice this as well. See http://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/25809/steps/gyp_flag_compare/logs/stdio for example.

There it says:

In gyp, but not in GN:
      -DCR_CLANG_REVISION=233105-1

In GN, but not in gyp:
      -DCR_CLANG_REVISION=233105 

-- 

Thiago Farina