Skip to content

Comments

save: Use dd with the notrunc & fsync and postpone truncation#3814

Merged
JoeKar merged 2 commits intomicro-editor:masterfrom
JoeKar:fix/sudo-save
Aug 12, 2025
Merged

save: Use dd with the notrunc & fsync and postpone truncation#3814
JoeKar merged 2 commits intomicro-editor:masterfrom
JoeKar:fix/sudo-save

Conversation

@JoeKar
Copy link
Member

@JoeKar JoeKar commented Jul 22, 2025

This will stop the overall truncation of the target file.
We need to do this because dd, like other coreutils, already truncates the file
on open(). In case we can't store the backup file afterwards we would end up in
a truncated file for which the user has no write permission by default.

@dmaluka
Copy link
Collaborator

dmaluka commented Jul 22, 2025

Erm... conv=notrunc, unsurprisingly, disables truncate completely, not just when opening the file.

Try opening a file which is bigger than 4K, delete all the text in it and replace it with a single line saying: I'm the only line in this file. Then save it with sudo. Then open it again, or cat it, and see what was actually saved.

EDIT: not even necessarily bigger than 4K, just any file bigger than this single line.

@JoeKar
Copy link
Member Author

JoeKar commented Jul 23, 2025

Erm... conv=notrunc, unsurprisingly, disables truncate completely, not just when opening the file.

Holy, I knew that I had forgotten something (just used a smaller one for testing). 🤦‍♂️
Since tee behaves the same as dd we can't simply change the used coreutil, but we could chain them:

We still could use conv=notrunc and invoke "truncate", "-s "+name afterwards on the command line. Either before writing with the size 0 or after the write with the size we determined. I will try this by creating a truncate method of wrappedFile.

@JoeKar JoeKar changed the title save: Use dd with the notrunc and fsync option save: Use dd with the notrunc option Jul 24, 2025
@JoeKar JoeKar changed the title save: Use dd with the notrunc option save: Use dd with the notrunc & fsync option and truncate as command Jul 31, 2025
if wf.withSudo {
// we don't need to stop the screen here, since it is still stopped
// by openFile()
cmd := exec.Command(config.GlobalSettings["sucmd"].(string), "truncate", "-s", strconv.FormatInt(size, 10), wf.name)
Copy link
Contributor

@niten94 niten94 Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked all Unix platforms in cross-compile.sh and only 3 platforms don't have truncate as a command, which are Illumos, NetBSD, and OpenBSD.

If we will replace the truncate command, dd count=0 of=path may work on all such platforms but could only truncate to 0 bytes. There might be a difference in behavior, since the command may open the file with O_TRUNC (and closes it after) instead of just using the truncate system call, but I doubt it would be different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for testing this on other platforms! 👍
So exec.Command(config.GlobalSettings["sucmd"].(string), "dd", "count=0", "if=/dev/zero", "of="+wf.name) indeed looks like the better option then.

Great to have such a fragmentation with the coreutils too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to have such a fragmentation with the coreutils too.

Maybe not really, since it's hard to ensure compatibility of shell scripts without reading the documentation of each platform.

I currently find POSIX 2017 (command list) to be most reliable when strictly writing portable commands, but it takes a while to read and understand.

@JoeKar JoeKar changed the title save: Use dd with the notrunc & fsync option and truncate as command save: Use dd with the notrunc & fsync and postpone truncation Aug 5, 2025
Comment on lines 86 to 87
cmd = exec.Command(config.GlobalSettings["sucmd"].(string), "dd", "bs=4k", "of="+name)
cmd = exec.Command(config.GlobalSettings["sucmd"].(string), "dd", "bs=4k", "conv=notrunc,fsync", "of="+name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to check this, but conv=fsync seems to be unsupported only on NetBSD and Illumos. Maybe run dd without conv=fsync on both platforms and add a TODO comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn...

Maybe run dd without conv=fsync on both platforms and add a TODO comment?

Looks like the only option right now. I've to better idea so far. 🤔

Using notrunc  will stop the overall truncation of the target file done by sudo.
We need to do this because dd, like other coreutils, already truncates the file
on open(). In case we can't store the backup file afterwards we would end up in
a truncated file for which the user has no write permission by default.
Instead we use a second call of `dd` to perform the necessary truncation
on the command line.
With the fsync option we force the dd process to synchronize the written file
to the underlying device.
@JoeKar JoeKar merged commit 1dbb058 into micro-editor:master Aug 12, 2025
6 checks passed
@JoeKar JoeKar deleted the fix/sudo-save branch August 12, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants