Skip to content

Conversation

@urpetkov-amd
Copy link
Contributor

Fixing dump model ops feature for MIGraphX EP on Windows. The feature wasn't functional because of saving format rules on Windows which are opposite from Linux.

Current state of the feature gives us opportunity to generate and save onnx model after subgraph optimizations before compiling it. On this way we can look how model graph looks like after optimizations and we can use the optimized model.

snnn
snnn previously approved these changes Oct 24, 2025
Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

Yes, all files should be opened in binary mode.

Also, you should check if the file was closed successfully, which is another bug.

@urpetkov-amd
Copy link
Contributor Author

Thanks.
Okay, should we merge this and do that in another PR or add to this one? @snnn

@urpetkov-amd
Copy link
Contributor Author

@snnn

I added changes for this. I added check if file is open, if it's not successfully opened then we can throw an error, otherwise print a log. In this way check for closing is not necessary, it will be closed in the ofstream destructor after leaving the block.

@snnn
Copy link
Member

snnn commented Oct 29, 2025

This PR looks good and fixes the issue on Windows.

From a security perspective, there's a potential path traversal vulnerability here. The filenames for the dumped models are created from the graph/node names in the ONNX model without sanitization. An attacker could craft a model with a name like ../../etc/passwd to write files to arbitrary locations.

This feature is for debugging, so the risk is lower, but it's still a potential issue if used in a shared environment.

I'd recommend sanitizing the names before using them as filenames. A simple sanitization function could be something like this:

#include <string>
#include <string_view>

std::string SanitizeFileName(std::string_view name) {
    std::string newName(name);
    for (char& c : newName) {
        switch (c) {
            case '\\':
            case '/':
            case '\"':
            case '|':
            case '<':
            case '>':
            case ':':
            case '?':
            case '*':
                c = '_';
                break;
        }
    }
    return newName;
}

// ... usage ...
std::string model_name = SanitizeFileName(graph_viewer.Name()) + ".onnx";

I've noticed that other parts of onnxruntime that write files might also not have strong path traversal prevention, so this might be a broader issue. Given that, I'll leave it to you to decide if this is something you want to address in this PR or if it should be handled separately as a larger effort to improve security across the project.

Other than this security consideration, the PR looks good to me.

if (dump_model_ops_) {
std::string onnx_name = fused_node.Name() + ".onnx";
std::ofstream ofs(onnx_name);
std::ofstream ofs(onnx_name, std::ios::binary);
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a small typo here. The variable should probably be onnx_name instead of model_name to match the variable defined a few lines above.

std::ofstream ofs(onnx_name, std::ios::binary);
if (!ofs.is_open()) {
ORT_THROW("Failed to open file to dump ONNX model: " + model_name);
}
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the ORT_THROW above, this should likely use onnx_name to log the correct filename.

std::ofstream ofs(onnx_name);
std::ofstream ofs(onnx_name, std::ios::binary);
if (!ofs.is_open()) {
ORT_THROW("Failed to open file to dump ONNX model: " + model_name);
Copy link
Member

Choose a reason for hiding this comment

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

This is a good fix. For even more robust error handling, you might consider checking the stream's state after closing the file (e.g., if (!ofs)). This would catch potential errors during the final flush to disk, which could result in a corrupt or incomplete file. However, I see that not checking the return of close() is a common pattern in this codebase, so this is more of a suggestion for hardening the code rather than a required change for this PR.

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.

2 participants