Skip to content

Instantly share code, notes, and snippets.

Show Gist options
  • Select an option

  • Save al-maisan/da6fa2a8fe5fe82fe4862ba0fc5058d8 to your computer and use it in GitHub Desktop.

Select an option

Save al-maisan/da6fa2a8fe5fe82fe4862ba0fc5058d8 to your computer and use it in GitHub Desktop.
Code Analysis — Round 2

Code Analysis — Round 2

CRITICAL — Logic Bugs That Produce Wrong Results

1. Daily login reward off-by-one

File: src/mission/types/daily_login.rs:102-121

try_progress() mutates self.day (increments it) before returning. Then current_reward() does self.day.saturating_sub(1) to index into REWARDS. Result: the reward is always one day ahead. A user continuing a streak on day 1 gets the day-2 reward (10 coins instead of 5). On day 7, REWARDS[7] is out of bounds, hitting the unwrap_or_else fallback that returns REWARDS[0] (5 coins) instead of the intended 100-coin max reward.


2. ContextValue::from maps 0.0 to f64::MAX

File: src/notifications/types/context/mod.rs:27-29

let n = if n.is_normal() { n } else { f64::MAX };

f64::is_normal() returns false for 0.0. Any notification context variable with value zero is silently replaced with f64::MAX. Conditions like games_played == 0 (e.g., "send reminder if user has never played") evaluate as f64::MAX == 0 = false, suppressing the notification. Should use n.is_finite() instead.


3. get_transition_time_abs returns relative duration for non-Open states

File: src/db/types.rs:273-281

For Processing/Processed states, returns max(start + 60 - now, 0) — a relative duration in seconds, not an absolute timestamp. The function name says _abs and the Open branch correctly returns an absolute timestamp. Clients using this value as an absolute time will interpret near-zero values as Unix epoch (1970).


4. Job queue retry delay overflows on first attempt

File: src/services/job_queue.rs:280-282

let delay_secs = RETRY_BASE_DELAY_SECS * 2_i64.saturating_pow((attempt - 1) as u32);

When attempt = 0 (first failure), (0 - 1) as u32 wraps to 4294967295. 2_i64.saturating_pow(u32::MAX) saturates to i64::MAX. The multiplication 15 * i64::MAX overflows — panic in debug, wrapping to negative in release. The .min(300) guard runs after the overflow. In release, the negative value means immediate re-retry instead of a 15-second backoff.


5. Season enrollment race condition (TOCTOU)

File: src/http/api/protected/game_report.rs:125-206

user_season_enrollment runs entirely outside the game-report advisory lock and transaction. The check-then-enroll sequence (user_has_groupenroll_user_to_season) is not atomic. Two concurrent game reports from the same user can both pass the check and both enroll, producing duplicate ranking group assignments or constraint violations. insert_group and assign_player_to_group are also separate autocommitted operations with no encompassing transaction.


6. assign_player_to_group missing ON CONFLICT

File: src/db/rankings.rs:141-170

The INSERT INTO ranking_group_assignments has no ON CONFLICT clause. Combined with the race in issue 5, concurrent enrollment attempts will fail with a unique constraint violation surfaced as a 400 to the user.


HIGH — Panics That Crash the Server

7. testing_cats and testing_questions — bare .unwrap() on DB results

File: src/http/api/protected/mod.rs:439, 447

Both test endpoints call .unwrap() on DB query results. Any DB error panics the handler. These are exposed in the production route table.


8. utils::get_timestamp() panics on NTP clock skew

File: src/utils.rs:89-113

SystemTime::now().duration_since(UNIX_EPOCH).unwrap() panics if the system clock goes backward by even 1 nanosecond. This is a real risk in containerized/cloud environments during NTP adjustments. The function is called in every hot path (missions, rewards, sessions).


9. RwLock::unwrap() in session store — lock poisoning kills all auth

File: src/http/session.rs:35, 39-43, 51, 55

Every RwLock acquisition calls .unwrap(). If any task panics while holding the lock, the lock is permanently poisoned and all subsequent auth calls panic. One concurrent panic → server-wide auth failure until restart.


10. usize underflow in category_unlock::get_progress

File: src/mission/types/category_unlock.rs:58-62

progress.level - 1 and progress.score - previous_levelup_score are usize subtractions. If level == 0 or score < previous_levelup_score (data corruption), this panics in debug or wraps to usize::MAX in release, producing nonsense progress values.


11. .unwrap() on CredentialString::new(token) after committed registration tx

File: src/http/api/public/reg.rs:102

If mk_dev_token() produces an invalid token, this panics after the registration transaction is already committed. The user is registered in the DB but receives no credential — permanently locked out.


12. Double .unwrap() in notification template store

File: src/notifications/templates.rs:269

CONTENT.get().unwrap().read().unwrap() panics if called before template initialization or if the RwLock is poisoned from a failed template sync.


13. .unwrap() on game score maps in submit_leg

File: src/http/api/protected/async_pvp/mod.rs:496-498

.get(&user_id).unwrap().get(&category.category_id).unwrap() — panics if the game state is corrupted or unexpected.


MEDIUM — Data Consistency / Correctness

14. Wrong UUID logged in challenge_friend error

File: src/http/api/protected/async_pvp/mod.rs:834-839

Error message says "for target" but logs user_id instead of friend_id. Copy-paste error from the block above. Wrong user in logs makes debugging impossible.


15. last_online set to created_at in accept_request

File: src/http/api/protected/friends.rs:378-384

The requestor_perspective struct sets last_online: friendship.created_at instead of the actual last-online timestamp. The notification sent to the challenger contains wrong last_online data.


16. insert_daily_batch casts i64 to int4 — truncation after 2038

File: src/db/missions.rs:197-209

$3::int4[] casts 64-bit expires_at timestamps to 32-bit integers. Truncation or runtime error for any expiry beyond January 19, 2038. i64::MAX is used as a sentinel elsewhere in the codebase, making this actively dangerous.


17. update_game does not persist elo field

File: src/db/async_pvp/mod.rs:360-374

The Game struct has an elo: i32 field that is written on insert but not included in the UPDATE SET clause. Any elo changes after game creation are silently lost.


18. rewards::insert_batch uses json[] for reward_data but column is jsonb

File: src/db/rewards/mod.rs:142-178

The batch insert casts reward_data as $5::json[] while single-row insert and reads use jsonb. Inconsistent type casting may cause implicit conversion issues or skip jsonb normalization.


19. Notification ArithmeticOperator::Div has no divide-by-zero guard

File: src/notifications/types/condition/operators.rs:231-255

a / b with f64 produces Infinity or NaN when b == 0. These propagate silently through comparisons, causing notification conditions to be always-true (Inf > 5) or always-false (NaN > 5). A misconfigured template silently spams or suppresses all notifications.


20. add_with_code logic inversion: inactive friendships go through request flow

File: src/http/api/protected/friends.rs:153-227

When a prior friendship exists but is inactive (previously unfriended), the code sends a friend request. When there is no prior record at all, the code directly adds the friend without a request. The inactive friendship row is never updated/deleted, creating orphaned data.


21. get_info_for_source: one handler error discards all valid rewards

File: src/reward/mod.rs:58-74

If any one of N parallel reward info tasks fails, all successfully computed RewardInfo results are discarded and the entire call returns an error. A single corrupt reward blocks the user from seeing any of their pending rewards.


22. Username length validation uses byte length, not character count

File: src/http/api/protected/user.rs:37-39

String::len() returns bytes, not characters. Multi-byte Unicode usernames get incorrect validation (rejected at fewer visible chars than the limit, or accepted at more).


LOW — Operational / Maintainability

23. BAD_REQUEST used for server errors (pervasive)

Nearly every handler maps DB errors, transaction failures, and AWS errors to StatusCode::BAD_REQUEST (400) instead of INTERNAL_SERVER_ERROR (500). This misleads clients and monitoring systems.


24. FOR UPDATE functions accept impl PgExecutor — lock never held

Files: src/db/users.rs:123-136, 195-237, src/db/mod.rs:132-149

Locking query functions accept impl PgExecutor<'_> instead of requiring &mut PgConnection. When called with a pool connection, the FOR UPDATE lock is acquired and immediately released. The function signatures don't enforce transactional usage.


25. reap_stale background jobs: two UPDATEs not in a transaction

File: src/db/background_jobs.rs:120-155

The retry and abandon updates run on separate connections. A job could be claimed between the two, causing inconsistent reap counts or bypassing the max-attempts guard.


26. N+1 query in season reward processing

File: src/season/mod.rs:351-357

Per-group score fetch inside a loop: 200 queries per batch. Could be a single query grouped by ranking_group_id for all groups in the batch.


27. DDL uses format! string interpolation

File: src/db/partitions.rs:8-88

All DDL statements use format! with season_id: i32. Safe because i32 can't inject SQL, but the pattern is a maintainability risk if ever copied with a string parameter.


28. Hardcoded category ID 10 in SQL

File: src/db/categories.rs:67, 112

Business-logic constant embedded directly in SQL strings. Should use the existing CATEGORY_REWARD_ID constant pattern.


29. Dead code: insert_groups and insert_group_assignments

File: src/db/rankings.rs:10-78

Neither function is called anywhere. Dead code with dynamic SQL string building.


30. get_next_season() returns SeasonState::Processing instead of Open

File: src/db/types.rs:239-246

The returned struct has the wrong state. The caller ignores the field and hardcodes Open, but it's a trap for future callers.


31. Analytics body size check is dead code

File: src/http/api/protected/mod.rs:252-259

Handler checks for 5 MB body, but a global DefaultBodyLimit::max(2 MB) rejects oversized bodies before the handler runs.


32. Push token register/update returns 200 OK for invalid token format

File: src/http/api/protected/push_token.rs:13-19, 56-61

Bad tokens are silently accepted with a 200 response. Clients have no feedback that their token was rejected.


33. Daily login progress meter shows time-of-day, not mission completion

File: src/mission/types/daily_login.rs:42-52

The progress percentage is (seconds since midnight UTC) / 86400 * 100. This represents "how far through today we are" — not mission completion. A user who already claimed their daily login still sees a moving progress bar.

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