ximion / appstream-generator

A fast AppStream metadata generator
GNU Lesser General Public License v3.0
44 stars 29 forks source link

utils: guard mkdir call with std.file.exists #114

Closed pabloyoyoista closed 1 year ago

pabloyoyoista commented 1 year ago

Fixes the following stack trace:

2023-04-23 21:02:16 - INFO: Exporting data for v3.15 (main/aarch64) 2023-04-23 21:02:16std.file.FileException@/usr/include/d/std/file.d(2972): /home/abuild/cache/export/media/v3.15/g/gv/gvim.desktop/4016a69fc2135fa29a3414b2c8bd20d9//icons: File exists ---------------- ??:? @trusted bool std.file.cenforce!(bool).cenforce(bool, scope const(char)[], scope const(char)*, immutable(char)[], ulong) [0x563624998170] ??:? @safe void std.file.mkdir!(immutable(char)[]).mkdir(immutable(char)[]) [0x563624a439d0] ??:? @trusted void asgen.utils.copyDir(in immutable(char)[], in immutable(char)[], bool) [0x563624a43500] ??:? /usr/bin/appstream-generator [0x5636249e2190] ??:? void std.parallelism.ParallelForeach!(asgen.backends.interfaces.Package[]).ParallelForeach.opApply(scope int delegate(ref asgen.backends.interfaces.Package)).doIt() [0x5636249e8f30] ??:? void std.parallelism.TaskPool.executeWorkLoop() [0x7fd515e4efe0] ??:? thread_entryPoint [0x7fd515b9d5b0]

This occurred once during debugging, but haven't tested this in-depth. Anyway, looking at the code seems sort of obvious fix.

nekopsykose commented 1 year ago

isn't this still liable to TOCTOU in the exact same way? it's better to catch the FileException or something instead and check what it is (though i'm not sure it disambiguates 'what exception')

nekopsykose commented 1 year ago

or maybe there's an api better than that 'mkdir' that can just pass if it already exists

nekopsykose commented 1 year ago

(the toctou here is something can make the dir between when you check it and when you mkdir it, which would crash the same way, just much more rare)

pabloyoyoista commented 1 year ago

Sure, thank you both! I am pretty lost with D. It's changed now!

nekopsykose commented 1 year ago

you should update the commit message too

pabloyoyoista commented 1 year ago

Done!