Crash when bookmark URL is not accessible

[leafee98]

Step to reproduce:

1. Start up shiori serve and login
2. Add a bookmark that is not accessible for shiori
   1. If the bookmark is likely i/o timeout, need to click the Update archive on the saved bookmark, the options Keep the old title and excerpt and Update archive as well is not necessary to check or uncheck.
   2. If the bookmark is likely connection refused, the shiori will crash immediately.
3. shiori crash

Log is following:

2022/08/04 08:57:14 error downloading boorkmark: error downloading bookmark: Get "http://127.0.0.1:2323": dial tcp 127.0.0.1:2323: connect: connection refused
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xaaf64a]

goroutine 68 [running]:
github.com/go-shiori/shiori/internal/webserver.(*handler).apiInsertBookmark.func1()
        /home/runner/work/shiori/shiori/internal/webserver/handler-api.go:329 +0x8a
created by github.com/go-shiori/shiori/internal/webserver.(*handler).apiInsertBookmark
        /home/runner/work/shiori/shiori/internal/webserver/handler-api.go:324 +0x5c5

The results is same when the error message is i/o timeout, read: connection reset by peer or connection refused like above.

Version

shiori_1.5.3_Linux_x86_64.tar.gz from GitHub release page.

[leafee98]

After crash and restart shiori, there is no offline save or archive for the crashed bookmark. And click Update archive will do nothing, no offline save or archive will be created, and shiori will not crash again for this one bookmark.

[hulb]

@leafee98 Does this issue still exist in the latest release? I try to reproduce it but it seems the issue is gone.

[leafee98]

I think it still exist, I'm on commit d15dc186741c8b8c37949e8b0ef6e7e6273714e8.

I build the shiori by run go get -u -v github.com/go-shiori/shiori and then go build, and get binary shiori.

After configuring database with MariaDB by environment variables like follows.

export SHIORI_DBMS=mysql
export SHIORI_MYSQL_USER=tst
export SHIORI_MYSQL_PASS=test
export SHIORI_MYSQL_NAME=test
export SHIORI_MYSQL_ADDRESS=tcp(127.0.0.1:3306)

Run ./shiori --portable migrate and ./shiori --portable serve.

Then login the web UI and type an inaccessible URL like http://localhost:2323, and I got the final crash log.

INFO[0053] POST /api/bookmarks                           proto=HTTP/1.1 remote="127.0.0.1:50046" reqlen=97 size=223 status=200
2022/10/09 22:18:54 error downloading boorkmark: error downloading bookmark: Get "http://localhost:2323": dial tcp [::1]:2323: connect: connection refused
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xaf85b7]

goroutine 374 [running]:
github.com/go-shiori/shiori/internal/webserver.(*handler).apiInsertBookmark.func1()
        /home/leafee98/Desktop/project/shiori/internal/webserver/handler-api.go:342 +0x97
created by github.com/go-shiori/shiori/internal/webserver.(*handler).apiInsertBookmark
        /home/leafee98/Desktop/project/shiori/internal/webserver/handler-api.go:337 +0x62e

If there are any wrong in my building steps or configuration or usage, let me know.

[hulb]

@leafee98 thx, I see. There is an issue there. But it may not causes the main process crash and exit, it should only crash the goroutine.It's nothing to do with your configuration. You could simply add a return after internal/webserver/handler-api.go:340 to fix it. But yes, somewhere nearby the same issue will cause the main process crash.

[fmartingr]

There's an insane abuse of panic through all the code, so most of things that can fail just panics. For CLI users it may be ok depending on what it is, but for server users mean a restart of the service, losing the session, etc etc. Error handling and logging requires huge improvements.

[hulb]

There's an insane abuse of panic through all the code, so most of things that can fail just panics. For CLI users it may be ok depending on what it is, but for server users mean a restart of the service, losing the session, etc etc. Error handling and logging requires huge improvements.

agree. Panic should be more careful. At present, the web controller layer uses panic to throw out an err and handles it in a recover middlerware. I think it's quite normal, since the err handling in go is annoying.

[hulb]

Maybe a more elegant way is to implement a fail(w *http.Response, err error) function to handle all expected server err and return a more friendly response.