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

Prevent crash on local calibre OPDS server #5572

Merged
merged 1 commit into from Nov 7, 2019
Merged

Conversation

@robert00s
Copy link
Contributor

robert00s commented Nov 6, 2019

Close: #5476

@Frenzie Frenzie added this to the 2019.11 milestone Nov 6, 2019
@Frenzie Frenzie added the bug label Nov 6, 2019
@@ -84,7 +84,7 @@ function OPDSParser:parse(text)
return s:gsub( "%p", {["&"] = "&amp;", ["<"] = "&lt;", [">"] = "&gt;" } )
end )
local xlex = luxl.new(text, #text)
return self:createFlatXTable(xlex)
return assert(self:createFlatXTable(xlex))

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 6, 2019

Member

Canned response, but is this assert protected by a pcall? ;-)

This comment has been minimized.

Copy link
@robert00s

robert00s Nov 6, 2019

Author Contributor

Do you suggest to use
assert(pcall(createFleXTable, xlex)) ?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 6, 2019

Member

I suggest whatever doesn't crash when the assertion fails, which would be the other way around (pcall(assert(). ;-) Note that we also have a debug.assert function (might be named slightly differently) which will crash the program in debug mode.

I'm not sufficiently familiar with the code to know what makes the most sense here specifically. It could also be a level higher up, like: pcall(OPDSParser.parse, text).

This comment has been minimized.

Copy link
@robert00s

robert00s Nov 7, 2019

Author Contributor

I suggest whatever doesn't crash when the assertion fails, which would be the other way around (pcall(assert(). ;-) Note that we also have a debug.assert function (might be named slightly differently) which will crash the program in debug mode.

Example please. I really don't understand :)

I'm not sufficiently familiar with the code to know what makes the most sense here specifically. It could also be a level higher up, like: pcall(OPDSParser.parse, text).

pcall(OPDSParser.parse, text) won't work. It can't enter to any opds server.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 7, 2019

Member

I mean dbg.dassert(). But that's not really appropriate here.

Dbg.dassert = function(check, msg)
assert(check, msg)
return check
end

pcall(OPDSParser.parse, text) won't work. It can't enter to any opds server.

How do you mean? Something like pcall(OPDSParser.parse, OPDSParser, text) or pcall(OPDSParser.parse, self, text), whatever works in the relevant context.

My point is simply that assert() means the program halts unless it's taken care of in some way.

> function bla() assert(false) end
> bla()
stdin:1: assertion failed!
stack traceback:
	[C]: in function 'assert'
	stdin:1: in function 'bla'
	stdin:1: in main chunk
	[C]: in ?

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 7, 2019

Contributor

I was going to ask : but why add that assert() ?! Why would youd need that?
But following the code, the only place this is called is actually wrapped in a pcall():

function OPDSBrowser:parseFeed(item_url, username, password)
local feed
local feed_last_modified = self:fetchFeed(item_url, username, password, "HEAD")
local hash = "opds|catalog|" .. item_url
if feed_last_modified then
hash = hash .. feed_last_modified
end
local cache = CatalogCache:check(hash)
if cache then
feed = cache.feed
else
logger.dbg("cache", hash)
feed = self:fetchFeed(item_url, username, password)
if feed then
CatalogCache:insert(hash, CatalogCacheItem:new{ feed = feed })
end
end
if feed then
return OPDSParser:parse(feed)
end
end
function OPDSBrowser:getCatalog(item_url, username, password)
local ok, catalog = pcall(self.parseFeed, self, item_url, username, password)
if not ok and catalog and not NetworkMgr:isOnline() then
NetworkMgr:promptWifiOn()
return
elseif not ok and catalog then
logger.info("cannot get catalog info from", item_url, catalog)
UIManager:show(InfoMessage:new{
text = T(_("Cannot get catalog info from %1"), (item_url or "")),
})
return
end
if ok and catalog then
return catalog
end
end

So, you actually added it to make that pcall() fail.
So, case closed :)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 7, 2019

Member

Excellent, so the answer to my initial question is a straightforward "yes". 👍

@Frenzie Frenzie merged commit 2ae91ac into koreader:master Nov 7, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@robert00s robert00s deleted the robert00s:opds_crash branch Nov 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.