Opened 10 years ago

Last modified 8 years ago

#4018 assigned Enhancement

Use octal notation for umask in settings.json

Reported by: taem Owned by: gvdl
Priority: Normal Milestone: None Set
Component: libtransmission Version: 2.20
Severity: Minor Keywords: umask patch
Cc:

Description

Hi,

When T creates session default settings, it is better to use process umask value, I guess. Without this patch it is impossible to use User Private Groups, for example.

getumask()'s code is copy/paste from getumask(3) manpage, which is glibc extension.

Thanks.

Attachments (3)

umask.patch (1.1 KB) - added by taem 10 years ago.
4018bencStrtointmax.patch (1.8 KB) - added by gvdl 8 years ago.
Parse integers from benc strings if requested.
4018-2-bencNumeric.patch (4.1 KB) - added by gvdl 8 years ago.

Download all attachments as: .zip

Change History (14)

Changed 10 years ago by taem

comment:1 follow-up: Changed 10 years ago by jordan

taem, why is it impossible to use "User Private Groups" without this patch? All this patch does is change the default umask value; people could still change it by editing settings.json.

What is the use case for this change? I'm not against it or for it at this point but I don't understand how it adds value.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 10 years ago by taem

Replying to jordan:

taem, why is it impossible to use "User Private Groups" without this patch? All this patch does is change the default umask value; people could still change it by editing settings.json.

Sorry, I was incorrect.

What is the use case for this change? I'm not against it or for it at this point but I don't understand how it adds value.

It is not obvious how user can change umask value. Default umask in settings.json is 18. IMHO, ordinary user can't translate it to 022.

Ok. Maybe there is no need to save umask value to settings.json? Maybe let the user to define which umask value to use by the system tools (e.g. umask).

comment:3 in reply to: ↑ 2 Changed 10 years ago by jordan

Replying to taem:

It is not obvious how user can change umask value. Default umask in settings.json is 18. IMHO, ordinary user can't translate it to 022.

Yes, it's unfortunate that settings.json can't handle octal notation. Maybe it would be better to have a umask_string key that reads "022".

Ok. Maybe there is no need to save umask value to settings.json? Maybe let the user to define which umask value to use by the system tools (e.g. umask).

umask support was added to settings.json at the request of users back in ticket #2234. The permissions issues reported with transmission-daemon have been a recurring issue, and IMO taking away the umask setting would make that issue even worse.

comment:4 follow-up: Changed 10 years ago by jordan

taem, if I'm re-reading this ticket correctly, the original issue of using umask() in tr_sessionGetDefaultSettings() has been dropped, and the remaining issue in this ticket is to possibly have settings.json's `umask' field be a string, rather than a number, so that it can be represented in the octal notation we're all used to for umask.

Is that a correct summary? I want to make sure I understand before I change any code... :)

comment:5 in reply to: ↑ 4 Changed 10 years ago by taem

Replying to jordan:

Is that a correct summary? I want to make sure I understand before I change any code... :)

Yes, umask octal notation in settings.json would be better.

Thanks.

comment:6 Changed 9 years ago by x190

  • Summary changed from Use process umask in session default settings to Use octal notation for umask in settings.json

comment:7 Changed 9 years ago by x190

The wiki does explain this quite well. https://trac.transmissionbt.com/wiki/EditConfigFiles#FilesandLocations

" umask: Number (default = 18) Sets transmission's file mode creation mask. See the umask(2) manpage for more information. Users who want their saved torrents to be world-writable may want to set this value to 0. Bear in mind that the json markup language only accepts numbers in base 10, so the standard umask(2) octal notation "022" is written in settings.json as 18. "

Changed 8 years ago by gvdl

Parse integers from benc strings if requested.

comment:8 Changed 8 years ago by gvdl

While the problem with JSON is explained in the wiki it is also true that umask settings have traditionally been octal since the pdp-11 days back in the '70s. Sometimes it just doesn't do to buck and eternity of tradition.

I have modified tr_bencGetInt to parse a pure numeric string (No trailing non-numeric characters). This allows the umask to be set to "022" or in my case "002".

The patch to GetInt? is attached, however I'm not sure if this is the right thing to do as it may be too general. I personally like being able to use any of the strtol conversions (including both 0x hex and 0 octal), but that may just be personal taste.

comment:9 Changed 8 years ago by gvdl

  • Owner changed from jordan to gvdl
  • Status changed from new to assigned

comment:10 Changed 8 years ago by jordan

This strikes me as risky code. Are we really certain that the only place we're ever going to see a leading zero is in the umask code?

I can't think of a counterexample off the top of my head, but it still seems like a dangerous assumption to make.

comment:11 Changed 8 years ago by gvdl

The answer is, as with most thing, it depends.

The danger is limited as it only effects those numeric fields that have strings in them, since that is currently illegal we know that no files out in the wild are using stings as that would get errors, therefore we don't have to worry about existing settings.json files.

The use of strings to get at other numeric bases is something that only developers will really do, since the fields will continue to be described as numeric non-developers will stick with decimal. So the risk is probably only with developers, now I admit the 0 prefix can trip them up, let's face it accidental octalisation is one of the typical newbie mistakes in C. Still the only time I've ever seen developers fall into that trap is when they are formatting decimal digits into a column, for instance declaring a lot of #defines at once. Unlikely to be a problem in the settings.json file.

As I said I've used this exact technique in OSX's IOKit plist files and never experienced any problems with it.

However, I have another patch that only does the conversion to the umask field, you can take whichever patch you prefer.

Changed 8 years ago by gvdl

Note: See TracTickets for help on using tickets.