Opened 9 months ago

Closed 8 months ago

Last modified 7 months ago

#27629 closed enhancement (fixed)

add len argument to consensus parsing functions

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: #27244 Points:
Reviewer: mikeperry Sponsor: Sponsor8

Description


Child Tickets

TicketTypeStatusOwnerSummary
#27630enhancementcloseduse strcmpstart() in rend_parse_v2_service_descriptor
#27644defectclosedwrong documentation of networkstatus_read_cached_consensus_impl

Change History (14)

comment:1 Changed 9 months ago by cyberpunks

Created the branch parselen1 at https://gitgud.io/onionk/tor.git

comment:2 Changed 9 months ago by teor

Milestone: Tor: 0.3.6.x-finalTor: 0.3.5.x-final
Sponsor: Sponsor8
Status: newneeds_review

Thanks for this branch.
It looks good to me, but I'd like to make sure that CI passes, and that someone who is more familiar with this code also does a review.

Please don't assign tickets to releases, we'll do that based on our roadmap for the release.
(This task is on our roadmap as sponsored work, so we will prioritise reviewing it before the feature freeze on Friday.)

Can you explain the fix in 5d27c458 in the commit message and changes file?
Each fix and feature also gets its own ticket number.
We usually open a child ticket for each unrelated fix in a branch.

Or is this change just refactoring?
For refactoring, the changes file should have the heading:
Code simplification and refactoring
https://github.com/torproject/tor/blob/master/scripts/maint/lintChanges.py#L22

comment:4 in reply to:  2 ; Changed 9 months ago by cyberpunks

Replying to teor:

Or is this change just refactoring?
For refactoring, the changes file should have the heading:
Code simplification and refactoring
https://github.com/torproject/tor/blob/master/scripts/maint/lintChanges.py#L22

Yeah, that was just simplifying, no behavior change replacing strncmp with strcmpstart. #27630.

So two changes files, both labeled as Code simplification and refactoring?

comment:5 in reply to:  4 ; Changed 9 months ago by teor

Type: defectenhancement

Replying to cyberpunks:

Replying to teor:

Or is this change just refactoring?
For refactoring, the changes file should have the heading:
Code simplification and refactoring
https://github.com/torproject/tor/blob/master/scripts/maint/lintChanges.py#L22

Yeah, that was just simplifying, no behavior change replacing strncmp with strcmpstart. #27630.

So two changes files, both labeled as Code simplification and refactoring?

"use strcmpstart() in rend_parse_v2_service_descriptor" is definitely "Code simplification and refactoring".

The rest of the branch makes Tor use less RAM, so it can be: "Minor feature (RAM usage)".

comment:6 Changed 9 months ago by teor

Oh, and the CI passed.

comment:7 in reply to:  5 ; Changed 9 months ago by cyberpunks

Replying to teor:

The rest of the branch makes Tor use less RAM

How does it do that? Well, there's removal of an allocation in the fuzz test. But no reduction in any actually running Tor client. This branch is just prerequisite refactoring.

There is another branch that does the actual switch to using mmap, but it's based on top of #27247 due to conflicts, so waiting on that to be merged.

comment:8 in reply to:  7 Changed 9 months ago by teor

Replying to cyberpunks:

Replying to teor:

The rest of the branch makes Tor use less RAM

How does it do that? Well, there's removal of an allocation in the fuzz test. But no reduction in any actually running Tor client. This branch is just prerequisite refactoring.

There is another branch that does the actual switch to using mmap, but it's based on top of #27247 due to conflicts, so waiting on that to be merged.

Oh, right. I looked at all the commits, including the mmap ones that are in parselen1-on-ticket27247.

So for parselen1, it's just refactoring. For the mmap branch, it's an enhancement.

comment:9 Changed 9 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

I spoke to nickm, and we don't think it's a good idea to change parsing just before a feature freeze. Let's merge this ticket in 0.3.6 instead.

comment:10 Changed 8 months ago by dgoulet

Reviewer: mikeperry

comment:11 Changed 8 months ago by ahf

why is this needed in relation to the patches in #27244 ? This seems like a lot of clean up patches in related code?

comment:12 in reply to:  11 Changed 8 months ago by cyberpunks

Replying to ahf:

why is this needed in relation to the patches in #27244 ? This seems like a lot of clean up patches in related code?

It's the same work. That's why it's a child ticket. All these functions assumed strings are NUL-terminated, which they aren't if passed something mmap'd, so they need explicit length added.

The branch parselen1-on-ticket27247 finishes the job of #27244. All this stuff needs to be rebased now though.

comment:13 Changed 8 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:14 Changed 7 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

Note: See TracTickets for help on using tickets.