USD IO: Validate data in the operator, before starting background job. #105404

New Issue

Open

opened 2023-03-03 15:41:12 +01:00 by Bastien Montagne · 10 comments

Bastien Montagne commented

2023-03-03 15:41:12 +01:00

Owner

While the crash revealed by #105160 has now been fixed (d66672e17a), don't think USD I/O operators should start the background job at all when obviously wrong data has been selected/passed as parameter. It's not really good behavior for the IO operator to return "Success" after starting a background job that will immediately fail.

Not all errors can be caught at that level of course, but things like not passing an a priori valid filepath to the import operator can be checked by the operator code itself, and immediately return CANCELLED status.

Note: Dev who intends to work on this task should first do some research over USD file specs to identify what can be easily checked at the operator level, and what should be left to actual USD loading code to error on.
Typically, invalid inputs that can be selected from the USD import file browser UI should be checked by the operator code, or even fully prevented.
Some may be prevented by proper setting of the FileBrowser options, e.g. if selecting a directory is never expected to be a valid USD import path, then the filebrowser settings should not allow a directory path to be selected, etc.

While the crash revealed by #105160 has now been fixed (d66672e17a), don't think USD I/O operators should start the background job at all when obviously wrong data has been selected/passed as parameter. It's not really good behavior for the IO operator to return "Success" after starting a background job that will immediately fail. Not all errors can be caught at that level of course, but things like not passing an *a priori* valid filepath to the import operator can be checked by the operator code itself, and immediately return `CANCELLED` status. > Note: Dev who intends to work on this task should first do some research over USD file specs to identify what can be easily checked at the operator level, and what should be left to actual USD loading code to error on. Typically, invalid inputs that can be selected from the USD import file browser UI should be checked by the operator code, or even fully prevented. Some may be prevented by proper setting of the FileBrowser options, e.g. if selecting a directory is never expected to be a valid USD import path, then the filebrowser settings should not allow a directory path to be selected, etc.

Bastien Montagne added the

Pipeline, Assets & IO

Good First Issue

labels 2023-03-03 15:41:13 +01:00

Bastien Montagne added this to the USD project 2023-03-03 15:41:14 +01:00

Bastien Montagne referenced this issue

2023-03-03 15:41:49 +01:00

Regression: Crash when user opens directory instead of file on USD import #105160

Prakhar-Singh-Chouhan commented

2023-03-05 13:06:00 +01:00

Contributor

hey @mont29 this issue is open right ? Could you please explain to me in more detail what needs to be done here?

hey @mont29 this issue is open right ? Could you please explain to me in more detail what needs to be done here?

Bastien Montagne commented

2023-03-06 11:06:48 +01:00

Author

Owner

Yes it is. Essentially, code in exec callback of WM_OT_usd_import should check that the selected path is a priori a valid candidate for USD import.

Yes it is. Essentially, code in `exec` callback of `WM_OT_usd_import` should check that the selected path is *a priori* a valid candidate for USD import.

Piyush Aniruddha Udhao commented

2023-03-10 13:44:57 +01:00

Hey @mont29, so I have been going through the source code and I just wanted to confirm if the bug lies in this piece of code.

char filename[FILE_MAX];
RNA_string_get(op->ptr, "filepath", filename);

This is in the /source/blender/editors/io/io_usd.c file.

Hey @mont29, so I have been going through the source code and I just wanted to confirm if the bug lies in this piece of code. ``` char filename[FILE_MAX]; RNA_string_get(op->ptr, "filepath", filename); ``` ![image](/attachments/38a684f9-41a2-4dd0-beca-261256f39775) This is in the /source/blender/editors/io/io_usd.c file.

61 KiB

Piyush Aniruddha Udhao commented

2023-03-10 13:55:56 +01:00

I dove into the code of RNA_string_get(), and it calls another function RNA_property_string_get() which is where I feel a check for char *value is required?

I dove into the code of `RNA_string_get()`, and it calls another function `RNA_property_string_get()` which is where I feel a check for `char *value` is required?

Mike-Vita commented

2023-03-10 14:08:00 +01:00

I think you need to do an extra check after these two lines. It checks for "No filename given" but it doesn't check for "Invalid filename".

I think you need to do an extra check after these two lines. It checks for "No filename given" but it doesn't check for "Invalid filename".

Piyush Aniruddha Udhao referenced a pull request that will close this issue

2023-03-11 13:59:03 +01:00

WIP: Fix #105404: The USD Input operator does not validate the file received as input. #105659

Brecht Van Lommel commented

2023-03-11 14:22:51 +01:00

Owner

In my opinion this is not necessary, what is the benefit of validating data in two places? Performance is not a concern here, and duplicated validation code means possibility of things to go out of sync.

In my opinion this is not necessary, what is the benefit of validating data in two places? Performance is not a concern here, and duplicated validation code means possibility of things to go out of sync.

Brecht Van Lommel commented

2023-03-11 14:28:52 +01:00

Owner

If this is marked as a good first issue it should also which conditions you expect this to check, because even for me it's not clear what the intent is here. A valid file path can mean many things, including file existence, file/directory, read permissions, extensions, etc.

If this is marked as a good first issue it should also which conditions you expect this to check, because even for me it's not clear what the intent is here. A valid file path can mean many things, including file existence, file/directory, read permissions, extensions, etc.

Piyush Aniruddha Udhao commented

2023-03-11 15:42:02 +01:00

@brecht yeah it was confusing, so I just assumed .usd files would be the right ones and continued. But I should've done proper research, my bad.
As far as the necessity of this issue goes, it sure helped me gain experience with submitting PRs for blender. Messed up a lot, learnt a lot. All in all, good practice.

@brecht yeah it was confusing, so I just assumed .usd files would be the right ones and continued. But I should've done proper research, my bad. As far as the necessity of this issue goes, it sure helped me gain experience with submitting PRs for blender. Messed up a lot, learnt a lot. All in all, good practice.

Sreejan-Chattopadhyay commented

2023-03-28 20:48:03 +02:00

hey @mont29 I want to know is this issue still open? If yes, can you explain me which parameters to check for a valid filename?

hey @mont29 I want to know is this issue still open? If yes, can you explain me which parameters to check for a valid filename?

Bastien Montagne removed the

Good First Issue

label 2023-03-29 11:12:13 +02:00

Bastien Montagne commented

2023-03-29 11:27:22 +02:00

Author

Owner

Removing the good first issue tag since there is clearly no common understanding of its meaning.

Updated task with some more context and expectations info.

Removing the good first issue tag since there is clearly no common understanding of its meaning. Updated task with some more context and expectations info.

Sign in to join this conversation.

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

6 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#105404

No description provided.