Skip to content

filetransfer: handle exceptions thrown from enqueueItem #348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cole-h wants to merge 3 commits into
base: main
Choose a base branch
from
Open

filetransfer: handle exceptions thrown from enqueueItem #348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b80d320
filetransfer: mark enqueueFileTransfer() as noexcept
cole-h Feb 12, 2026
2208cf9
filetransfer: handle exceptions thrown from enqueueItem
cole-h Feb 12, 2026
3619618
fixup: make ItemHandle own a ref<Item> to prevent UAF on failed trans…
cole-h Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions src/libstore/filetransfer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,19 +962,32 @@ struct curlFileTransfer : public FileTransfer
writeFull(wakeupPipe.writeSide.get(), " ");
#endif

return ItemHandle(static_cast<Item &>(*item));
return ItemHandle(ref<Item>(std::move(item)));
}

ItemHandle enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) override
ItemHandle
enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override
{
/* Handle s3:// URIs by converting to HTTPS and optionally adding auth */
if (request.uri.scheme() == "s3") {
auto modifiedRequest = request;
modifiedRequest.setupForS3();
return enqueueItem(make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback)));
auto item = make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback));
try {
Copy link
Member Author

@cole-h cole-h Feb 12, 2026

Choose a reason for hiding this comment

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

It was suggested that I move this try-catch into enqueueItem directly, but I decided not to do that because I didn't want to mess with the fix in NixOS#14418

I tried a few different ways to reduce the duplication, but I couldn't figure out the best way to do that. ref<TransferItem> item; outside of the if doesn't work because there's no constructor, and I didn't want to copy request every time in the off chance that it's an s3 request...

Happy to take suggestions on improvements here.

Copy link

@proski proski Feb 12, 2026

Choose a reason for hiding this comment

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

Please see my comment below (I should have posted it here). Refactoring is definitely doable if item is initialized once and unconditionally, meaning that its value should come from another function (make it inline if you want). No new copying is needed, the request is passed by reference and the callback is moved.

return enqueueItem(item);
} catch (const nix::Error & e) {
item->fail(e);
return ItemHandle(ref<Item>(std::move(item)));
}
}

return enqueueItem(make_ref<TransferItem>(*this, request, std::move(callback)));
auto item = make_ref<TransferItem>(*this, request, std::move(callback));
try {
return enqueueItem(item);
} catch (const nix::Error & e) {
item->fail(e);
return ItemHandle(ref<Item>(std::move(item)));
}
Copy link

@proski proski Feb 12, 2026

Choose a reason for hiding this comment

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

That's a lot of repetition. Maybe this function should be refactored?

    inline ref<TransferItem>
    makeTransferItem(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept
    {
        /* Handle s3:// URIs by converting to HTTPS and optionally adding auth */
        if (request.uri.scheme() == "s3") {
            auto modifiedRequest = request;
            modifiedRequest.setupForS3();
            return make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback));
        } else {
            return make_ref<TransferItem>(*this, request, std::move(callback));
        }
    }

    ItemHandle
    enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override
    {
        const auto item = makeTransferItem(request, std::move(callback));

        try {
            return enqueueItem(item);
        } catch (const nix::Error & e) {
            item->fail(e);
            return ItemHandle(static_cast<Item &>(*item));
        }
    }

It compiles, but please don't trust my choices about naming and inlining too much.

}
Comment on lines +968 to 991
Copy link

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the actual function in the file to confirm its current state
head -1000 src/libstore/filetransfer.cc | tail -100

Repository: DeterminateSystems/nix-src

Length of output: 3430

🏁 Script executed:

# Check if setupForS3 can throw nix::Error
rg -n 'setupForS3' --type cpp -B 2 -A 10

Repository: DeterminateSystems/nix-src

Length of output: 6673

🏁 Script executed:

# Check for throw statements in s3-related code
rg -n 'throw' --type cpp -g '*s3*' | head -30

Repository: DeterminateSystems/nix-src

Length of output: 1282

🏁 Script executed:

# Look for ParsedS3URL::parse implementation
rg -n 'ParsedS3URL::parse' --type cpp -B 2 -A 15

Repository: DeterminateSystems/nix-src

Length of output: 7084

🏁 Script executed:

# Check if TransferItem constructor can throw
rg -n 'struct TransferItem\|class TransferItem' --type cpp -A 20

Repository: DeterminateSystems/nix-src

Length of output: 52

noexcept may cause std::terminate if pre-enqueue operations throw.

The function is marked noexcept, but several operations before the try-catch blocks can throw:

  • modifiedRequest.setupForS3() (line 974) calls ParsedS3URL::parse() which throws BadURL (e.g., for invalid bucket names or missing keys)
  • make_ref<TransferItem>(...) (lines 975, 984) can throw std::bad_alloc or exceptions from the TransferItem constructor

If any of these throw, std::terminate will be called. Consider wrapping the entire function body in a try-catch, or removing noexcept if the exception guarantee cannot be upheld.

🛠️ Suggested fix: wrap entire body in try-catch 
 ItemHandle
 enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override
 {
+    try {
     /* Handle s3:// URIs by converting to HTTPS and optionally adding auth */
     if (request.uri.scheme() == "s3") {
         auto modifiedRequest = request;
         modifiedRequest.setupForS3();
         auto item = make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback));
-        try {
-            return enqueueItem(item);
-        } catch (const nix::Error & e) {
-            item->fail(e);
-            return ItemHandle(ref<Item>(std::move(item)));
-        }
+        return enqueueItem(item);
     }

     auto item = make_ref<TransferItem>(*this, request, std::move(callback));
-    try {
-        return enqueueItem(item);
-    } catch (const nix::Error & e) {
-        item->fail(e);
-        return ItemHandle(ref<Item>(std::move(item)));
+    return enqueueItem(item);
+    } catch (const nix::Error & e) {
+        // Need to handle the case where item doesn't exist yet
+        // This requires restructuring to ensure item is always created first
+        // or using a different error reporting mechanism
     }
 }

Note: A complete fix requires restructuring to handle exceptions that occur before the TransferItem is created (e.g., in setupForS3()), since there's no item to mark as failed in that case. You may need to create a "dummy" failed item or reconsider the noexcept guarantee.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/libstore/filetransfer.cc` around lines 968 - 991, The function
enqueueFileTransfer is marked noexcept but operations like
modifiedRequest.setupForS3(), make_ref<TransferItem>(...), and the TransferItem
constructor can throw, which would call std::terminate; to fix, remove the
noexcept specifier or wrap the entire function body in a try-catch that catches
std::exception (and ...), and in the catch create and return a failed ItemHandle
(or otherwise return an ItemHandle that signals failure) while calling
item->fail(e) when a TransferItem exists; specifically adjust the s3 branch
(where setupForS3() is called) and the common path that constructs
make_ref<TransferItem> so exceptions are caught before escaping
enqueueFileTransfer and handled by returning a dummy/failed ItemHandle or
propagating safely if you choose to drop noexcept.


void unpauseTransfer(ref<TransferItem> item)
Expand All @@ -988,7 +1001,7 @@ struct curlFileTransfer : public FileTransfer

void unpauseTransfer(ItemHandle handle) override
{
unpauseTransfer(ref{static_cast<TransferItem &>(handle.item.get()).shared_from_this()});
unpauseTransfer(ref{static_cast<TransferItem &>(*handle.item).shared_from_this()});
}
};

Expand Down Expand Up @@ -1047,7 +1060,7 @@ void FileTransferRequest::setupForS3()
#endif
}

std::future<FileTransferResult> FileTransfer::enqueueFileTransfer(const FileTransferRequest & request)
std::future<FileTransferResult> FileTransfer::enqueueFileTransfer(const FileTransferRequest & request) noexcept
{
auto promise = std::make_shared<std::promise<FileTransferResult>>();
enqueueFileTransfer(request, {[promise](std::future<FileTransferResult> fut) {
Expand Down
10 changes: 5 additions & 5 deletions src/libstore/include/nix/store/filetransfer.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,11 +262,11 @@ public:
*/
struct ItemHandle
{
std::reference_wrapper<Item> item;
ref<Item> item;
friend struct FileTransfer;

ItemHandle(Item & item)
: item(item)
explicit ItemHandle(ref<Item> item)
: item(std::move(item))
{
}
};
Expand All @@ -279,14 +279,14 @@ public:
* exception.
*/
virtual ItemHandle
enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) = 0;
enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept = 0;

/**
* Unpause a transfer that has been previously paused by a dataCallback.
*/
virtual void unpauseTransfer(ItemHandle handle) = 0;

std::future<FileTransferResult> enqueueFileTransfer(const FileTransferRequest & request);
std::future<FileTransferResult> enqueueFileTransfer(const FileTransferRequest & request) noexcept;

/**
* Synchronously download a file.
Expand Down
Loading