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 u32 parse panic in decode_csv #288

Merged

Conversation

smackysnacks
Copy link
Contributor

@smackysnacks smackysnacks commented May 27, 2024

This PR fixes an unhandled parsing error in decode_csv found with cargo fuzz. The following input no longer causes a panic:

<?xml version="1.0" encoding="UTF-8"?>
<map version="1.4" tiledversion="1.4.0" orientation="orthogonal" renderorder="right-down" width="100" height="100" tilewidth="32" tileheight="32" infinite="0" backgroundcolor="#ff00ff" nextlayerid="3" nextobjectid="5">
 <tileset firstgid="1" name="tilesheet" tilewidth="32" tileheight="32" tilecount="84" columns="14">
  <properties>
   <property name="tileset property" value="tsp"/>
  </properties>
  <image source="tilesheet.png" width="448" height="192"/>
  <tile id="1">
   <properties>
    <property name="a tile property" value="123"/>
   </properties>
  </tile>
 </tileset>
 <layer id="1" name="Tile Layer 1" width="100" height="100">
  <properties>
   <property name="prop1" value="12"/>
   <property name="prop2" value="some text"/>
   <property name="prop3">Line 1
Line 2
Line 3,
  etc
   </property>
  </properties>
  <data encoding="csv">
35,35,35,35,35,33,33,33,33,33,33,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,,0,
0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
 </objectgroup>
</map>

@smackysnacks smackysnacks force-pushed the fix/csvdecode-unhandled-parse-err branch from 2df6fd9 to af005c6 Compare May 27, 2024 18:24
Copy link
Contributor

@aleokdev aleokdev left a comment

Choose a reason for hiding this comment

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

Many thanks!

src/error.rs Outdated
@@ -12,6 +12,8 @@ pub enum Error {
DecompressingError(std::io::Error),
/// An error occured when decoding a base64 encoded dataset.
Base64DecodingError(base64::DecodeError),
/// An error occurred when decoding a csv encoded dataset.
CsvDecodingError(String),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer that this were an error type so the user could handle it (or format it some other way) if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we have several options here. One option would be

CsvDecodingError(Box<dyn std::error::Error + Send + Sync + 'static>)

Alternatively, we could do something like

/// Errors that can occur while decoding csv data.
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum CsvError {
    TileDataParseError(ParseIntError)
}

/// Errors which occurred when parsing the file
#[derive(Debug)]
#[non_exhaustive]
pub enum Error {
<SNIP>
    /// An error occurred when decoding a csv encoded dataset.
    CsvDecodingError(CsvError),

Do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Second one looks better IMO: Lets you look into the error, like in the other enum variants.

src/error.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@aleokdev aleokdev left a comment

Choose a reason for hiding this comment

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

LGTM!

@aleokdev aleokdev requested a review from bjorn May 28, 2024 18:00
@aleokdev aleokdev merged commit 49881df into mapeditor:next May 29, 2024
3 checks passed
@aleokdev
Copy link
Contributor

I forgot to check for the changelog again x_x I'll update it when the version gets released I guess

@bjorn
Copy link
Member

bjorn commented May 29, 2024

@aleokdev Heh, we should probably look into adding a workflow warning about this. :D

@aleokdev
Copy link
Contributor

I have one set up for another project as a GH action, I'll look into importing it whenever possible if you'd like.

@smackysnacks smackysnacks deleted the fix/csvdecode-unhandled-parse-err branch May 29, 2024 12:06
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.

3 participants