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

Fix ld "multiple definition" errors #4258

Merged
merged 1 commit into from
May 4, 2020

Conversation

claudioandre-br
Copy link
Member

First attempt; I'm still confused by how it was working before.

@solardiz
Copy link
Member

solardiz commented May 3, 2020

Thanks Claudio. I'm also confused by how some of this worked before - must have been luck.

That said, I think some of your changes here are wrong. The arrays used by Sayantan's bitslice DES code should probably be shared and only initialized once per format, not separate in each of the several plug files that the same format uses. So I think they shouldn't be static, but rather should be extern in some .h file and defined (non-static) and initialized in a .c file.

@claudioandre-br
Copy link
Member Author

claudioandre-br commented May 3, 2020

So I think they shouldn't be static, but rather should be extern in some .h file and defined (non-static) and initialized in a .c file.

Reverted to what it used to be, but now using extern in the .h.


Running my CI session now.

@solardiz
Copy link
Member

solardiz commented May 3, 2020

Now this looks sane for opencl_DES*, but wrong for opencl_lm*.

There should be just one shared opencl_lm_index768, defined in one .c file, and declared extern in the .h file. opencl_lm_plug.c writes it, opencl_lm_b_plug.c reads it. Perhaps it's cleaner to define it in the writer.

@solardiz
Copy link
Member

solardiz commented May 3, 2020

opencl_lm_u should also be handled the same as I suggested for opencl_lm_index768 above.

Why did this pass CI? Are we not testing lm-opencl there? I'd expect this format's test to fail with this PR's commits so far.

@claudioandre-br
Copy link
Member Author

claudioandre-br commented May 3, 2020

Why did this pass CI? Are we not testing lm-opencl there?

Yes, we are (AMD OpenCL and Mac). LM variables do not need to be global, they can be local (my conclusion).

Testing: LM-opencl [DES BS OpenCL]... PASS

@solardiz
Copy link
Member

solardiz commented May 3, 2020

LM variables do not need to be global, they can be local (my conclusion).

My conclusion from source code grepping is different. For example, opencl_lm_b_plug.c only reads opencl_lm_u, never writing it. If you provide a separate copy of opencl_lm_u in that file, it will always be reading all-zeroes from there. This doesn't make sense.

@solardiz
Copy link
Member

solardiz commented May 3, 2020

I expect -format=lm-opencl -test -mask will fail with this PR's current code.

@claudioandre-br
Copy link
Member Author

I'll make it global. BTW:

form=lm-opencl                    guesses: 3000 0:00:00:02 DONE  [PASSED]
.pot CHK:lm-opencl                guesses: 1500 0:00:00:00 DONE  [PASSED] (1500 val-pwd)

When you start working on 2.0, we can add some rules about:

  • (add) testing (real testing) provided by the format author if he/she wants to get the code merged;
  • IMO, 2.0 can start as a lean binary since 1.* series will be still available as a stable version;
  • at least, we can move all "needing love" formats to an specific sub-directory.

@magnumripper
Copy link
Member

magnumripper commented May 3, 2020

When you start working on 2.0, we can add some rules about:

  • (add) testing (real testing) provided by the format author if he/she wants to get the code merged;
  • IMO, 2.0 can start as a lean binary since 1.* series will be still available as a stable version;
  • at least, we can move all "needing love" formats to an specific sub-directory.

We should discuss JtR 2.0 on john-dev ML, but anyway: I'm getting more and more tempted to "start over" from scratch. Heavily re-using existing code, but with extreme care. And not adding a single line of code until defining lots of things, like code style (tho' actually the code style could be not to have one), plug-in interface (we could have mode plugs as well), source tree structure and so on.

As for features:

  • Candidate strings should be UTF-32 and/or UTF-8-32 all the way from generation to set_key(). Perhaps also (in lots of places but not everywhere) use a "pascal string" struct for strings in many parts of core.
  • The use of threads (OpenMP or not) should be carefully planned so modes can use it.
  • All formats are plug-ins.
  • All modes are plug-ins.
  • A LOT better shared-code support for OpenCL/FPGA. This is tricky but will give a fantastic pay-off. An OpenCL format plugin should ideally not contain a single line of OpenCL code. Maybe we should also ditch the idea of having separate source files for CPU/OpenCL/ZTEX and so on - instead use a single one, calling abstraction layers in core for the non-CPU stuff.
  • I came up with this idea recently but am not sure it with fly: We could try making some compatibility layer for using legacy plugin formats as-is. That way we con't need to upgrade all 321 plugins at once (we might even opt to never fix some of the most obscure).
  • Ditch OpenSSL in favor of own code that can be more or less optimised (first prio is merely to have it working at all).
  • Re-write dynamic mode from scratch (using only the "dynamic compiler" mode, nothing else) and add OpenCL support for it.

@solardiz
Copy link
Member

solardiz commented May 3, 2020

All good thoughts, Claudio and magnum!

However, I think 2.0 should be nothing more than a stabilized release of the 1.9.x series, without major changes. Out of the changes listed, we should only do "ditch OpenSSL" and maybe "all formats are plug-ins" before 2.0. I'd also consider merging Johnny, but Shinnok brought up some bad news about that possibility (in private e-mail).

We can try and start over for 3.0, but I am not sure if we actually have the willpower to bring it to a new release.

Also, remove a couple of unused variables.
@claudioandre-br
Copy link
Member Author

I think 2.0 should be nothing more than a stabilized release of the 1.9.x series

I think 1.9 is the stabilized series. Good enough (TM), at least.

We can try and start over for 3.0, but I am not sure if we actually have the willpower to bring it to a new release.

Probably not. But, is there any other open, good for CPU, complete, with hundred of formats, with "dynamic", for Linux, Windows, BSD, ..., tool out there? If not, you should start thinking about how to build the future of JtR.

  • JtR can have (it has) a Windows binary that can be used from SSE2 to AVX512BW. With or without OpenMP.
  • JtR can have (it has) a package for PI.
    • It works in the "edge computing" (just in case anyone thinks it should).
  • JtR works (we test it) on BSD 11 and 12.
  • JtR works (we test it) on Mac.
  • ...

Copy link
Member

@solardiz solardiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this looks to me like it should work. Please make sure -format=lm-opencl -test -mask works. Then it's OK to merge. Thanks!

@solardiz
Copy link
Member

solardiz commented May 4, 2020

you should start thinking about how to build the future of JtR.

Sure. I recognize that there's value in the kind of functionality we have in JtR, and that it's in need of a partial redesign/rewrite. I've been thinking about its future for years.

@claudioandre-br claudioandre-br merged commit 4442241 into openwall:bleeding-jumbo May 4, 2020
@claudioandre-br claudioandre-br deleted the ld branch May 4, 2020 12:41
@its5Q
Copy link

its5Q commented May 21, 2020

You might want to look into ztex formats, as there are similar errors there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants