Opened 6 weeks ago

Last modified 5 weeks ago

#31571 merge_ready defect

Add the tor version and a newline to raw_assert()

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: consider-backport-after-042-stable, BugSmashFund, diagnostics, android, dgoulet-merge, macos 035-backport 040-backport 041-backport
Cc: Actual Points: 0.3
Parent ID: Points: 0.1
Reviewer: nickm Sponsor:

Description

We're missing a newline and the tor version in the logs for #31570.

Child Tickets

Change History (15)

comment:1 Changed 6 weeks ago by teor

Actual Points: 0.1
Cc: gaba added
Keywords: diagnostics macos 035-backport 040-backport 041-backport added; regression? mmap removed
Points: 0.1
Sponsor: Sponsor31-can
Status: newneeds_review

See my pull request:

The remaining merges are clean, you can see the test branches bug31571_* at:
https://github.com/teor2345/tor/branches

Gaba, I think this can be sponsor 31 can, because it helps us debug all our refactoring.

comment:2 Changed 6 weeks ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

Hi! This looks like a solid set of changes to some tricky code. I've left one suggestion and one question on the PR.

CI appears to be passing.

comment:3 Changed 6 weeks ago by nickm

Owner: set to teor
Status: needs_revisionassigned

Setting owner

comment:4 Changed 6 weeks ago by nickm

Status: assignedneeds_revision

Back to needs_revision.

comment:5 in reply to:  1 Changed 6 weeks ago by teor

Status: needs_revisionneeds_review

Replying to teor:

See my pull request:

I changed the code to use strlcpy() and strlcat() rather than strncpy() and snprintf(), and pushed an updated branch.

The remaining merges are clean, you can see the test branches bug31571_* at:
https://github.com/teor2345/tor/branches

I also added a refactor that simplifies the code, and a comment fix:

I put them on master, because they don't need to be backported.

I opened #31594 for the "close fds before abort" follow-up.

comment:6 Changed 6 weeks ago by teor

Status: needs_reviewneeds_revision

This compiled fine for me, but Windows gcc identified some bugs.
I'll fix them later tonight.

comment:7 in reply to:  1 Changed 6 weeks ago by teor

Status: needs_revisionneeds_review

Replying to teor:

See my pull request:

The remaining merges are clean, you can see the test branches bug31571_* at:
https://github.com/teor2345/tor/branches

It turns out that we can't use strlcpy() in lib/torerr, because strlcpy() is defined in lib/string on some platforms, and lib/string depends on lib/torerr.

I added a commit that checks the return values of snprintf() and strncpy() instead.

comment:8 Changed 6 weeks ago by nickm

Status: needs_reviewneeds_revision

The return-value checking looks okay to me.

I don't see any additional refactoring or comment changes on the master branch, however. Did they get eaten by a force-push? If you still want to do them, please put this back in needs_review when you're done. Otherwise this branch can go into merge_ready.

comment:9 in reply to:  8 Changed 6 weeks ago by teor

Status: needs_revisionmerge_ready

Replying to nickm:

The return-value checking looks okay to me.

I don't see any additional refactoring or comment changes on the master branch, however. Did they get eaten by a force-push? If you still want to do them, please put this back in needs_review when you're done. Otherwise this branch can go into merge_ready.

I split the comment changes into #31612, they were unrelated to this PR.
The refactoring wasn't possible due to layering issues with strlcpy().
So I just checked the return values in the 0.3.5 branch instead.

comment:10 Changed 6 weeks ago by nickm

Keywords: dgoulet-merge added; macos 035-backport 040-backport 041-backport removed

comment:11 Changed 6 weeks ago by nickm

Keywords: macos 035-backport 040-backport 041-backport added

whoops, I didn't mean to remove those tags.

comment:12 Changed 5 weeks ago by dgoulet

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

Merged PR 1287 to master.

Moving to 041 milestone for backport.

comment:13 Changed 5 weeks ago by teor

Actual Points: 0.10.3
Cc: gaba removed
Keywords: consider-backport-after-042-stable BugSmashFund added
Sponsor: Sponsor31-can

This is a complex series of changes, across multiple platforms. But it is well isolated.
It does fix a real bug that we have encountered before, and it significantly improves error diagnostics,
So we do want to backport, but not until after 0.4.2-stable.

comment:14 Changed 5 weeks ago by nickm

Keywords: 042-should removed

Removing 042-should, since this is already merged to 0.4.2

comment:15 Changed 5 weeks ago by dgoulet

Parent ID: #31570

Unparenting so I can close parent that was just merged.

Note: See TracTickets for help on using tickets.