fix nil pointer deref for ErrAccountRepoNotFound - #1142
Conversation
this change just makes sure `repo` is not nil before we try to pull a `Rev` out of it. assigning the Rev from the repo follows what's *almost* a boilerplate golang error handler, but is sneakily not handling relay.ErrAccountRepoNotFound -- seems the intent is to provide a happy-path response for a common error case. unfortunately the Rev assignment tried to dereference that unconditionally, so it panicked in the allowed-error path.
|
i wonder if the relay should actually just fail the request if the eg. listRepos fails in the same scenario, and gives a response with
https://relay3.fr.hose.cam/xrpc/com.atproto.sync.listRepos?limit=1000&cursor=14489 see it ^^ (bsky relays have the message hidden because of rainbow) (but also listRepos proabably shouldn't fail the whole request, i'm working on that separately) |
|
ok, tested and confirmed this fix at relay3.fire.hose.cam
it seems like there is another underlying problem here affecting the relays that makes all indigo/cmd/relay relays have trouble with that DID, but i haven't investigated that yet. the upstream pds (matsutake) has a in any case, at least it doesn't panic with this, but it also doesn't indicate that there's any problem. if the relay should fail the request instead, i can follow up with a change to do that instead. |
bnewbold
left a comment
bnewbold
left a comment
There was a problem hiding this comment.
thanks! sloppy on my part, though feels like static analysis could have caught this one 🤔
I checked and the rev field is optional under the getRepoStatus lexicon; the text says:
Optional field, the current rev of the repo, if active=true
So in theory if active=false we maybe shouldn't return the rev. I'm not sure if we should update the schema/description to have have that behavior, or update the code to not return the rev in that case (could also skip looking up repo status if active=false).
listRepos is different because rev is required in the response. that should probably be changed in the spec... it breaks the lexicon evolution rules but is the kind of change I think we should still consider as the overall protocol is not 100% baked/stabilized yet.
Regardless, I think this PR is a strict improvement on the current situation so going to merge.
|
as a note, hands are full right now and it will probably be a while before this gets deployed to our relay instances. created an issue with follow-ups: #1143 |
|
edit: moved to the new issue |
…unt_repo (#1144) Accounts sometimes don't have an entry in their account_repo extension table. This change handles that case for listRepos by including them in the response but with empty strings for the "Rev" and "Head" fields. ...which probably isn't ultimately correct, but is at least more usable for clients than missing an entire page of repos because one of them was affected by this. placeholder improvement directly related to #1143 basically the same as #1142 but that PR only addressed getRepoStatus --- for what it's worth, the sql join `TODO` suggested in the code here would have hidden (fixed?) this problem by omitting the affected accounts, assuming inner-join
this change just makes sure `repo` is not nil before we try to pull a `Rev` out of it. assigning the Rev from the repo follows what's *almost* a boilerplate golang error handler, but is sneakily not handling `relay.ErrAccountRepoNotFound` -- seems the intent is to provide a happy-path response for a common error case. unfortunately the Rev assignment tried to dereference that from `repo` unconditionally, so it panicked in the allowed-error path. (still need to test and will verify on my relays)
…unt_repo (#1144) Accounts sometimes don't have an entry in their account_repo extension table. This change handles that case for listRepos by including them in the response but with empty strings for the "Rev" and "Head" fields. ...which probably isn't ultimately correct, but is at least more usable for clients than missing an entire page of repos because one of them was affected by this. placeholder improvement directly related to #1143 basically the same as #1142 but that PR only addressed getRepoStatus --- for what it's worth, the sql join `TODO` suggested in the code here would have hidden (fixed?) this problem by omitting the affected accounts, assuming inner-join
2 participants
this change just makes sure
repois not nil before we try to pull aRevout of it.assigning the Rev from the repo follows what's almost a boilerplate golang error handler, but is sneakily not handling
relay.ErrAccountRepoNotFound-- seems the intent is to provide a happy-path response for a common error case.unfortunately the Rev assignment tried to dereference that from
repounconditionally, so it panicked in the allowed-error path.(still need to test and will verify on my relays)