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

Compiled binary includes full path to internal nim files #11572

Closed
daniel-j opened this issue Jun 24, 2019 · 17 comments

Comments

@daniel-j
Copy link

commented Jun 24, 2019

Compiling any program causes a long filesystem path outside my project to be included in the final binary. I have tried the following flags and it still gets included:
-d:release --listFullPaths:off --excessiveStackTrace:off --opt:size

Example

echo "Hello World!"

$ nim c -d:release --listFullPaths:off --excessiveStackTrace:off --opt:size helloworld.nim

Current Output

$ strings helloworld | grep toolchains
../../../home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/fatal.nim

Expected Output

(empty)

Possible Solution

Additional Information

@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Does this result in unexpected behavior if the linked files are no longer available?

@daniel-j

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

No, it's probably just for debugging. I just don't want my username etc to be included in the final binary.

@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Does it do the same thing when you compile with the release flag? If not, then that should be an easy feature/patch to implement.

@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Sorry, I misread your issue. I'll have a look when I next can. Exited to start contributing!

@daniel-j

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

Thanks!

@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Is anyone able to test this on a not arch-based distro?

@dom96

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

I doubt this is arch-specific. Yeah, we need a way to prevent these paths making their way into the binary.

@dom96

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

This should indeed be a fairly easy PR, probably just need to check one of these flags in a specific place. If fatal.nim is the only filename making its way in there then it might be even easier.

@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

I'm looking at the code in compiler/msgs.nim that is responsible for checking the flags and deciding what file path to use. Maybe it is an edgecase in the lines that process the full path? I'll be freed up in about an hour, so I'll start troubleshooting then.

@daniel-j

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

On the project I'm working on I get this in debug:

/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/io.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/iterators.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/alloc.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/gc_common.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/avltree.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/gc.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/osalloc.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/mmdisp.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/cellsets.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/strmantle.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/fatal.nim

If I add -d:release I get the following:

/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/fatal.nim

--listFullPaths:off turns some of the paths into relative (debug):

/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/io.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/iterators.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/alloc.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/gc_common.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/avltree.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/gc.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/osalloc.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/mmdisp.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/cellsets.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/strmantle.nim
/home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/fatal.nim
../../../../home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/fatal.nim
../../../../home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/gc.nim

Both -d:release and --listFullPaths:off:

../../../../home/djazz/.choosenim/toolchains/nim-0.20.0/lib/system/fatal.nim
@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

The problem here is on 210 of msgs.nim:
result = if relPath.count("..") > 2: absPath else: relPath
If the number of directories back the file is (relatively), is more than two, then it will compile with the full and absolute path. I personally, can't see a reason for this, so I'll make a pull request removing it.

@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

F-ed that one up because of a stupid mistake. My bad; trying again now...

@juancarlospaco

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Can --opt:size make those empty string
Or any other flag to not even include the relative paths...

@dizzyliam

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

That might be a good idea.

@SolitudeSF

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

how is that a good idea? --opt:size has nothing to do with that. size of the binary is completely unrelated to leaking build paths.

@juancarlospaco

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

I agree with @SolitudeSF
But I dont see other flag, unless we change the existing ones, what about:

  • --listFullPaths:off ➡️ Empty string everywhere
  • --listFullPaths:relative ➡️ Relative path everywhere
  • --listFullPaths:on➡️ Full path everywhere
@alaviss

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

--listFullPaths is a switch for compiler messages. Previously #9766 accidentally make this switch affects codegen, which I've fixed in #11583.

The problem now only resides in how the compiler use these paths. Paths of modules outside the project being compiled used during codegen is always relative, but can leak easily with ../../home/<snip>/system.nim.

A way to "fix" this is to apply extractFilename() on paths outside of the current project, or we do some magic to shorten them to <project name>/module.nim.

I think to remove all paths --stacktrace:off can be extended to do so (it's odd why that flag doesn't get rid of stack trace altogether).

alaviss added a commit to alaviss/Nim that referenced this issue Jun 29, 2019

ccgstmt: use quotedFilename() for raiseExceptionEx
This is the same proc used for stacktrace when --stacktrace:on

Fixes nim-lang#11572

@Araq Araq closed this in #11621 Jun 30, 2019

Araq added a commit that referenced this issue Jun 30, 2019

msgs, ccgstmts: fixes #11572 (#11621)
* [refactor] msgs: toFilename now return just the filename

The C codegen uses just the file name for stacktrace when
excessiveStackTrace is off (see quotedName),
so there aren't any reason for other codegen to not do the same.

The file name is now cached in TFileInfo.shortName, which was introduced
for nimsuggest, and went unused after several refactoring of the
compiler.

A toProjPath() proc has been added for the previous behavior of
toFilename().

* ccgstmt: use quotedFilename() for raiseExceptionEx

This is the same proc used for stacktrace when --stacktrace:on

Fixes #11572

* msgs: handle case where file name is not available

narimiran added a commit that referenced this issue Jul 2, 2019

msgs, ccgstmts: fixes #11572 (#11621)
* [refactor] msgs: toFilename now return just the filename

The C codegen uses just the file name for stacktrace when
excessiveStackTrace is off (see quotedName),
so there aren't any reason for other codegen to not do the same.

The file name is now cached in TFileInfo.shortName, which was introduced
for nimsuggest, and went unused after several refactoring of the
compiler.

A toProjPath() proc has been added for the previous behavior of
toFilename().

* ccgstmt: use quotedFilename() for raiseExceptionEx

This is the same proc used for stacktrace when --stacktrace:on

Fixes #11572

* msgs: handle case where file name is not available

(cherry picked from commit e259f80)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.