Skip to content
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.

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

[Fix] Fix potential array overrun risk #872

Merged
merged 1 commit into from
Jul 2, 2023
Merged

Conversation

kv-chiu
Copy link
Contributor

@kv-chiu kv-chiu commented Jun 8, 2023

Motivation

I would like to use the OrientedRCNN model, but it seems that the model library in version 1.x is not yet fully developed. Therefore, I choose to continue using the main version. In the main version, when testing with the DOTA dataset, I encounter the following error due to array out-of-bounds:

Merging patch bboxes into the full image!!!
Traceback (most recent call last):
  File "/my/path/to/mmrotate/tools/test.py", line 271, in <module>
    main()
  File "/my/path/to/mmrotate/tools/test.py", line 253, in main
    dataset.format_results(outputs, **kwargs)
  File "/my/path/to/mmrotate/mmrotate/datasets/dota.py", line 327, in format_results
    id_list, dets_list = self.merge_det(results, nproc)
  File "/my/path/to/mmrotate/mmrotate/datasets/dota.py", line 230, in merge_det
    x_y_2 = re.findall(r'\d+', x_y[0])
IndexError: list index out of range

Modification

To address this error, I examined the values of x_y on a small dataset and found that they were all empty lists. x_y is treated as an offset in the program, and to avoid array out-of-bounds, I added a conditional check. When x_y is empty, I set it to the default value of (0, 0).

I tested the modified program on both small and large datasets, and the error was resolved. The test yielded reasonable results that aligned with expectations.

The modifications are as follows:

def merge_det(self, results, nproc=4):
    """Merging patch bboxes into full image.

    Args:
        results (list): Testing results of the dataset.
        nproc (int): number of process. Default: 4.

	Returns:
        list: merged results.
    """

    def extract_xy(img_id):
        """Extract x and y coordinates from image ID.

        Args:
            img_id (str): ID of the image.

        Returns:
            Tuple of two integers, the x and y coordinates.
        """
        pattern = re.compile(r"__(\d+)___(\d+)")
        match = pattern.search(img_id)
        if match:
            x, y = int(match.group(1)), int(match.group(2))
            return x, y
        else:
            return 0, 0

    collector = defaultdict(list)
    for idx, img_id in enumerate(self.img_ids):
        result = results[idx]
        oriname = img_id.split('__', maxsplit=1)[0]
        x, y = extract_xy(img_id)
        new_result = []
        for i, dets in enumerate(result):
            bboxes, scores = dets[:, :-1], dets[:, [-1]]
            ori_bboxes = bboxes.copy()
            ori_bboxes[..., :2] = ori_bboxes[..., :2] + np.array([x, y], dtype=np.float32)
            labels = np.zeros((bboxes.shape[0], 1)) + i
            new_result.append(np.concatenate([labels, ori_bboxes, scores], axis=1))
        new_result = np.concatenate(new_result, axis=0)
        collector[oriname].append(new_result)

    merge_func = partial(_merge_func, CLASSES=self.CLASSES, iou_thr=0.1)
    if nproc <= 1:
        print('Executing on Single Processor')
        merged_results = mmcv.track_iter_progress((map(merge_func, collector.items()), len(collector)))
    else:
        print(f"Executing on {nproc} processors")
        merged_results = mmcv.track_parallel_progress(merge_func, list(collector.items()), nproc)

    # Return a zipped list of merged results
    return zip(*merged_results)

BC-breaking

The current modification is applicable to the main version and the current documentation. I noticed that the 1.x version has made changes to the overall architecture. Until the release of the 1.x version, this modification is worthwhile.

Use cases

This PR does not introduce any new features. Therefore, no specific use cases or updates to the documentation are necessary.

Checklist

  • Pre-commit or other linting tools have been used to fix potential lint issues.
  • The modification is covered by comprehensive unit tests. If not, please add more unit tests to ensure correctness.
  • The documentation has been appropriately updated, including docstrings or example tutorials.

Please let me know if you need any further assistance or if there is anything else I can help you with.

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2023

CLA assistant check
All committers have signed the CLA.

@kv-chiu kv-chiu changed the title Fix potential array overrun risk [Fix] Fix potential array overrun risk Jun 8, 2023
@vansin vansin requested a review from zytx121 June 26, 2023 13:55
Copy link
Collaborator

@liuyanyi liuyanyi left a comment

Choose a reason for hiding this comment

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

LGTM

@RangiLyu RangiLyu changed the base branch from main to dev July 2, 2023 07:24
@RangiLyu RangiLyu merged commit 9ea1aee into open-mmlab:dev Jul 2, 2023
4 of 20 checks passed
ssy-csu pushed a commit to ssy-csu/mmrotate that referenced this pull request Dec 28, 2023
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.

4 participants