Skip to content

Four structural cleanups worth discussing before anyone writes them #35

Description

@warpedatom

Four structural things came up repeatedly while working on #30, #31 and #33.
None is a bug and none needs fixing to ship anything, so I did not touch them.
Raising them as one issue rather than arriving with a large diff, because they
are the kind of change you should get to decide on before someone writes it.

Happy to implement any subset, none of it, or something different if you have a
shape in mind.

1. GenerateOptions takes 38 positional parameters

func GenerateOptions(stage, sleeptime, jitter, useragent, uri, customuri,
    customuriGET, customuriPOST, beacon_PE, processinject_min_alloc,
    Post_EX_Process_Name, metadata, injector, Host, Profile, ProfilePath,
    outFile, custom_cert, cert_password, CDN, CDN_Value, datajitter,
    Keylogger string, Forwarder bool, ...)

Twenty-three consecutive string parameters means transposing any two of them
compiles cleanly and silently produces a wrong profile. Adding a flag means
touching the signature, the call site, and every intermediate.

The type to pass already exists: Loader.FlagOptions is declared at the top of
Loader.go and is never referenced anywhere. Passing that struct instead would
make argument order unrepresentable as a bug class, and adding a flag would
become a one-line change.

2. num_Profile and Post are mutable package globals

GenerateProfile reads num_Profile expecting GenerateHTTPVaribles to have
already set it. That ordering is not expressed anywhere, so reordering the calls
in GenerateOptions breaks profile selection silently rather than failing to
compile. Returning the resolved profile number and passing it explicitly would
make the dependency visible.

3. Build() is nine copies of the same nine lines

Each of the nine template sections repeats parse, check error, reset buffer,
execute, check error, capture string. That is roughly 110 lines doing one thing
nine times. A slice of {name, template, data} and a loop would be about
fifteen. Relatedly, the nine types Beacon_Com, Beacon_Stage_p1 through
p3, Process_Inject, Beacon_PostEX, Beacon_GETPOST,
Beacon_GETPOST_Profile and Beacon_SSL are all identical:

type X struct {
    Variables map[string]string
}

One type used nine times would do, and the templates would not notice.

4. log.Fatal throughout makes error paths untestable

Every validation failure calls log.Fatal, which exits the process. That means
none of them can be covered by a test, because the test binary dies with them.
The tests I added in #30 and #33 cover the success paths and the pure helpers;
the error paths are covered only by running the binary by hand.

Returning errors up to main and calling log.Fatal in exactly one place would
make every validation rule testable. This is the largest of the four and the
one I would understand you declining, since it touches every function.

Why I am not sending these as PRs

Each one is mechanical but wide, and a wide diff is hard to review next to the
behavioural changes already in flight. #1 and #3 are behaviour-neutral and
mostly mechanical. #2 is small. #4 is the invasive one. If any of them appeal,
say which and I will send them one at a time, separately from the fixes, so the
diffs stay readable.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions