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

122 partitions detected on FreeNAS iso #6

Closed
zvin opened this issue Jun 18, 2020 · 6 comments
Closed

122 partitions detected on FreeNAS iso #6

zvin opened this issue Jun 18, 2020 · 6 comments
Labels

Comments

@zvin
Copy link
Contributor

zvin commented Jun 18, 2020

node-gpt detects 122 partitions with incoherent sizes, offsets and names on this iso: https://download.freenas.org/11.3/STABLE/U3.2/x64/FreeNAS-11.3-U3.2.iso

fdisk sees this:

The backup GPT table is corrupt, but the primary appears OK, so that will be used.
Disk FreeNAS-11.3-U3.2.iso: 748.16 MiB, 784496640 bytes, 1532220 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: gpt
Disk identifier: B05BDCDF-A44F-11EA-A1B4-0CC47A1228F8

Device                 Start   End Sectors  Size Type
FreeNAS-11.3-U3.2.iso1    80  1679    1600  800K EFI System
FreeNAS-11.3-U3.2.iso2     3    32      30   15K FreeBSD boot

Partition table entries are not in disk order.
@lurch
Copy link
Contributor

lurch commented Jun 18, 2020

parted version 2.3 agrees with fdisk:

$ parted -s FreeNAS-11.3-U3.2.iso u s p
Error: The backup GPT table is corrupt, but the primary appears OK, so that will be used.
Model:  (file)
Disk /home/andrew/Downloads/FreeNAS-11.3-U3.2.iso: 1532220s
Sector size (logical/physical): 512B/512B
Partition Table: gpt

Number  Start  End    Size   File system  Name  Flags
 2      3s     32s    30s
 1      80s    1679s  1600s                     boot

node-gpt detects 122 partitions with incoherent sizes, offsets and names

At a guess, I wonder if node-gpt isn't detecting that the backup GPT is corrupt, and is reading the incoherent info from the corrupt backup?

Although it does seem weird to see GPT used inside an ISO - maybe this is another weird hybrid ISO format? 🤷

@lurch
Copy link
Contributor

lurch commented Jun 18, 2020

A mixture of curiosity and boredom led me to try and match up parts of https://github.com/jhermsmeier/node-gpt/blob/master/lib/gpt.js with https://en.wikipedia.org/wiki/GUID_Partition_Table 😉
It's not immediately obvious why parse has two separate calls to parseTable?

parse seems to effectively be doing:

offset1 = offset + tableOffset
offset2 = offset1 + this.blockSize
offset3 = offset1 + this.tableSize
// Parse partition tables
this.parseTable( buffer, offset1, offset2 )
this.parseTable( buffer, offset2, offset3 )

and then parseTable uses while( ( offset + this.entrySize ) <= end ) which implies that the code in parse (if it's correct) could be collapsed to just this.parseTable( buffer, offset1, offset3 ) ??
I'm sure there's probably something obvious I'm missing though, given that I've not studied GPT in depth 😉 🤷

@lurch
Copy link
Contributor

lurch commented Jun 18, 2020

Oooh, if I change https://github.com/jhermsmeier/node-gpt/blob/master/lib/gpt.js#L162 to use Math.min instead of Math.max then node-gpt correctly reports only two partitions in FreeNAS-11.3-U3.2.iso 🎉 But that's only a naive guess at a fix so I dunno if it breaks anything else!! 🤣

@lurch
Copy link
Contributor

lurch commented Jun 18, 2020

Hmmm.... after a bit more background-reading, maybe a this is a "better" fix:

--- a/lib/gpt.js
+++ b/lib/gpt.js
@@ -264,8 +264,9 @@ GPT.prototype = {
     var tableOffset = ( this.tableOffset - this.currentLBA ) * this.blockSize
 
     // Parse partition tables
-    this.parseTable( buffer, offset + tableOffset, offset + tableOffset + this.blockSize )
-    this.parseTable( buffer, offset + tableOffset + this.blockSize, offset + tableOffset + this.tableSize )
+    var tableStart = offset + tableOffset
+    var tableEnd = tableStart + ( this.entries * this.entrySize )
+    this.parseTable( buffer, tableStart, tableEnd )
 
     return this

🤷‍♂️

@lurch
Copy link
Contributor

lurch commented Jun 18, 2020

More curiosity led me to do a bit more research... 😉

It's not immediately obvious why parse has two separate calls to parseTable?

I see there's a comment in the README which says:

  // We need to parse the first 4 partition entries & the rest separately
  // as the first 4 table entries always occupy one block,
  // with the rest following in subsequent blocks

However I wonder if this is simply @jhermsmeier misinterpreting the diagram on the Wikipedia page ? The diagram shows LBA0 as the "Protective MBR", LBA1 as the "Primary GPT Header", LBA2 as "Entry1, Entry2, Entry3, Entry4", and LBAs up to 33 as "Entries5-128". Reading the comment above, I wonder if Jonas has assumed that LBA3 is Entry5, LBA4 is Entry6, etc.?
Whereas (AFAICT) LBA3 is "Entry5, Entry6, Entry7, Entry8", LBA4 is "Entry9, Entry10, Entry11, Entry12", etc.

I.e. There's nothing "special" about the first 4 partition entries, so the two separate calls to parseTable could just be collapsed into a single call?
(with the same change also being made in other places where this assumption has been made, e.g. checksumTable, writeBackup, etc.)

@jhermsmeier
Copy link
Owner

jhermsmeier commented Jun 24, 2020

Fixed in gpt@2.0.3 gpt@2.0.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants