I like it, thanks!
Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
31–38 of 38 posts
Re: Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
#32https://github.com/rootVIII/pdfinverter/blob/master/utils.go... using go to wrap a python script - this is some "hacker man" stuff right here
It may be possible to replace the dependency on the python script by using objective c to call the required libraries, then expose a c API that can be called from the go application. E.g. https://gist.github.com/porty/6b94f7f908ed7af0f070 https://github.com/caseymrm/menuet That said, I've seen messier things than python-in-go shipped and used reliably in production to solve business problems.
That's a fantastic idea. I actually looked into it because I really didn't want to use Python to create the output PDFs. I have experience making C bindings in Go, but had issues with Objective C (I also don't know Obj-C). If you are able to remove the Python dependency with a CGo please open a PR! Thanks for the feedback too...
Re: Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
#33https://github.com/rootVIII/pdfinverter/blob/master/utils.go... using go to wrap a python script - this is some "hacker man" stuff right here
And written to be less readable with e.g.: return bytes.ReplaceAll(script, []byte{0x09}, []byte{0x20, 0x20, 0x20, 0x20}) instead of bytes.ReplaceAll(script, []byte("\t"), []byte(" ")) Skimming through the code, there's a lot of other pretty basic mistakes. The most egregious is that the use of a python script has a security issue. Let's see if you can spot it, it's in these lines: tmp := fmt.Sprintf("/var/tmp/invertp…
Well any old working directory that exists at startup is removed. So an attacker would not be able to create the directory before the user runs the program. And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700. Also, I would rather specify the actual bytes directly and force a lazy programmer to look at the ASCII table over using []byte(" "). These are just some of my after-thoughts.
Re: Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
#34Earlier quoted context omitted.
And written to be less readable with e.g.: return bytes.ReplaceAll(script, []byte{0x09}, []byte{0x20, 0x20, 0x20, 0x20}) instead of bytes.ReplaceAll(script, []byte("\t"), []byte(" ")) Skimming through the code, there's a lot of other pretty basic mistakes. The most egregious is that the use of a python script has a security issue. Let's see if you can spot it, it's in these lines: tmp := fmt.Sprintf("/var/tmp/invertp…
Well any old working directory that exists at startup is removed. So an attacker would not be able to create the directory before the user runs the program. And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700. Also, I would rather specify the actual bytes directly and force a lazy programmer to look at the ASCII table over using []byte(" "). These are just…
> So an attacker would not be able to create the directory before the user runs the program
I said it was a race. An attacker would race with the program's deletion of the directory to recreate it. This race is really easy to do, and there have been numerous CVEs for this sort of race.
> And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700
That's not true.
If I own a directory, I can delete and recreate the files in that directory, even if I don't own the file, even if the file is 0700. Feel free to experiment around to see this.
Re: Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
#35Earlier quoted context omitted.
And written to be less readable with e.g.: return bytes.ReplaceAll(script, []byte{0x09}, []byte{0x20, 0x20, 0x20, 0x20}) instead of bytes.ReplaceAll(script, []byte("\t"), []byte(" ")) Skimming through the code, there's a lot of other pretty basic mistakes. The most egregious is that the use of a python script has a security issue. Let's see if you can spot it, it's in these lines: tmp := fmt.Sprintf("/var/tmp/invertp…
Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!
It took me a few to respond, but I'll make some recommendations now:
1. Use filepath.Glob if you want to glob
Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do.
2. ExitErr shouldn't be used, especially not within nested functions.
You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr. You should be instead doing `return nil, err` all the way back up to `err := RunApp()`, and printing the error there. That will let defers run. In general, using os.Exit anywhere other than in the main function is an antipattern.
3. Your parallel processing should be using a waitgroup or errgroup. A slice of channels is super fragile.
In [1], do something like the following:
// before the for loop
var wg sync.WaitGroup
// in each iteration of the for loop
wg.Add(1)
go func(name string) {
defer wg.Done()
cli.ImageRoutine(fileName)
}(fileName)
// at the end, to wait
wg.Wait()
The channel thing you're doing now is more fragile than a waitgroup, and harder to reason about.
4. use of exported functions / unexported functions is all over the place and inconsistent.
There's a few other things, but that's what I've got offhand. I can't actually run the code because I only use linux, and it obv doesn't run on linux. Oh, I guess that's another thing, you want some build tags to make it more clear it won't run on linux.
Hopefully something in there was helpful
[0]: https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d...
[1]: https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc7254514...
Re: Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
#36Earlier quoted context omitted.
Well any old working directory that exists at startup is removed. So an attacker would not be able to create the directory before the user runs the program. And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700. Also, I would rather specify the actual bytes directly and force a lazy programmer to look at the ASCII table over using []byte(" "). These are just…
> So an attacker would not be able to create the directory before the user runs the program I said it was a race. An attacker would race with the program's deletion of the directory to recreate it. This race is really easy to do, and there have been numerous CVEs for this sort of race. > And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700 That's not true. I…
Hmm okay. I'm going to start fixing that part and also attempt some CGO/objective-C to replace the python dependency. I had figured that removing any possible directory that could be seen as an application directory (at app-startup) would alleviate the issue you described. Thanks for the feedback though and also taking the time to look.
Re: Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
#37Earlier quoted context omitted.
Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!
It took me a few to respond, but I'll make some recommendations now: 1. Use filepath.Glob if you want to glob Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do. 2. ExitErr shouldn't be used, especially not within nested functions. You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr.…
okay have some of these fixes in progress. Thanks again for taking the time to look at my code! Very appreciative
Re: Show HN: PDF Inverter for macOS (In Golang) Darken Your PDFs
#38Earlier quoted context omitted.
Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!
It took me a few to respond, but I'll make some recommendations now: 1. Use filepath.Glob if you want to glob Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do. 2. ExitErr shouldn't be used, especially not within nested functions. You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr.…
Hi TheDong. I put most of the changes you recommended and I am much happier with the overall project. I decided to remove the Python part though and went back to the Imagick bindings. There's probably still some things to fix but thanks again for taking the time to look and advise.